Support the new findbar API

chrome/content/ui/filters-search.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /**
  * Implementation of the filter search functionality.
  * @class
  */
 var FilterSearch =
 {
   /**
    * Initializes findbar widget.
    */
   init: function()
   {
     let findbar = E("findbar");
-
-    // The findbar API changed in Firefox 27, we still support the old one for
-    // backwards compatibility.
-    let oldFindBar = false;
-    try
-    {
-      Components.utils.import("resource://gre/modules/Finder.jsm", {});
-    }
-    catch (e)
-    {
-      oldFindBar = true;
-    }

[Wladimir Palant, 2014/03/06 13:20:51]

Does this check make sense? From the look of it, the following should always
work:

findbar.browser = FilterSearch.fakeBrowser;

We merely have to make sure that finder is a property of our fakeBrowser.

[Felix Dahlke, 2014/03/10 22:19:56]

Yeah, we can just use fakeBrowser instead of filtersContainer, makes things
quite a bit simpler. Did that.

-
-    if (oldFindBar)
-      findbar.browser = FilterSearch.fakeBrowser;
-    else
-    {
-      let container = document.getElementById("filtersContainer");

[Wladimir Palant, 2014/03/06 13:20:51]

We have a shortcut function for that, E("filtersContainer").

[Felix Dahlke, 2014/03/10 22:19:56]

Got rid of that code anyway.

-      container.finder = FilterSearch.finder;
-      findbar.browser = container;
-
-      // Without this, the initial search string will be "undefined"
-      findbar._findField.value = "";

[Wladimir Palant, 2014/03/06 13:20:51]

That's a rather unreliable hack. Instead please add a _lastSearchString property
FilterSearch.fakeBrowser instead, it should return
FilterSearch.finder.searchString.

[Felix Dahlke, 2014/03/10 22:19:56]

Done.

-    }
+    findbar.browser = FilterSearch.fakeBrowser;
 
     findbar.addEventListener("keypress", function(event)
     {
       // Work-around for bug 490047
       if (event.keyCode == KeyEvent.DOM_VK_RETURN)
         event.preventDefault();
     }, false);
 
     // Hack to prevent "highlight all" from getting enabled
     findbar.toggleHighlight = function() {};
@@ skipping 81 matching lines @@
           return result;
         }
       }
     }
 
     return Ci.nsITypeAheadFind.FIND_NOTFOUND;
   }
 };
 
 /**
- * Finder implementation that searches in the filter list.
+ * Fake browser implementation to make findbar widget happy - searches in
+ * the filter list.
  */
