メインコンテンツへスキップ

Mozilla の「スーパーレビュー」

Brendan Eich 、 Mitchell Baker

mozilla.org では、コードが mozilla.org CVS リポジトリーにチェックインされる前に、モジュールオーナーおよびピアレビューが義務づけられています。Mozilla ツリーにチェックインする際には、ほとんどの（全てではありませんが）場合、チェックイン前のコードレビュー 【原文：pre-check-in code review】 も必要です。このレビュー作業は、 reviewers@mozilla.org として知られる、任命された強力なハッカー集団によって行われます。このレベルでのレビュー作業が、「スーパーレビュー」 と呼ばれるものとなったのです。mozilla.org のスタッフはこの言葉を特に気に入っているわけではありませんが、どうやらこの言葉が広く使われているようです。

最近では 統合レビュー 【原文：integration review】 がスーパーレビューの替わりによく利用されます。この呼び名に皆さんはどんな印象を持ちますか？　皆さんの考えを brendan@mozilla.org で表明してください。

スーパーレビューの目的は、コードの質を高め、最善の開発成果を取り入れ、開発の逆行を防ぐことにあります。このレベルで行われるレビューでは、コードそのものの質を検査し、他のツリー部分への影響を予測し、インターフェースの仕様、そして Mozilla コーディングガイドラインに沿っているかを確認します。場合によっては （真っ先に思い浮かぶのが暗号化です）、スーパーレビューの担当者たちがその領域のコード品質を評価できるほど専門性を備えていない場合もあります。しかしその場合でも、その他の要素については検査をします。

どのような場合においても、スーパーレビューの担当者たちは必要な場合に、「code」 をはじめ 「design」 の赤旗を立てたり、改良を要求する権限を持っています。mozilla.org にはコードレビューに先だったデザインレビューを別途に求めているわけではないため、こうした作業が必要となるのです。その代わり、ハッカー達が自分たちのデザインについて話し合い、仕様を定めたり、目的についてざっくばらんに評価し合って、デザイン・実装段階の全て（普通は秩序だったものではありません）においてフィードバックを得られると考えています。

reviewers@mozilla.org というメーリングリストがあり、お決まりの模範的なネーミング mail gateway という名前で ニュースグループ でもミラーリングされています。このメール/ニュース設定は、他のレビュー担当者や関心のあるオブザーバーがレビューの要請やレビュー担当者からの反応をみることができるように設けられています（ただ、多くの場合レビュー担当者からの積極的な反応はニュースグループへのメッセージとしてよりも、 Bugzilla 【訳注：日本語版】でのコメントとして得られます）。このニュースグループ/メールエイリアスは、スーパーレビューへの変更を提案するために利用してください。関心を持った外部の人たちが状況を知り手助けをできるようにするためです。

スーパーレビューが必要かどうかは、どのコードをハッキングするのかによります。

どのコードがスーパーレビューを必要とするのでしょう？

  1. 現在のルールは：「in-process」 の状態になっているコードか、 Mozilla browser/mail/news/editor アプリケーションと共にビルドされているスウィートであるコードは、スーパーレビューが必要です。
  2. スーパーレビューを必要としない mozilla.org プロジェクトには次のものが含まれます：ウェブツール （例えば Bugzilla）、ネットワークセキュリティシステム （「NSS」）、クライアントカスタマイズキット （「CCK」） および Rhino。ただし、モジュールオーナーやピアレビューを受けたり、特定のプロジェクトで定められたその他のルールには従わなければなりません。
  3. mozilla.org には現時点で in-process となっていないコードがありますが、将来 in-process になるコードがあります。mozilla.org は、このようなコードもスーパーレビューを受けることを強く求めています（でも義務ではありません）。これは、現時点では Mozilla と共にビルドされておらず、将来ビルドに組み込まれる予定のコードについても当てはまります。このようにしないと、コードが準備できたときレビューをしなければならないものが大量になってしまいますし、そうなると永遠にレビューが終わらなくなってしまいます。遅い段階でのレビューでは、さらに悪いことになってしまいます。簡単に事前に見つけることができても解決にはかなりの努力を必要とする問題を見つけられなくなる可能性があるのです。このカテゴリーでは、最初プロセス外 【原文：out of process】 でプロジェクトが開始され、最終的には in-process となるであろうパーソナルセキュリティマネージャ (「PSM」) などのようなプロジェクトを指します。
  4. 例外もあります。当然ながら基本ルールにはいくつかの例外があります。（いつもそうですよね？）

