Status

defect
P1
normal
RESOLVED FIXED
Last year
6 months ago

People

(Reporter: boek, Assigned: jchen)

Tracking

unspecified
mozilla63

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

Details

(Whiteboard: [geckoview:fenix][geckoview:klar:p1][focus:feature])

Attachments

(2 attachments, 1 obsolete attachment)

Whiteboard: [geckoview:fenix][geckoview:klar]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → nchen
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255562

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1117
(Diff revision 1)
> +        /** Ordinal number of the current match starting from 1, or 0 if no match. */
> +        public final int current;
> +        /** Total number of matches found so far, or -1 if unknown. */
> +        public final int total;
> +        /** Search string. */
> +        @NonNull public final String searchString;

searchString seems redundant, since it's obviously a string -- "needle" and "haystack" are somewhat common terms. Or maybe "searchQuery".

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1178
(Diff revision 1)
> +     *               not needed. The {@link FindInPageResult} response may be null if an error
> +     *               occurred during the search. May be called multiple times during a search as
> +     *               more information about the search becomes available (e.g. the total number of
> +     *               matches may be updated as matches are counted).
> +     */
> +    public void findInPage(@Nullable final String searchString, @FindInPageFlags final int flags,

Would it be better if this returned an iterator type of object instead of requiring multiple calls to findInPage()? I think that's nicer from the app perspective, but probably more of a pain for us to implement.
Assignee

Comment 5

Last year
mozreview-review-reply
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255562

> searchString seems redundant, since it's obviously a string -- "needle" and "haystack" are somewhat common terms. Or maybe "searchQuery".

"searchString" seems to be the term Gecko prefers [1]. Also I think "string" makes it more clear it's a plain string versus a regex or something.

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/toolkit/modules/Finder.jsm#125

> Would it be better if this returned an iterator type of object instead of requiring multiple calls to findInPage()? I think that's nicer from the app perspective, but probably more of a pain for us to implement.

I'm not sure an iterator would be nicer, since the app would then have to save the iterator object somewhere. We also don't support multiple concurrent searches, which individual iterator objects would imply.
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

Adding snorp as second r? for API change. Also f? ekager to go over the API.
Attachment #8983538 - Flags: review?(snorp)
Attachment #8983538 - Flags: feedback?(ekager)
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255562

> "searchString" seems to be the term Gecko prefers [1]. Also I think "string" makes it more clear it's a plain string versus a regex or something.
> 
> [1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/toolkit/modules/Finder.jsm#125

We don't have any obligation to carry over Gecko-isms into GeckoView. Also, JS isn't strongly typed, so having the type as part of the name may make more sense there. In GeckoView, you know the type is String (or Regex, etc), so I wouldn't ever expect to see "fooString" as a public member name.

> I'm not sure an iterator would be nicer, since the app would then have to save the iterator object somewhere. We also don't support multiple concurrent searches, which individual iterator objects would imply.

The app is going to have some kind of View with the next/prev buttons, etc. They could just pass the iterator to that.

For the second issue we could just have findInPage() throw if there's an open find-in-page session. It does open the possibility of a dangling find-in-page session becoming a problem, though.
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255582

I think I would prefer a different API, but this also looks fairly close to what WebView does, so *shrug*

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1079
(Diff revision 1)
>      public void goForward() {
>          mEventDispatcher.dispatch("GeckoView:GoForward", null);
>      }
>  
> +    /** Go backwards when finding the next match. */
> +    public static final int FIND_IN_PAGE_BACKWARDS = 1;

We have some other API that puts the flags into the delegate class, which means you don't need the FIND_IN_PAGE prefix. Consider putting the flags into FindInPageResult for this reason.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1178
(Diff revision 1)
> +     *               not needed. The {@link FindInPageResult} response may be null if an error
> +     *               occurred during the search. May be called multiple times during a search as
> +     *               more information about the search becomes available (e.g. the total number of
> +     *               matches may be updated as matches are counted).
> +     */
> +    public void findInPage(@Nullable final String searchString, @FindInPageFlags final int flags,

It may be preferable to have findInPageAgain() which has no arguments, rather than requiring a null query. If you have findInPageForward() and findInPageBack() you could eliminate those flags. The other flags would then only apply to the initial search. As it is, it's not clear to me what should happen if you repeat a search with changed flags.
Attachment #8983538 - Flags: review?(snorp) → review+
Assignee

Comment 9

Last year
mozreview-review-reply
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255582

> We have some other API that puts the flags into the delegate class, which means you don't need the FIND_IN_PAGE prefix. Consider putting the flags into FindInPageResult for this reason.

I was following the `loadUri` flags that are in `GeckoSession`. The other delegate flags are only used within the delegates, so it makes more sense for those to be inside the delegates, whereas in this case we use the flags outside of `FindInPageResult` as well.

> It may be preferable to have findInPageAgain() which has no arguments, rather than requiring a null query. If you have findInPageForward() and findInPageBack() you could eliminate those flags. The other flags would then only apply to the initial search. As it is, it's not clear to me what should happen if you repeat a search with changed flags.

Repeating a search with changed flags does just that, repeating the search but with new flags applied. Let's see what Emily says.
Assignee

Comment 10

Last year
mozreview-review-reply
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255562

> We don't have any obligation to carry over Gecko-isms into GeckoView. Also, JS isn't strongly typed, so having the type as part of the name may make more sense there. In GeckoView, you know the type is String (or Regex, etc), so I wouldn't ever expect to see "fooString" as a public member name.

A lot of Java APIs have regex parameters whose types are String, so the ambiguity definitely exists in Java as well. In any case, it's just a name, so let's see what Emily says.
NI for Sebastian to review the API here as well, since presumably we'll be using this from the mobile components.
Flags: needinfo?(s.kaspari)

Comment 12

Last year
mozreview-review
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review255836

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1093
(Diff revision 1)
> +
> +    @Retention(RetentionPolicy.SOURCE)
> +    @IntDef(flag = true,
> +            value = { FIND_IN_PAGE_BACKWARDS, FIND_IN_PAGE_LINKS_ONLY, FIND_IN_PAGE_DRAW_OUTLINE,
> +                      FIND_IN_PAGE_MATCH_CASE, FIND_IN_PAGE_WHOLE_WORD })
> +    public @interface FindInPageFlags {}

Please put that before the constant declarations for consistency.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1122
(Diff revision 1)
> +        @NonNull public final String searchString;
> +        /** Flags used for the search; either 0 or a combination of {@link #FIND_IN_PAGE_BACKWARDS
> +         * FIND_IN_PAGE_*} flags. */
> +        @FindInPageFlags public final int flags;
> +        /** URL of the link, if the current match is a link, or null otherwise. */
> +        @Nullable public final String linkUrl;

I think we've decided to go with URI.
Attachment #8983538 - Flags: review?(esawin) → review+

Comment 13

Last year
mozreview-review
Comment on attachment 8983540 [details]
Bug 1463484 - 2. Add test for find-in-page;

https://reviewboard.mozilla.org/r/249392/#review255842
Attachment #8983540 - Flags: review?(esawin) → review+
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

Looks good!
Attachment #8983538 - Flags: feedback?(ekager) → feedback+
API:

> public void findInPage(
>    @Nullable final String searchString,
>    @FindInPageFlags final int flags,
>    @Nullable final GeckoResponse<FindInPageResult> result
> )

Questions:

* Does GeckoView highlight all matches? From the API it looks like it selects the found word. So I assume it does not? (WebView does highlighting and therefore has additional methods like clearMatches())

* If I want to find the next word I can just call this again, I assume? Does it remember the state internally or does it always start from what is visible or the selected word? (WebView has findNext())

> {@code searchString} may be null on subsequent calls of {@code findInPage} to perform the same search again

That's pretty weird :D
Flags: needinfo?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> API:
> 
> > public void findInPage(
> >    @Nullable final String searchString,
> >    @FindInPageFlags final int flags,
> >    @Nullable final GeckoResponse<FindInPageResult> result
> > )
> 
> Questions:
> 
> * Does GeckoView highlight all matches? From the API it looks like it
> selects the found word. So I assume it does not? (WebView does highlighting
> and therefore has additional methods like clearMatches())

Not currently, but we can. Is highlighting required?

> * If I want to find the next word I can just call this again, I assume? Does
> it remember the state internally or does it always start from what is
> visible or the selected word? (WebView has findNext())

Yeah, calling it again is equivalent to `findNext()`. We were wondering if it's preferable to have one `findInPage` method or separate `findInPage` and `findNextInPage` methods. What do you think?

> > {@code searchString} may be null on subsequent calls of {@code findInPage} to perform the same search again
> 
> That's pretty weird :D

Ok so separate `findNextInPage` method? Originally I wanted the`findInPage` method to be the single entry point for find-in-page functionality, but maybe that's not desirable?
Flags: needinfo?(s.kaspari)
> Not currently, but we can. Is highlighting required?

Good question. That's at least how we do it in the WebView build. I'll flag :bram to answer that from UX's point of view. Is this something we would/could expose as a flag to be used with findInPage()?

> Ok so separate `findNextInPage` method? Originally I wanted the`findInPage` method to be the single entry point for find-in-page functionality, but maybe that's not desirable?

I think a single findInPage method is fine in general. How does GeckoView determine whether to continue or start from the beginning of the document? Does it remember the state internally or does it always continue from the current position / selected word?

Do I have to explicitly pass in null to continue or can I use the same search string as before?
Flags: needinfo?(s.kaspari) → needinfo?(bram)
(In reply to Sebastian Kaspari (:sebastian) from comment #17)
> > Not currently, but we can. Is highlighting required?
> 
> Good question. That's at least how we do it in the WebView build. I'll flag
> :bram to answer that from UX's point of view. Is this something we
> would/could expose as a flag to be used with findInPage()?

Yeah we can expose a highlight flag, in which case we also need a "clear matches" method.

> > Ok so separate `findNextInPage` method? Originally I wanted the`findInPage` method to be the single entry point for find-in-page functionality, but maybe that's not desirable?
> 
> I think a single findInPage method is fine in general. How does GeckoView
> determine whether to continue or start from the beginning of the document?
> Does it remember the state internally or does it always continue from the
> current position / selected word?

Yeah it always continues from current selection.

> Do I have to explicitly pass in null to continue or can I use the same
> search string as before?

Either null or same search string works.
(In reply to Jim Chen [:jchen] [:darchons] from comment #16)
> (In reply to Sebastian Kaspari (:sebastian) from comment #15)
> > Questions:
> > 
> > * Does GeckoView highlight all matches? […]
> 
> Not currently, but we can. Is highlighting required?

Yes. I would say that highlighting is a feature that we should ship with.
Flags: needinfo?(bram)
Whiteboard: [geckoview:fenix][geckoview:klar] → [geckoview:fenix][geckoview:klar][focus:feature]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8983539 - Attachment is obsolete: true

Comment 22

Last year
mozreview-review
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review262578


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/modules/geckoview/GeckoViewContent.jsm:296
(Diff revision 2)
> +
> +    let finder;
> +    try {
> +      finder = this.browser.finder;
> +    } catch (e) {
> +      if (aCallback) {

Error: 'aCallback' is not defined. [eslint: no-undef]

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:297
(Diff revision 2)
> +    let finder;
> +    try {
> +      finder = this.browser.finder;
> +    } catch (e) {
> +      if (aCallback) {
> +        aCallback.onError(`No finder: ${e}`);

Error: 'aCallback' is not defined. [eslint: no-undef]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Updated patches to use `GeckoResult`. Kept the "find again" behavior based on comment 17.

Comment 26

Last year
mozreview-review
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

https://reviewboard.mozilla.org/r/249388/#review262580

Looks good to me with some documentation fixes.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1109
(Diff revision 2)
> +    /** Limit matches to links on the page. */
> +    public static final int FINDER_FIND_LINKS_ONLY = 1 << 3;
> +
> +    @Retention(RetentionPolicy.SOURCE)
> +    @IntDef(flag = true,
> +            value = {FINDER_DISPLAY_HIGHTLIGHT_ALL, FINDER_DISPLAY_DIM_PAGE,

Should be FINDER_DISPLAY_HIGHLIGHT_ALL (there's an extra T in HIGHLIGHT). Likewise with other instances of this flag.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1143
(Diff revision 2)
> +        /** Total number of matches found so far, or -1 if unknown. */
> +        public final int total;
> +        /** Search string. */
> +        @NonNull public final String searchString;
> +        /** Flags used for the search; either 0 or a combination of {@link #FINDER_FIND_BACKWARDS
> +         * FIND_IN_PAGE_*} flags. */

Should be {@link #FINDER_FIND_BACKWARDS FINDER_FIND_*} I think?
Attachment #8983538 - Flags: review?(droeh) → review+
Comment on attachment 8983540 [details]
Bug 1463484 - 2. Add test for find-in-page;

https://reviewboard.mozilla.org/r/249392/#review262600

::: commit-message-5ec88:3
(Diff revision 3)
> +Bug 1463484 - 2. Add test for find-in-page; r?snorp
> +
> +Add some test cases for find-in-page and the different flags.

The finder selects the results as well, right? It would be nice to be able to verify the selection is correct when we get a result.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/FinderTest.kt:29
(Diff revision 3)
> +        mainSession.waitForPageStop()
> +
> +        // Initial search.
> +        var result = sessionRule.waitForResult(mainSession.finder.find("dolore", 0))
> +
> +        assertThat("Should be found", result.found, equalTo(true))

You may be able to factor out these assertions into a assertFindResult() helper so you don't have to repeat the strings so much.
Attachment #8983540 - Flags: review?(snorp) → review+
Assignee

Comment 28

Last year
mozreview-review-reply
Comment on attachment 8983540 [details]
Bug 1463484 - 2. Add test for find-in-page;

https://reviewboard.mozilla.org/r/249392/#review262600

> The finder selects the results as well, right? It would be nice to be able to verify the selection is correct when we get a result.

The `clear` test does verify the selection.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

Last year
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1ee6c3f12e6
1. Add SessionFinder; r=droeh,esawin,snorp
https://hg.mozilla.org/integration/autoland/rev/9cbe048f7395
2. Add test for find-in-page; r=esawin,snorp

Comment 32

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1ee6c3f12e6
https://hg.mozilla.org/mozilla-central/rev/9cbe048f7395
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Focus will require GeckoView to have the find-in-page feature
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Slightly.
[Why is the change risky/not risky?]: Only the new GeckoView find-in-page feature is risky. Nothing else should be affected.
[String changes made/needed]: None
Attachment #8983538 - Flags: approval-mozilla-beta?
Comment on attachment 8983538 [details]
Bug 1463484 - 1. Add SessionFinder

Should only affect geckoview, let's uplift for beta 8.
Attachment #8983538 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [geckoview:fenix][geckoview:klar][focus:feature] → [geckoview:fenix][geckoview:klar:p1][focus:feature]

Updated

6 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.