916483 - Replace usage of fastFind with finder

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tom S. (please needinfo tschuster)]

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/FindHelper.js still uses fastFind, changing this to finder should be quite easy.

Comment 1

[jones3.hardy]

I'd like to take on this one.  It seems simple enough to get my feet wet with the whole process.  I just have a few questions: should I just make the changes requested and create a patch, or is there something else that needs to be done?  Also, is it only this file that needs the changes?  And finally, what is the purpose for the change from `fastFind` to `finder`?

Comment 2

[:Margaret Leibovic]

(In reply to jones3.hardy from comment #1)
> I'd like to take on this one.  It seems simple enough to get my feet wet
> with the whole process.  I just have a few questions: should I just make the
> changes requested and create a patch, or is there something else that needs
> to be done?

Yep, you can just make the changes locally, then generate a patch. This page is a pretty good reference for how we use mercurial:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ

You should definitely make sure you have a local build set up, so that you can test your changes to verify that the find bar still works correctly.

Unfortunately, I don't believe we have any automated tests that cover the find bar right now. I would love to see some get written, but it may be really hard with our current test framework, since this is a feature that depends on an interaction between the JS and Java sides of our app, and our test frameworks are designed to test either JS or Java, but not both at the same time.

> Also, is it only this file that needs the changes?  And
> finally, what is the purpose for the change from `fastFind` to `finder`?

I haven't been following the desktop changes myself. Tom, can you provide some more context around this bug?

Comment 3

[Tom S. (please needinfo tschuster)]

Okay, so what happened is we reimplemented the findbar based on the Finder.jsm, which provides browser.finder. The main change is that it provides an asynchronous API, which was important for multiprocess desktop firefox. So in doFind in if (!fastFind) or now if (!finder) you need to call addResultListener. find and findAgain don't return anything instead the onFindResult is called when something is found/not found. Just renaming handleResult to onFindResult could already be enough.

Comment 4

[jones3.hardy]

(In reply to :Margaret Leibovic from comment #2)
> Yep, you can just make the changes locally, then generate a patch. This page
> is a pretty good reference for how we use mercurial:
> https://developer.mozilla.org/en-US/docs/Mercurial_FAQ

Cool, thanks.

> You should definitely make sure you have a local build set up, so that you
> can test your changes to verify that the find bar still works correctly.

I made the changes, pulled, and went to do a partial rebuild `./mach build mobile/android`, but I'm running into some error now

> /home/joneshf/programming/mozilla/src/mobile/android/base/home/TopSitesGridItemView.java:186: error: cannot find symbol
>             mTitleView.setText(R.string.home_top_sites_add);
>                                       ^
>   symbol:   variable home_top_sites_add
>   location: class string

Going to try a full rebuild and see if that changes anything.  Maybe something needed to be updated.

(In reply to Tom Schuster [:evilpie] from comment #3)
> Okay, so what happened is we reimplemented the findbar based on the
> Finder.jsm, which provides browser.finder. The main change is that it
> provides an asynchronous API, which was important for multiprocess desktop
> firefox. So in doFind in if (!fastFind) or now if (!finder) you need to call
> addResultListener. find and findAgain don't return anything instead the
> onFindResult is called when something is found/not found. Just renaming
> handleResult to onFindResult could already be enough.

I see, thanks for the explanation.  So just to be clear, somewhere in this block: http://hg.mozilla.org/mozilla-central/file/28e5d67b6b5a/mobile/android/chrome/content/FindHelper.js#l35 we need to call `addResultListener`?  On what are we calling that, `this._targetTab.browser` or something else?

I assume this is the Finder.jsm that you mention: http://hg.mozilla.org/mozilla-central/file/28e5d67b6b5a/toolkit/modules/Finder.jsm  It seems to still use `fastFind`.  Is that going to be an issue, or not really a concern here?

Comment 5

[jones3.hardy]

Attachment: 916483.patch

Got it to build, and assuming this was really the only thing that needed to be changed.

Comment 6

[:Margaret Leibovic]

Comment on attachment 808990 [details] [diff] [review]
916483.patch

I think what you meant to do was request review from me :) I'll take a closer look at this tomorrow!

Comment 7

[:Margaret Leibovic]

Comment on attachment 808990 [details] [diff] [review]
916483.patch

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

I was wrong! There is a test for find in page:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testFindInPage.java.in

So we'll need to make sure this still passes before landing a patch for this bug.

I'm going to clear review for now, since I don't think we were clear enough about what needs to happen for this bug, so you'll nee to update this patch to address this.

::: mobile/android/chrome/content/FindHelper.js
@@ +35,2 @@
>        this._targetTab = BrowserApp.selectedTab;
> +      this._finder = this._targetTab.browser.fastFind;

The point of this bug is to replace our use of browser.fastFind with browser.finder, so you'll need to change that here, then make the appropriate changes to use this new async API (as Tom described earlier).

You can look at the desktop implementation to see an example of this API in action:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml

Comment 8

[jones3.hardy]

Thanks for the update.

Okay, so let's see if I've got this straight now.

We need http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/FindHelper.js#36 to be `browser.finder`.

Then we call `this._fastFind.addResultListener(this)`.

Next we can do away with http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/FindHelper.js#42 and http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/FindHelper.js#53 along with saving the `result`.

Finally change the name `handleResult` to `onFindResult`

Is that all correct?

Here are some things I'm not clear about.

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Finder.jsm#71 still uses `fastFind`, is Finder.jsm not a file that I should be worrying about?  Or does the finder come from some other file? 

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Finder.jsm#59 is passing three values to `onFindResult`.  Should I care about the other two values in FindHelper.js, or do they not mean anything in this context?

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#873 is still using `fastFind`.  Is this how I replace line 41 in FindHelper.js?  Something like `this._fastFind.fastFind(aSearchString, false);`

Sorry for all the questions.  It's just a bit confusing where everything is coming from.

Comment 9

[:Margaret Leibovic]

(In reply to jones3.hardy from comment #8)

I'm sorry if I haven't given very clear advice before, this Finder.jsm API is all new to me, too! :)

> We need
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> FindHelper.js#36 to be `browser.finder`.

Yes.

> Then we call `this._fastFind.addResultListener(this)`.

That sounds correct. I'm not totally familiar with this myself, so I've just been looking through the changes Tom made for desktop, and that looks correct:
http://hg.mozilla.org/mozilla-central/diff/295578d99074/toolkit/content/widgets/findbar.xml#l1.224

> Next we can do away with
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> FindHelper.js#42 and
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> FindHelper.js#53 along with saving the `result`.
> 
> Finally change the name `handleResult` to `onFindResult`
> 
> Is that all correct?

Yes, this sounds correct.

> Here are some things I'm not clear about.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Finder.jsm#71
> still uses `fastFind`, is Finder.jsm not a file that I should be worrying
> about?  Or does the finder come from some other file? 

So, the finder we're using will come from browser.xml, which creates an instance of Finder.jsm that we'll be using:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#301

This bug is about replacing the use of browser.fastFind with browser.finder, but you're right, it does look like we'll need to use this new browser.finder.fastFind() method, similarly to how desktop does:
http://hg.mozilla.org/mozilla-central/diff/295578d99074/toolkit/content/widgets/findbar.xml#l1.1329

> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Finder.jsm#59
> is passing three values to `onFindResult`.  Should I care about the other
> two values in FindHelper.js, or do they not mean anything in this context?

I don't think we need to worry about those for our purposes, although maybe we could use them to enhance our find bar experience in the future.

> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> findbar.xml#873 is still using `fastFind`.  Is this how I replace line 41 in
> FindHelper.js?  Something like `this._fastFind.fastFind(aSearchString,
> false);`

I mentioned this up above, but yes, we'll want to use this fastFind method like desktop is using, but it will come from a different place. So this line in FindHelper.js will look like `this._finder.fastFind(...)`, assuming you set this._finder to browser.finder.

> Sorry for all the questions.  It's just a bit confusing where everything is
> coming from.

Totally understandable, it actually took me a bit of time to try to understand this myself. The naming here is tricky!

Comment 10

[Tom S. (please needinfo tschuster)]

>Totally understandable, it actually took me a bit of time to try to understand this myself. The naming here is tricky!
This is a good point. Soon Finder.jsm will get some appropriate documentation. Would it help if we renamed fastFind to just find?

Comment 11

[:Margaret Leibovic]

(In reply to Tom Schuster [:evilpie] from comment #10)
> >Totally understandable, it actually took me a bit of time to try to understand this myself. The naming here is tricky!
> This is a good point. Soon Finder.jsm will get some appropriate
> documentation. Would it help if we renamed fastFind to just find?

That sounds like a good idea.

Comment 12

[jones3.hardy]

Cool, I'll look at this more in depth later on tonight.  Thanks for all the help.

Comment 13

[jones3.hardy]

(In reply to :Margaret Leibovic from comment #11)
> (In reply to Tom Schuster [:evilpie] from comment #10)
> > >Totally understandable, it actually took me a bit of time to try to understand this myself. The naming here is tricky!
> > This is a good point. Soon Finder.jsm will get some appropriate
> > documentation. Would it help if we renamed fastFind to just find?
> 
> That sounds like a good idea.

So, should we wait until that happens to finish this one?

Comment 14

[Tom S. (please needinfo tschuster)]

No, we will take care of it, this is not really something that blocks this work. You can just use fastFind for now.

Comment 15

[Tom S. (please needinfo tschuster)]

Are you still working on this?

Comment 16

[:Margaret Leibovic]

To second Tom's question, are you still working on this? Let us know if you need any help!

Comment 17

[jones3.hardy]

Apologies Margaret and Tom.

I got really busy with school and stuff, I haven't had a chance to set up my environment again, and I'm not sure if I'll have time to get set up again in the near future. Probably best to hand this off to someone else in the meantime.

Comment 18

[:Margaret Leibovic]

(In reply to jones3.hardy from comment #17)
> Apologies Margaret and Tom.
> 
> I got really busy with school and stuff, I haven't had a chance to set up my
> environment again, and I'm not sure if I'll have time to get set up again in
> the near future. Probably best to hand this off to someone else in the
> meantime.

Thanks for letting us know! I'll unassign you from this bug, but feel free to pick it up again if you ever have time. And if someone else fixes it, there are always lots of other bugs to choose from! :)

Comment 19

[:Margaret Leibovic]

I talked to SirSkidmore on IRC, and he's going to work on this.

Comment 20

[Taylor Skidmore [:SirSkidmore]]

Attachment: Replaces fastFind with finder. r=margaret

Comment 21

[Tom S. (please needinfo tschuster)]

Comment on attachment 8336390 [details] [diff] [review]
Replaces fastFind with finder. r=margaret

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

I am just going to leave some comments. I hope you don't mind. Thanks for picking this up!

::: mobile/android/chrome/content/FindHelper.js
@@ +35,2 @@
>        this._targetTab = BrowserApp.selectedTab;
> +      this._finder = this._targetTab.browser.finder;

You should set up the result listener during the initialization here. After the fastFind call is to late, but you also only want to do it once.

@@ +48,5 @@
>        this.doFind(aString);
>        return;
>      }
>  
> +    let result = this._finder.findAgain(aFindBackwards, false);

Find again takes a third parameter, that you should set to false.

@@ +59,3 @@
>        return;
>  
> +    this._finder.collapseSelection();

The closest thing to collapseSelection Finder.jsm is removeSelection, which you probably want here.

@@ +64,5 @@
>      this._initialViewport = null;
>      this._viewportChanged = false;
>    },
>  
> +  onFindResult: function(aResult) {

It think it's cleaner to define the rest of the parameters even when they are not used. "aFindBackwards" and "aLinkURL".

Comment 22

[Taylor Skidmore [:SirSkidmore]]

Attachment: Update patch for replacing fastFind with finder. r=margaret

Comment 23

[:Margaret Leibovic]

Comment on attachment 8336409 [details] [diff] [review]
Update patch for replacing fastFind with finder. r=margaret

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

Looks good to me! I just have two small suggestions. We can also let Tom take a look to double-check before landing this.

I also ran testFindInPage locally, and verified it still passes with this patch applied.

::: mobile/android/chrome/content/FindHelper.js
@@ +38,5 @@
>        this._initialViewport = JSON.stringify(this._targetTab.getViewport());
>        this._viewportChanged = false;
>      }
>  
> +    let result = this._finder.fastFind(aSearchString, false);

You don't need to declare this variable anymore (just make the .fastFind call on its own).

@@ +48,5 @@
>        this.doFind(aString);
>        return;
>      }
>  
> +    let result = this._finder.findAgain(aFindBackwards, false, false);

Same here.

Comment 24

[Tom S. (please needinfo tschuster)]

Comment on attachment 8336409 [details] [diff] [review]
Update patch for replacing fastFind with finder. r=margaret

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

I guess there is only one thing, I am a bit concerned about, because I don't really know the code. The init code should only called once per browser, but it absolutely needs to be called when the browser changes. If the init code runs multiple times we attach the result listener more then once as well, which can be annoying in some cases. The old code already used a reference to the browser, so the later part is probably no issue. Otherwise it looks good to me.

Comment 25

[Taylor Skidmore [:SirSkidmore]]

Attachment: Update: remove binding results to variable.

Comment 26

[:Margaret Leibovic]

(In reply to Tom Schuster [:evilpie] from comment #24)
> Comment on attachment 8336409 [details] [diff] [review]
> Update patch for replacing fastFind with finder. r=margaret
> 
> Review of attachment 8336409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess there is only one thing, I am a bit concerned about, because I don't
> really know the code. The init code should only called once per browser, but
> it absolutely needs to be called when the browser changes. If the init code
> runs multiple times we attach the result listener more then once as well,
> which can be annoying in some cases. The old code already used a reference
> to the browser, so the later part is probably no issue. Otherwise it looks
> good to me.

Good catch! This doFind() method is called every time the find bar is opened, which can definitely happen multiple times to a single browser. To fix this, can we just call `this._finder.removeResultListener(this)` before setting this._finder to null?

Comment 27

[Taylor Skidmore [:SirSkidmore]]

Attachment: Update: call removeResultListener, as per Margaret's suggestion. r=margaret

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8597295fd7d7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8597295fd7d7