例外

blizzard@mozilla.org は、多くのプラットフォーム固有部分に関連する分野に登場しますが、全てのプラットフォームにわたって専門知識を持っているわけではありません。彼は、ウィジェット、 gfx に関する決定を下してきました。ただし、お互いによく似たプラットフォーム関連コードに関しては、彼の個人的なレビューは必要なく、モジュールオーナーによるレビューで十分です。また blizzard@mozilla.org は、設計段階でのポートのためのフィードバックのため、API の不整合問題については必ず知りたいと考えています。また、buck をどの時点で終えるのか、全てのプラットフォーム関連の決定について、彼が最終判断の権限を持っています。

cls@seawood.org は、Unix、OS2、そして他の gmake を利用したプラットフォーム向けの autoconf のモジュールオーナーです。上記のシステム向けファイルへの変更をチェックインするには、 彼のレビューがあれば十分です。 staff@mozilla.org は、この分野でのそれ以上のレビューを求めていません。

NSPR は熟成されて安定しており、長年にわたって Mozilla の先行バージョンでも利用されてきました。これらのファイルをチェックインするためには、ピアおよびモジュールオーナーによるレビューで十分です。staff@mozilla.org ではそれ以上のレビューを求めていません。

tao@netscape.com は、mozilla/l10n （Mozilla ローカライゼーション） を担当しています。ローカライズされたコンテンツに関する変更のみをファイルをチェックインするためには、ピアおよびモジュールオーナーによるレビューで十分です。staff@mozilla.org ではそれ以上のレビューを求めていません。

NSPR のような JavaScript エンジンは、もう一つの閉じられた CVS パーティション（NSPR）への依存のみを持つ、完成されたコードが入っている閉じた CVS パーティションです。 JS コードを変更するためにはモジュールオーナーのレビューで十分です。

モジュール PSM への変更は、オーナー / ピアによるレビューが少なくとも 1 回は必要です。変更を求めている人がオーナーでもピアでもない場合、PSM オーナー、PSM ピア、スーパーレビューアといういずれかのグループに属する第 2 者が変更を承認しなければなりません。すべての Mozilla アプリケーションで共有されている UI （例：エラーメッセージ、パスワードプロンプト、情報ウィンドウ、証明書マネージャ） への変更は、Mozilla Foundation の Unser Experience チームによるレビュー / 承認が必要です。主要なアプリケーションウィンドウにある UI （例：Firefox の設定画面、メールのセキュリティボタン、ステータスバーの項目） への変更は、各アプリケーションの UI オーナーによるレビュー / 承認が必要です。

一般的に、きちんと管理されたモジュールはスーパーレビューの条件から免除されるように スーパーレビューア達 に要求することができます。閉じた CVS パーティションに対してのみ依存するどのような免除モジュールも、ビルドできないようなバグや他の予期しない問題を実際場面で修正することができる十分な peer がいる限り、閉じた CVS モジュールとなることができます。誰かが CVS へのアクセス権を悪用しているとは考えていませんが、パーティションを閉じることで、強力なオーナーシップを強調することができると共に、偶発的なチェックインによる損害を抑えることができます。

モジュール（つまりパーティション）についての詳しい情報は despot.cgi を参照してください。

スーパーレビューが必要ですか？

