916481 - Use Finder and RemoteFinder in Metro Find Bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Tom S [:evilpie]]

We now have a e10s compatible implementation of the findbar functions, Metro already has their own implementation of something like that with FindHandler.js. It seems to do special handling for scrolling, but maybe that could be ported.

Comment 1

[Matt Brubeck (:mbrubeck)]

If anyone would like to work on this, you can find the code here:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/FindHandler.js

The work will be similar to parts of the patches in bug 666816 or bug 916483.

Comment 2

[Casper van Donderen (Telenor)]

I would like to look at this one, to get into the JS layer in the browser. Where I am stuck now is how to get a 'Finder' object. In Finder.jsm (which is the file that is Finder, right?) it uses the same code to get mozilla\typeaheadfind, so this should be removed from the Metro code then, right?

Comment 3

[Matt Brubeck (:mbrubeck)]

Sorry, I pointed to the wrong code in comment 1.  To make this change, you can remove FindHandler.js; it will be replaced by Finder.jsm.  The main code you will be modifying is here:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/helperui/FindHelperUI.js

In particular, instead of using Browser.selectedBrowser.messageManager to send messages to the old FindHandler.js code, it should use Browser.selectedBrowser.finder to get the Finder object for the current browser, and call its methods:
http://hg.mozilla.org/mozilla-central/file/7f14d602e0c5/browser/metro/base/content/helperui/FindHelperUI.js#l156

Similar to attachment 8336981 [details] [diff] [review], you should make FindHelperUI call finder.addResultListener before calling fastFind, and add an onFindResult method to receive the results of the call.  This will replace the "FindAssist:Show" message listener in the current code.

Comment 4

[Casper van Donderen (Telenor)]

Attachment: Use Finder instead of a custom find implementation in the Metro browser.

This replicates the previous behavior using Finder, I am not sure though if it is a bug that 'search -> close find bar -> open find bar' should clear the search field, it does not do that on the desktop, is that another bug, or is that a feature?

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8340334 [details] [diff] [review]
Use Finder instead of a custom find implementation in the Metro browser.

Review of attachment 8340334 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks great!  There are a few minor changes needed, and one somewhat larger change.  See below for details.