-FilterSearch.finder =
-{
-  searchString: null,
-  caseSensitive: false,
-
-  fastFind: function(searchString, linksOnly, drawOutline)
-  {
-    this.searchString = searchString;
-    FilterSearch.search(this.searchString, 0, this.caseSensitive);

[Wladimir Palant, 2014/03/06 13:20:51]

The return value of the search function should not be ignored. The
backwards-compatible implementation needs to return it immediately, the new one
calls listeners instead. Otherwise there won't be any notifications about
nothing being found or search wrapping.

[Felix Dahlke, 2014/03/10 22:19:56]

Done.

-  },
-
-  findAgain: function(findBackwards, linksOnly, drawOutline)
-  {
-    FilterSearch.search(this.searchString, findBackwards ? -1 : 1,
-                        this.caseSensitive);
-  },
-
-  // Irrelevant for us
-  addResultListener: function(listener) {},
-  removeResultListener: function(listener) {},

[Wladimir Palant, 2014/03/06 13:20:51]

Actually, the listeners are not irrelevant - that's how we notify the findbar
about nothing being found.

[Felix Dahlke, 2014/03/10 22:19:56]

Done.

-  highlight: function(highlight, word) {},
-  enableSelection: function() {},
-  focusContent: function() {}

[Wladimir Palant, 2014/03/06 13:20:51]

What about removeSelection and keyPress? The latter would actually need a
non-trivial implementation.

[Felix Dahlke, 2014/03/10 22:19:56]

Neither is actually being called in practice, at least not for me. Think we
should have stubs for those to be on the safe side?

[Wladimir Palant, 2014/03/13 11:40:01]

Yes, we definitely need stubs at the very least -
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar...
is calling them. It seems that removeSelection() is currently only being called
by the API provided by the findbar widget (and that we don't call) but this can
change.

[Felix Dahlke, 2014/03/18 15:51:50]

Added stubs for removeSelection and keyPress. Judging from findbar.xml, stubs
should be fine for both. Also, the patch Mike de Boer created for Stylish only
has stubs for those as well:
https://bugzilla.mozilla.org/attachment.cgi?id=8369419&action=edit

-};
-
-/**
-  * Fake browser implementation to make findbar widget happy - searches in
-  * the filter list.
-  */
 FilterSearch.fakeBrowser =
 {
+  finder:
+  {
+    _resultListeners: [],
+    searchString: null,
+    caseSensitive: false,
+    lastResult: null,
+
+    _notifyResultListeners: function(result, findBackwards)
+    {
+      this.lastResult = result;
+      for each (let listener in this._resultListeners)
+        listener.onFindResult(result, findBackwards);
+    },
+
+    fastFind: function(searchString, linksOnly, drawOutline)
+    {
+      this.searchString = searchString;
+      let result = FilterSearch.search(this.searchString, 0,
+                                       this.caseSensitive);
+      this._notifyResultListeners(result, false);
+    },
+
+    findAgain: function(findBackwards, linksOnly, drawOutline)
+    {
+      let result = FilterSearch.search(this.searchString,
+                                       findBackwards ? -1 : 1,
+                                       this.caseSensitive);
+      this._notifyResultListeners(result, findBackwards);
+    },
+
+    addResultListener: function(listener)
+    {
+      if (this._resultListeners.indexOf(listener) === -1)
+        this._resultListeners.push(listener);
+    },
+
+    removeResultListener: function(listener)
+    {
+      let index = this._resultListeners.indexOf(listener);
+      if (index !== -1)
+        this._resultListeners.splice(index, 1);
+    },
+
+    // Irrelevant for us
+    highlight: function(highlight, word) {},
+    enableSelection: function() {},
+    removeSelection: function() {},
+    focusContent: function() {},
+    keyPress: function() {}
+  },
+
+  get _lastSearchString()
+  {
+    return this.finder.searchString;
+  },
+
+  // This was used before Firefox 27 instead of the "finder" property.
   fastFind:
   {
     get searchString()
     {
-      return FilterSearch.finder.searchString;
+      return FilterSearch.fakeBrowser.finder.searchString;
     },
 
     set searchString(searchString)
     {
-      FilterSearch.finder.searchString = searchString;
+      FilterSearch.fakeBrowser.finder.searchString = searchString;
     },
 
     foundLink: null,
     foundEditable: null,
 
     get caseSensitive()
     {
-      return FilterSearch.finder.caseSensitive;
+      return FilterSearch.fakeBrowser.finder.caseSensitive;
     },
 
     set caseSensitive(caseSensitive)
     {
-      FilterSearch.finder.caseSensitive = caseSensitive;
+      FilterSearch.fakeBrowser.finder.caseSensitive = caseSensitive;
     },
 
     get currentWindow() FilterSearch.fakeBrowser.contentWindow,
 
     find: function(searchString, linksOnly)
     {
-      FilterSearch.finder.fastFind(searchString, linksOnly);
+      FilterSearch.fakeBrowser.finder.fastFind(searchString, linksOnly);
+      return FilterSearch.fakeBrowser.finder.lastResult;
     },

[Wladimir Palant, 2014/03/06 13:20:51]

How about:

  find: FilterSearch.finder.bind(FilterSearch.finder)

And the same for findAgain.

[Felix Dahlke, 2014/03/10 22:19:56]

Not relevant anymore since the code changed.

 
     findAgain: function(findBackwards, linksOnly)
     {
-      FilterSearch.finder.findAgain(findBackwards, linksOnly);
+      FilterSearch.fakeBrowser.finder.findAgain(findBackwards, linksOnly);
+      return FilterSearch.fakeBrowser.finder.lastResult;
     },
 
     // Irrelevant for us
     init: function() {},
     setDocShell: function() {},
     setSelectionModeAndRepaint: function() {},
     collapseSelection: function() {}
   },
   currentURI: Utils.makeURI("http://example.com/"),
   contentWindow:
@@ skipping 19 matching lines @@
   removeEventListener: function(event, handler, capture)
   {
     E("filtersTree").addEventListener(event, handler, capture);
   },
 };
 
 window.addEventListener("load", function()
 {
   FilterSearch.init();
 }, false);