Latest topics

XUL/Migemoのリファクタリングとか改善とか - May 04, 2008

pXMigemoFind（pIXMigemoFindの実装）、特にfindメソッドまわりを中心にだいぶ書き直した。といってもアルゴリズム的には変わってなくて、主にメンテナンス性を向上することを目的にした書き換えです。こういうのもリファクタリングと言っていいんでしょうか。

findInDocumentメソッドがかなり長くて中のループのネストも深くなってたので、これをだいぶ細かく分けた。

まず、フレームのツリーを辿る処理がループの最後の方にどかっとあったので、これを取り出してIteratorパターンのヘルパーオブジェクトとして実装することにした（最後の方にあるDocShellIteratorというやつ）。最初Iteratorパターンというのを知った時には「こんなの何の役に立つんだ？」とか「配列みたいに長さを取り出せた方が便利じゃね？」とか思ってたけど、こうして実際に書いてみると、「次に処理対象にする物を探す」部分だけに特化して作り込めるので便利だ、ということがよく分かった。

具体的には、今までは前方検索と後方検索それぞれの場合で「一つ前のフレーム」に処理を移すのか「一つ後のフレーム」に処理を移すのかをいちいち判別してたんだけど、その判別を行う部分まで含めてDocShellIteratorとして分離することで、findInDocumentの側では何も考えずにDocShellIteratorのiterateNextメソッドを呼べば適切な結果が帰ってきてウマーという風にできるようになった。

実際のコードで見ると、findInDocument側は

docShell = this.getDocShellForFrame(doc.defaultView)
  .QueryInterface(Components.interfaces.nsIDocShellTreeNode);

if (aFindFlag & this.FIND_BACK) { // back
  docShell = this.getPrevDocShell(docShell);
  if (!docShell) {
    if (!(aFindFlag & this.FIND_WRAP)) {
      docShell = this.getDocShellForFrame(doc.defaultView.top);
      docShell = this.getLastChildDocShell(docShell.QueryInterface(Components.interfaces.nsIDocShellTreeNode));
      doc = docShell
        .QueryInterface(Components.interfaces.nsIDocShell)
        .QueryInterface(Components.interfaces.nsIWebNavigation)
        .document;
      this.document.commandDispatcher.focusedWindow = docShell
        .QueryInterface(Components.interfaces.nsIDocShell)
        .QueryInterface(Components.interfaces.nsIWebNavigation)
        .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
        .getInterface(Components.interfaces.nsIDOMWindow);
      if (
        !editableInOut ||
        findRange.sRange.startContainer == aDocument.body ||
        findRange.sRange.startContainer == aDocument.documentElement
        )
        aFindFlag |= this.FIND_WRAP;
      continue;
    }
    this.dispatchProgressEvent(found, aFindFlag);
    break doFind;
  }
}
else { // forward
  docShell = this.getNextDocShell(docShell);
  if (!docShell) {
    if (!(aFindFlag & this.FIND_WRAP)) {
      doc = Components.lookupMethod(doc.defaultView.top, 'document').call(doc.defaultView.top);
      this.document.commandDispatcher.focusedWindow = doc.defaultView.top;
      if (
        !editableInOut ||
        findRange.sRange.endContainer == aDocument.body ||
        findRange.sRange.endContainer == aDocument.documentElement
        )
        aFindFlag |= this.FIND_WRAP;
      continue;
    }
    this.dispatchProgressEvent(found, aFindFlag);
    break doFind;
  }
}
doc = docShell
    .QueryInterface(Components.interfaces.nsIDocShell)
    .QueryInterface(Components.interfaces.nsIWebNavigation)
    .document;
if (doc == aDocument) {
  this.dispatchProgressEvent(found, aFindFlag);
  break doFind;
}

こーんなだったのが

aDocShellIterator.iterateNext();