あなたが誰であれ （そうなんです。次のルールはモジュールオーナーにも Mozilla スーパーレビューアにも当てはまります）、レビューが必要な場合は、レビュー担当者にメールを送る前に次のリストをチェックしてください。

  1. パッチ （あるいは、 Mozilla の拡張に関しては追加された機能）が対応するバグの症状とあなたの診断を説明するファイルに、そのバグが記述されているかを確認してください。
  2. チェックイン前のテスト （pre-checkin test）およびあなたが作成したパッチが作動するか、その他のテストを行ってください。
  3. cvs diff -u フォーマットを使って、そのバグに対応したパッチを最低限添付してください（より読みやすい diff と共に添付ファイルを付けても構いません）。
  4. パッチが何をするのかやパッチを作成した理由を、バグの comment 欄に分かりやすく書いてください。箇条書きはやめてください。というのも、変更が明瞭でない場合は、パッチ内のコードに説明用のコメントがあるはずだからです。
  5. モジュールオーナー や誰でも見つけることができたピアレビュー担当者にコードをレビューしてもらってください。オーナーは、バグのパッチフラッグを review+ に設定します。
  6. 下の ルールとヒント を読んで、重要な部分を再度確認してください。

モジュールオーナーは自分のパッチをピアレビューしてもらいましょう。決して自己点検で済まして、問題が起こらないようにと祈るようなことはしないでください。

下のリスト から適切な スーパーレビューアを選んでください。バグのパッチフラッグを使って、選んだ スーパーレビューアに対してレビューのリクエストをしてください。 super-review で ? を設定して、レビューアの名前を記入してください。

スーパーレビューアに連絡を取る

以下に上げた人たちは、 mozilla.org で strong hackers と呼ばれ、ユニバーサルコードレビューを担当しています：

担当分野ごとにインデックス化して、 primary または most-expert reviewer そして aggregating reviewer を mailto: リンクを加えてソートすると次のリストようになります。スーパーレビューには担当分野の専門知識が必要なわけではありません（大抵の場合、モジュールオーナーやピアレビューアーが専門知識を提供します）。下に示した分野は整理棚ではありません。皆さんは下のリストの誰にでもスーパーレビューを要請しても構いません。ただ多くの場合において、リストに掲げられた分野のレビューアーを指名するとより早く返事が得られるでしょう。

browser
alecf@flett.org、firefox@blakeross.com、jag@tty.nl
cache
darin@meer.net
composer
neil@parkwaycc.co.uk
content、layout
dbaron@dbaron.org, roc@ocallahan.org、rbs@maths.uq.edu.au、bzbarsky@mit.edu、bryner@brianryner.com, jst@mozilla.jstenback.com (content、dom)、peterv@propagandism.org (content)
directory
dmose@mozilla.org
docshell、webshell
mscott@mozilla.org
dom
jst@mozilla.jstenback.com, peterv@propagandism.org
drag and drop
blizzard@mozilla.org
editor
kinmoz@netscape.net、 sfraser@aol.net
embedding
blizzard@mozilla.org
event loop
blizzard@mozilla.org
gfx
rbs@maths.uq.edu.au、blizzard@mozilla.org
htmlparser
jst@mozilla.jstenback.com
image libraries
tor@acm.org
intl (i18n)
blizzard@mozilla.org
js
brendan@mozilla.org, shaver@mozilla.org
mailnews
bienvenu@nventure.com, mscott@mozilla.org、sspitzer@mozilla.org、dmose@mozilla.org、Henry.Jia@sun.com
netwerk
darin@meer.net、 bzbarsky@mit.edu
strings
scc@mozilla.org、jag@tty.nl
uriloader
mscott@mozilla.org
widget
blizzard@mozilla.org、bryner@brianryner.com
xpcom
brendan@mozilla.org, scc@mozilla.org、shaver@mozilla.org、Henry.Jia@sun.com
xpconnect
shaver@mozilla.org、 brendan@mozilla.org
xpfe
neil@parkwaycc.co.uk、alecf@flett.org、jag@tty.nl
xpinstall
dveditz@cruzio.com
xul、xbl
hyatt@mozilla.org, bryner@brianryner.com
catch-all、when in doubt
brendan@mozilla.org、dveditz@cruzio.com、scc@mozilla.org

