Issue 5938722247802880: Support the new findbar API (Closed)
|
Created:
March 5, 2014, 9:51 p.m. by Felix Dahlke Modified:
March 18, 2014, 4:07 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionFirefox 27 introduced a new (cleaner) findbar API, replacing the old one that required faking a browser.
With this patch, we support both. Tested in Firefox 17 and 27.
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Fixed issues #Patch Set 3 : Added remaining stubs #MessagesTotal messages: 6
http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... File chrome/content/ui/filters-search.js (right): http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:41: } 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. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:47: let container = document.getElementById("filtersContainer"); We have a shortcut function for that, E("filtersContainer"). http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:52: findbar._findField.value = ""; That's a rather unreliable hack. Instead please add a _lastSearchString property FilterSearch.fakeBrowser instead, it should return FilterSearch.finder.searchString. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:165: FilterSearch.search(this.searchString, 0, this.caseSensitive); 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. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:176: removeResultListener: function(listener) {}, Actually, the listeners are not irrelevant - that's how we notify the findbar about nothing being found. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:179: focusContent: function() {} What about removeSelection and keyPress? The latter would actually need a non-trivial implementation. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:218: }, How about: find: FilterSearch.finder.bind(FilterSearch.finder) And the same for findAgain.
Addressed all issues. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... File chrome/content/ui/filters-search.js (right): http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:41: } On 2014/03/06 13:20:51, Wladimir Palant wrote: > 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. Yeah, we can just use fakeBrowser instead of filtersContainer, makes things quite a bit simpler. Did that. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:47: let container = document.getElementById("filtersContainer"); On 2014/03/06 13:20:51, Wladimir Palant wrote: > We have a shortcut function for that, E("filtersContainer"). Got rid of that code anyway. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:52: findbar._findField.value = ""; On 2014/03/06 13:20:51, Wladimir Palant wrote: > That's a rather unreliable hack. Instead please add a _lastSearchString property > FilterSearch.fakeBrowser instead, it should return > FilterSearch.finder.searchString. Done. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:165: FilterSearch.search(this.searchString, 0, this.caseSensitive); On 2014/03/06 13:20:51, Wladimir Palant wrote: > 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. Done. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:176: removeResultListener: function(listener) {}, On 2014/03/06 13:20:51, Wladimir Palant wrote: > Actually, the listeners are not irrelevant - that's how we notify the findbar > about nothing being found. Done. http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:179: focusContent: function() {} On 2014/03/06 13:20:51, Wladimir Palant wrote: > What about removeSelection and keyPress? The latter would actually need a > non-trivial implementation. 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? http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:218: }, On 2014/03/06 13:20:51, Wladimir Palant wrote: > How about: > > find: FilterSearch.finder.bind(FilterSearch.finder) > > And the same for findAgain. Not relevant anymore since the code changed.
http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... File chrome/content/ui/filters-search.js (right): http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:179: focusContent: function() {} 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.
http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... File chrome/content/ui/filters-search.js (right): http://codereview.adblockplus.org/5938722247802880/diff/5649050225344512/chro... chrome/content/ui/filters-search.js:179: focusContent: function() {} On 2014/03/13 11:40:01, Wladimir Palant wrote: > 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. 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
LGTM 
|