if (aDocShellIterator.wrapped) {
  if (!(aFindFlag & this.FIND_WRAP)) {
    this.document.commandDispatcher.focusedWindow = aDocShellIterator.view;
    if (
      !editableInOut ||
      !rangeSet ||
      aDocShellIterator.isRangeTopLevel(rangeSet.range)
      )
      aFindFlag |= this.FIND_WRAP;
    continue;
  }
  this.dispatchProgressEvent(aFindFlag, resultFlag);
  break;
}

if (aDocShellIterator.isInitial) {
  this.dispatchProgressEvent(aFindFlag, resultFlag);
  break;
}

ここまでスッキリしました。まあその代わり、フレームのツリーを辿る処理（DocShellIteratorの方がちょっと長くなってしまったんだけど、それぞれロジック的には綺麗に分かれているから、今後手を入れる時は「ツリーを辿ること」と「検索すること」のどっちか一方のことだけに集中して作業できるわけで、これこそがイテレータパターンの最も大きな意義だったんですね。デザインパターン万歳。

あと、findInDocumentの中でループの中でループを回していた部分をfindInDocumentInternalメソッドとして分離した。これはロジックを分離する効果はなくて、単にネストを浅くしてコードを見やすくするためだけの変更。のつもりだったんだけど、これをうまくやるために、他の部分も含めてだいぶ手直ししないといけなかった。

何故かというと、単純に内側のループだけをfindInDocumentInternalとして分離したところで、元のfindInDocumentとfindInDocumentInternalとの間でお互いに引き渡さないといけない情報（引数として引き渡す情報と、返り値として戻す情報の両方）が多すぎて、そのまま二つに分けただけだと却って分かりにくくなってしまったから。普通にreturnするだけじゃ情報を渡しきれないから、オブジェクトを引数で渡して、そのオブジェクトのプロパティとして返り値を設定して、変更されたプロパティを呼び出し元の側でまた参照して……という感じになってしまって、せっかくメソッドを分けたのにそこの所でガッツリ繋がってしまってて、これじゃ分けた意味がないよと。

というわけで最低限必要な情報だけに絞り込んでやりとりするように設計を検討したのに加えて、インターフェースの定義も見直して、今まであんまり有効活用してなかったビット演算を多用するようにしたことで、findInDocumentInternalからの返り値は最終的にビット列一つだけにまでまとめることができた。ビット演算いいよビット演算。

ビット演算を有効に利用するためには、どの意味をどのビットに与えるかというのをよく考えておかないといけない。とかなんとか大仰な言い方をしてみたけど、要するに、適当に定数プロパティを名前順に並べて先頭から0, 1, 2, 4, 8……と値を割り振っていくなんていいかげんな事してちゃ駄目だよってことですね。0.8.1まででは

const unsigned short FOUND             = 0;
const unsigned short NOTFOUND          = 1;
const unsigned short WRAPPED           = 2;
const unsigned short NOTLINK           = 4;
const unsigned short FOUND_IN_EDITABLE = 8;

こんなだったけど、これじゃあ実質的にはただの定数値が並んでるだけで単純比較にしか使えない。FOUNDが0だったり「検索はヒットしたけどリンクじゃないからヒット無しとみなす」という意味のNOTLINKなんてのがあったり。一つの値に複数の意味が割り当てられてるんじゃ、重複しないビットを割り当てても意味がないわけです。

const unsigned short NOTFOUND          = 0;
const unsigned short FOUND             = 1;
const unsigned short WRAPPED           = 2;
const unsigned short FOUND_IN_LINK     = 4;
const unsigned short FOUND_IN_EDITABLE = 8;

こーいう風にしておけば、NOTLINKに相当する場合は「FOUND | WRAPED」、そうでない場合には「FOUND | FOUND_IN_LINK」とかそんな風に書けるわけで、これなら「普通にヒットした」「普通にヒットしたけど一度ページの末尾まで検索してもう一度頭からやり直した」「入力欄の中でヒットした」などなどいろんな場合もひっくるめて全部「flag & FOUND」というビット演算いっこで判定できる。

あとSafari風強調表示の改善もあるんだけど、長くなるから別のエントリに分けます。