注記： ben@bengoodger.com はスーパーレビューを認可する権限を持っていますが、パッチのレビューは現在行っていません。

ルールとヒント

mozilla.org が全ての開発者に向けてお勧めするドキュメントは、 SeaMonkey Code Reviewer's Guide そして SeaMonkey Engineering Bible です。このページは先の二つのページを私がまとめたもので、それに様々な人から提案されたり、実際に行われているヒントやルールを加えてあります。以下に 21 のルールとヒントを、質問形式に （うんざりする質問の羅列ではなくて！） まとめてあります。以下の部分で私が 「patch」 として表現しているのは、新しいコードが提供された場合の 「完全に新しいファイル」 のことを表します。

  1. モジュールオーナーと複数のピアレビュー担当者がすでにパッチを見ているかを確認してください。バグ報告の中で r=レビュー担当者のメールアドレス という形で署名が入っているはずです。モジュールピアの後、他の専門家、または reviewers@mozilla.org さらに porkjockeys@mozilla.org に依頼する必要があるかを検討してください。
  2. XP コードへの変更ですか？　それなら、その変更は 64-bit clean でしょうか？　また C++ portability guide に沿っていますか？
  3. その変更はエンドユーザーに分かりますか？　もし分かる場合は、 ローカライズ できますか？　 「ローカライズ・カスタマイズできるコードを書くには」 で定められた勧告に従っていますか？
  4. 変更にはエンドユーザの目に触れる部分である XUL ユーザインタフェースに含まれて / 影響しますか？　もし含まれている / 影響する場合は、 アクセシブル 【訳注：ユーザに優しいもの 】ですか？ アクセス可能な XUL オーサリング・ガイドライン の勧告に従っていますか？
  5. バグ がきちんと添付ファイルと一緒にあるかどうか確認してください。
  6. 関係する分野の専門家で、まだバグを読んでいないレビュー協力者には、気軽に cc: してください。
  7. パッチを生成する際には cvs diff -u 形式を使うようにパッチの作成者に勧めてください。またパッチファイルを一つにするために、最も高い位置の共通ディレクトリで cvs diff を行うようにしてください。そうすれば一回のパッチコマンドで済みます。
  8. パッチがタブやスペースによる空白部分が多く含む場合、外見のデザイン変更のみを含む場合、パッチを提出する人たちはそのパッチの cvs diff -wu バージョンを、 diff -u 形式の添付ファイルに加えて添付するようにしてください。他の人たちがパッチを当てることができるようにするためです。cvs diff -p オプションも利用してください。（diff -pu8 または何か同様のものを .cvsrc ファイルに書き込んでください。）
  9. unified コードやその他のコンテキスト diff だけでなくコード全体も見てください。 LXR があなたの友です。
  10. これまでの問題のなかで、パッチによる完全なフィックスではなく、ただ回避されているファイルにバグがありますか？
  11. パッチはどのようにテストされましたか？　 pre-checkin テストを行いましたか？　変更が大きく、分析が難しい場合は、 スモークテスト でも確認しましたか？ JS テスト や viewer といったユニットテストやスウィートはどうですか？
  12. パッチによってメモリーが割り当てられたり、割り当や既存コードのフリー構造（free constructs） が変更される場合、またデータ構造の strong references を追加、削除、または移動した場合、パッチの提出者は、 メモリーツール を作動させ、 tinderbox bloat ナンバーと比較しましたか？
  13. パッチへの作業が膨大な場合、 Mozilla コミュニティーの複数の人が、そのパッチを当て、そのパッチと共に作動させ、コメントともにバグを更新し、良い評価を得ているでしょうか？　もし好評でない場合は、 patch-buddies を探してください。
  14. 下部レベルモジュールへのパッチが 「分かりにくい形」 で上部レイヤーモジュール（特に、 XUL、XBL など） に影響を与えるような変更を含んでいますか？　もしそのような変更が含まれている場合は、パッチの提供者はそのパッチが、幅広く上位レイヤー （特に XUL ベースのアプリケーションやダイアログ）に良い結果をもたらすかどうかを実際に確認しましたか？
  15. パッチは strong XPCOM references である、 nsWeakPtr 向けに nsCOMPtr を使用し （生の C++ インターフェースポインタに対して適切な場合）、また weak XPCOM references にも使用しており、最新の string テクノロジーを使用していますか？　同じような質問が、ProgIDs や新しい contractids （まだ rayw@netscape.com たち によって contractids に変更されていないコードはどれでも）にも当てはまります。
  16. そのパッチは、はっきりとは報告されていない再入可能性やスレッド・セーフティを使用することを前提にしていますか？　もしそうなら、パッチの提供者に対して、意見を求めたり、 Mozilla ニュースグループでの議論に参加を求めたり、最終的にはドキュメントにまとめてもらえるように働きかけてください。
  17. もしパッチが新しい XPCOM インターフェースを定義している場合、そのインターフェースは XPIDL を使用して宣言していますか？　もしそうなら、そのインターフェースまたは新しいメソッド、あるいは属性は [scriptable] ですか？　もしそうでないなら、そうでない正当な理由がありますか？　いずれにしても、 JavaDoc-style コメントは、 Doxygen によって処理できるタイプのもので、インターフェース定義できちんと用いられていますか？
  18. 互換性はどうですか？　フリーズされた XPCOM インターフェースは変更してはいけません （その代わり、新しいインターフェースを作成しなければなりません）。非 XPCOM インターフェースが変更された場合、後方互換が保たれていますか？　 DOM、 XUL、 XBL、などでの変更は、 C または C++ での変更よりも大きな影響を及ぼす可能性があります。もしその変更の後方互換が保たれていない場合、既存の機能を保つような代替手段を講じてください。もし互換性を犠牲にしなければならない場合、ニュースグループおよびバグ報告で告知し、その変更が良く理解され、記録されるようにしてください。
  19. spot-fix のような仕方で特定されている不正なパターンがファイルのほかの場所、あるいは近接するファイルで繰り返されている場合、 そのパッチはコード内の問題を特定していますか？　もし特定しているなら、そしてもし マイルストーンスケジュール の日程に余裕があるのなら、パッチ提供者とモジュールオーナーに、全ての場所やファイルバグについて必要な作業を行うように要請してください。
  20. マイルストーンスケジュール の関係で、リスクを最小化する時期の場合は、できるだけ影響の少ない変更を考えて、バグを修正したり、症状を緩和してください。関連するバグがオープンにしておき、オープンな事項を追跡できるようにしておいてください （ルール/ヒント #8 以前）。
  21. コードをレビューする際、そのパッチではできるだけ一般的なモジュールスタイルに沿ってください（「郷にいれば･･･」）。もしモジュールがぐちゃぐちゃな場合は、オーナーを特定し、コードスタイルを一致させるように要請してください。モジュールがぐちゃぐちゃだということは、それ自体きちんと管理されていないことの現われなのです。それぞれのファイルのトップの Emacs モードラインコメント で、ファイルのインディケーションスタイルが正確に特定されており、indent-tabs-mode が nil となっており、適切なスペースを残して、パッチではファイルからタブを除去してください。

最後に、あなたがレビューをしてパッチを受け取り、喜んで自分の名誉を賭けようという場合は、バグ報告に sr=あなたのメールアドレス を記入して、誰もがあなたの認証印を見られるようにしてください。ミスは必ず起こるものだと思っておいてください。誰もがミスをするのですから。大切なことは完璧を目指すのでも、慎重になるあまりパッチの受理を必要以上に遅らせてしまうことでもありません。大切なことは、私達が開発しているコードに対しての正しい判断と、多くの人の判断を仰ぐことなのです。