(In reply to Casper van Donderen from comment #4)
> This replicates the previous behavior using Finder, I am not sure though if
> it is a bug that 'search -> close find bar -> open find bar' should clear
> the search field, it does not do that on the desktop, is that another bug,
> or is that a feature?

I believe this is the intended behavior for now, though we could file a separate bug to decide whether to change it to be more like desktop Firefox.

::: browser/metro/base/content/contenthandlers/FindHandler.js
@@ -59,5 @@
> -
> -    // Return the bounding selection rect in content coordinates
> -    // including the scroll offset.
> -
> -    let offset = ContentScroll.getScrollOffset(content);

Hmm, I forgot about this code, which we use to slide the content up if the result would be hidden behind the find bar otherwise.  I guess we'll need to add similar code to Finder.jsm and RemoteFinder.jsm, and have it pass the rect to the result listeners.  Sorry, this is getting more complicated than I realized!  Let me know if you'd like to work on this part too, or if we should split it into a separate bug.

@@ -73,5 @@
> -                        rangeRect.width, rangeRect.height);
> -
> -    // Ensure the potential "scroll" event fired during a search as already fired
> -    let timer = new Util.Timeout(function() {
> -      sendAsyncMessage("FindAssist:Show", {

Since you are removing the code to send this message, you can also remove the code that receives it in FindHelperUI.js (addMessageListener and receiveMessage).  That includes the "FindAssist:Hide" listener, which is apparently unused but we forgot to remove it before.

::: browser/metro/base/content/helperui/FindHelperUI.js
@@ +154,5 @@
>  
> +  search: function findHelperSearch(aValue) {
> +    if (!this._finder) {
> +      this._finder = Browser.selectedBrowser.finder;
> +      this._finder.addResultListener(this);

This is the finder for the currently selected tab.  You'll need to make sure to call removeResultListener and set _finder to null when the findbar closes, so that we will get the correct finder if the next search happens in a different tab.

@@ -158,4 @@
>    },
>  
> -  goToNext: function findHelperGoToNext() {
> -    Browser.selectedBrowser.messageManager.sendAsyncMessage("FindAssist:Next", { });

There is some code in browser/metro/base/content/browser.xul that calls goToPrevious and goToNext, which will need to be updated.  (It's used for keyboard shortcuts like Control-G.)

@@ +167,5 @@
>  
> +  searchAgain: function findHelperSearchAgain(aValue, aFindBackwards) {
> +    // This can happen if the user taps next/previous after re-opening the search bar
> +    if (!this._finder) {
> +      this.doFind(aString);

I think "doFind" should be "search" here.

Comment 6

[Casper van Donderen (Telenor)]

Attachment: Use Finder instead of a custom find implementation in the Metro browser.

This fixes the issues mentioned in the previous review. I'm not sure how to fix the things in Finder.jsm and RemoteFinder.jsm, because those files get used on all platforms right? Or is there a need to #ifdef or similar in those files to only have something extra for Metro?

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Casper van Donderen from comment #6)
> This fixes the issues mentioned in the previous review. I'm not sure how to
> fix the things in Finder.jsm and RemoteFinder.jsm, because those files get
> used on all platforms right? Or is there a need to #ifdef or similar in
> those files to only have something extra for Metro?

I think those fixes would only require passing some extra parameters to onFindResult.  Other platforms could simply ignore those parameters.

I'll go ahead and review the patch you submitted, and we can add the rect stuff to Finder.jsm in a separate patch.  (Feel free to start working on that patch if you'd like; if not then I'm happy to do it.)

Comment 8

[Casper van Donderen (Telenor)]

I think it would be better if you would do the change in Finder.jsm, because doing something with the rect might make it slower on other platforms as well right?

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8341649 [details] [diff] [review]
Use Finder instead of a custom find implementation in the Metro browser.

Review of attachment 8341649 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!  Thank you very much.  I'll try to whip up a patch to add the necessary rect-finding code to Finder and RemoteFinder, and I'll land the two patches together once that one is ready and reviewed.  (It might take me a week or two, since I'm busy with other tasks at the moment.)

Comment 10

[Matt Brubeck (:mbrubeck)]

Just setting a reminder to myself to finish the follow-up work and then check in this patch.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Effort estimation: Leaving p=0 because there is no more work to do in this bug.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Make Finder.jsm pass the selection rect to its listeners

This moves the old rect-finding code from Metro into Finder.jsm.  It also restores the Metro code to use the rect that gets passed, and adds a basic regression test for that code.  Requesting review from evilpies for the toolkit part and emtwo for the Metro part.

I also noticed one missing part from the previous patch, which is to add the "finder" attribute to Metro's remote-browser binding.  (Ultimately I hope we will get rid of the Metro remote-browser binding and switch to the one in toolkit.  I'll file a separate bug for that.)

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8357392 [details] [diff] [review]
part 2: Make Finder.jsm pass the selection rect to its listeners

>--- a/toolkit/modules/Finder.jsm
>+++ b/toolkit/modules/Finder.jsm
>@@ -4,16 +4,17 @@
> 
> this.EXPORTED_SYMBOLS = ["Finder"];
> 
> const Ci = Components.interfaces;
> const Cc = Components.classes;
> const Cu = Components.utils;
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+const Rect = Cu.import("resource://gre/modules/Geometry.jsm").Rect;

You need to pass {} as the second argument, otherwise you import stuff you apparently don't want to import.

> const Services = Cu.import("resource://gre/modules/Services.jsm").Services;

And this should just be Cu.import("resource://gre/modules/Services.jsm"); ...

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8357392 [details] [diff] [review]
part 2: Make Finder.jsm pass the selection rect to its listeners

Review of attachment 8357392 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't review the actual code that calculates the rect. The rest seems fine. I thought about adding the aRect parameter to all onFindResult listeners, but that probably doesn't scale. Maybe that function should really receive an object instead. It might be a good idea to ifdef the _getResultRect code so that it only runs on Metro.

::: toolkit/modules/RemoteFinder.jsm
@@ +113,5 @@
> +      result: aResult,
> +      findBackwards: aFindBackwards,
> +      linkURL: aLinkURL,
> +      searchString: this._finder.searchString,
> +      rect: aRect,

super nit: move before searchString.

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Make Finder.jsm pass the selection rect to its listeners (v2)

This fixes the issues found by dao and evilpie, and also fixes a problem with the test that was causing failures in later tests on Try.  New Try push: https://tbpl.mozilla.org/?tree=Try&rev=e1bc205e60ed

(In reply to Tom Schuster [:evilpie] from comment #14)
> I thought about adding the aRect parameter to all onFindResult
> listeners, but that probably doesn't scale. Maybe that function should
> really receive an object instead.

I'd be happy to change it to an object either here or in a follow-up bug, if you'd like.

> It might be a good idea to ifdef the _getResultRect code so that it
> only runs on Metro.

We can't actually use a build-time #ifdef (since desktop and metro share the same toolkit build on Windows), but we could use a run-time platform check, or Metro Firefox could set some sort of pref or flag to tell Finder.jsm to run that code.  I think we should avoid a hard-coded platform check, since this data is also potentially useful on Android, and maybe B2G, and even on desktop if we do a Chromium-style overlay find bar.  Any opinions?

The performance hit from _getResultRect on my fast-ish ultrabook is around around 0.1 to 0.2ms when the result is *not* in an editable field, and around 0.8 to 1.1ms to run when the result *is* in an editable field.  I tested on moderately complex pages like Planet Mozilla and Bugzilla.  Even on hardware that is 10x slower, I don't think this'll add perceptible latency on typical pages.  However, it could get worse on a page with a huge number of form fields.

Comment 16

[Marina Samuel [:emtwo]]

Comment on attachment 8357598 [details] [diff] [review]
part 2: Make Finder.jsm pass the selection rect to its listeners (v2)

Review of attachment 8357598 [details] [diff] [review]:
-----------------------------------------------------------------

The Metro code changes look good to me.

Comment 17

[Tom S [:evilpie]]

Matt: It would be super nice if you would change the onFindResult signature in a follow up. If we can't ifdef the code, than just leave it in. As you nicely demonstrated, a runtime check doesn't seem worth it.

Comment 18

[Matt Brubeck (:mbrubeck)]

Thanks again, Casper!  This change will be included in nightly builds within the next 2 days or so.

https://hg.mozilla.org/integration/fx-team/rev/faa87475ab44
https://hg.mozilla.org/integration/fx-team/rev/18acca8fc681

Tests passed on Try: https://tbpl.mozilla.org/?tree=Try&rev=e1bc205e60ed

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/faa87475ab44
https://hg.mozilla.org/mozilla-central/rev/18acca8fc681