Use Finder and RemoteFinder in Metro Find Bar

RESOLVED FIXED in Firefox 29

Status

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: evilpie, Assigned: casper.vandonderen)

Tracking

Trunk
Firefox 29
All
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mbrubeck][lang=js] [feature] p=0)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
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.
Whiteboard: [mentor=mbrubeck][lang=js]

Updated

6 years ago
Summary: Use Finder and RemoteFinder → Use Finder and RemoteFinder in Metro Find Bar
(Assignee)

Comment 2

6 years ago
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?
(Assignee)

Updated

6 years ago
Flags: needinfo?(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.
Flags: needinfo?(mbrubeck)
Assignee: nobody → casper.vandonderen
Blocks: metrofxe10s
OS: Linux → Windows 8
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 4

6 years ago
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?
Attachment #8340334 - Flags: review?(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.
Attachment #8340334 - Flags: review?(mbrubeck) → review-
(Assignee)

Comment 6

6 years ago
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?
Attachment #8340334 - Attachment is obsolete: true
Attachment #8341649 - Flags: review?(mbrubeck)
Flags: needinfo?(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.)
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 8

6 years ago
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 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.)
Attachment #8341649 - Flags: review?(mbrubeck) → review+
Blocks: metrobacklog
Whiteboard: [mentor=mbrubeck][lang=js] → [mentor=mbrubeck][lang=js] [feature] p=0
Just setting a reminder to myself to finish the follow-up work and then check in this patch.
Flags: needinfo?(mbrubeck)
Effort estimation: Leaving p=0 because there is no more work to do in this bug.
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.)
Attachment #8357392 - Flags: review?(msamuel)
Attachment #8357392 - Flags: review?(evilpies)
Flags: needinfo?(mbrubeck)
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"); ...
(Reporter)

Comment 14

5 years ago
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.
Attachment #8357392 - Flags: review?(evilpies) → review+
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.
Attachment #8357392 - Attachment is obsolete: true
Attachment #8357392 - Flags: review?(msamuel)
Attachment #8357598 - Flags: review?(msamuel)
Flags: needinfo?(evilpies)
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.
Attachment #8357598 - Flags: review?(msamuel) → review+
(Reporter)

Comment 17

5 years ago
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.
Flags: needinfo?(evilpies)
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
Status: NEW → ASSIGNED
Blocks: 958101
Blocks: 958111
https://hg.mozilla.org/mozilla-central/rev/faa87475ab44
https://hg.mozilla.org/mozilla-central/rev/18acca8fc681
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
No longer blocks: metrobacklog

Updated

5 years ago
Depends on: 952993

Updated

3 years ago
Depends on: 1236972
You need to log in before you can comment on or make changes to this bug.