Support the new findbar API

chrome/content/ui/filters-search.js

Index: chrome/content/ui/filters-search.js
===================================================================
--- a/chrome/content/ui/filters-search.js
+++ b/chrome/content/ui/filters-search.js
@@ -27,7 +27,30 @@
   init: function()
   {
     let findbar = E("findbar");
-    findbar.browser = FilterSearch.fakeBrowser;
+
+    // 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.addEventListener("keypress", function(event)
     {
@@ -129,28 +152,74 @@
 };
 
 /**
- * Fake browser implementation to make findbar widget happy - searches in
- * the filter list.
+ * Finder implementation that 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 =
 {
   fastFind:
   {
-    searchString: null,
+    get searchString()
+    {
+      return FilterSearch.finder.searchString;
+    },
+
+    set searchString(searchString)
+    {
+      FilterSearch.finder.searchString = searchString;
+    },
+
     foundLink: null,
     foundEditable: null,
-    caseSensitive: false,
+
+    get caseSensitive()
+    {
+      return FilterSearch.finder.caseSensitive;
+    },
+
+    set caseSensitive(caseSensitive)
+    {
+      FilterSearch.finder.caseSensitive = caseSensitive;
+    },
+
     get currentWindow() FilterSearch.fakeBrowser.contentWindow,
 
     find: function(searchString, linksOnly)
     {
-      this.searchString = searchString;
-      return FilterSearch.search(this.searchString, 0, this.caseSensitive);
+      FilterSearch.finder.fastFind(searchString, linksOnly);
     },

[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)
     {
-      return FilterSearch.search(this.searchString, findBackwards ? -1 : 1, this.caseSensitive);
+      FilterSearch.finder.findAgain(findBackwards, linksOnly);
     },
 
     // Irrelevant for us