917350 - First letter typed into the awesomescreen does not find any bookmarks or history

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

When you type one letter into the awesomescreen nothing is found for bookmarks or history.

Comment 1

[:Margaret Leibovic]

This sounds similar to bug 917962, I'll take a look.

Comment 2

[:Margaret Leibovic]

Attachment: patch

The problem here is that the first BrowserSearch.filter call happens before the fragment's activity is created, so we don't end up doing the stuff inside the isVisible() if block.

I think this also fixes part of the problem from bug 917962.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 813230 [details] [diff] [review]
patch

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

Actually, this bug is caused by the fact that the search loader is initialized with empty search term *and* it's not actually loaded when the first input occurs.

Comment 4

[:Margaret Leibovic]

Lucas told me he has a better patch for this bug, so it's all his! :)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: 1: Factor out method to create SearchLoader arguments

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: 2: Move loader init() implementation to SearchLoader

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: 3: Enable loading unconditionally in BrowserSearch

Comment 8

[:Margaret Leibovic]

Comment on attachment 815437 [details] [diff] [review]
2: Move loader init() implementation to SearchLoader

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

::: mobile/android/base/home/BrowserSearch.java
@@ -312,5 @@
>      }
>  
>      @Override
>      protected void load() {
> -        getLoaderManager().initLoader(LOADER_ID_SEARCH, null, mCursorLoaderCallbacks);

This cursor loader logic is still a bit mysterious to me... now that we're not going to be initialing the loader with null args, will this line ever be called?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SearchLoader.java#37

I'm also a bit confused about what the performEmptySearch parameter is used for. Could you explain this a bit more?

Comment 9

[:Margaret Leibovic]

Comment on attachment 815438 [details] [diff] [review]
3: Enable loading unconditionally in BrowserSearch

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

::: mobile/android/base/home/BrowserSearch.java
@@ +142,5 @@
>      public static BrowserSearch newInstance() {
> +        BrowserSearch browserSearch = new BrowserSearch();
> +
> +        final Bundle args = new Bundle();
> +        args.putBoolean(HomePager.CAN_LOAD_ARG, true);

I'm also confused about what this CAN_LOAD_ARG is used for. Does that mean that we'll automatically load this cursor rather than waiting for some other action? Maybe it would be good to add a comment here explaining why we need this.

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #8)
> Comment on attachment 815437 [details] [diff] [review]
> 2: Move loader init() implementation to SearchLoader
> 
> Review of attachment 815437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BrowserSearch.java
> @@ -312,5 @@
> >      }
> >  
> >      @Override
> >      protected void load() {
> > -        getLoaderManager().initLoader(LOADER_ID_SEARCH, null, mCursorLoaderCallbacks);
> 
> This cursor loader logic is still a bit mysterious to me... now that we're
> not going to be initialing the loader with null args, will this line ever be
> called?
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> SearchLoader.java#37

Yep, PinSiteDialog creates SearchLoader with no arguments.

> I'm also a bit confused about what the performEmptySearch parameter is used
> for. Could you explain this a bit more?

IIRC, this was added by sriram in context of the PinSiteDialog. It's used to avoid triggering empty search in BrowserSearch. To be honest, this is not strictly necessary as we guard against that in BrowserApp, see filterEditingMode().

This is unrelated to this patch though. I can file a follow-up if you want.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 815438 [details] [diff] [review]
> 3: Enable loading unconditionally in BrowserSearch
> 
> Review of attachment 815438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +142,5 @@
> >      public static BrowserSearch newInstance() {
> > +        BrowserSearch browserSearch = new BrowserSearch();
> > +
> > +        final Bundle args = new Bundle();
> > +        args.putBoolean(HomePager.CAN_LOAD_ARG, true);
> 
> I'm also confused about what this CAN_LOAD_ARG is used for. Does that mean
> that we'll automatically load this cursor rather than waiting for some other
> action? Maybe it would be good to add a comment here explaining why we need
> this.

This is used by HomePager to lazy-load the content of its pages in two cases:

1. To wait for the editing mode animation to finish before loading the default page's content:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java#172

2. Only load a page's content once it gets selected. In which case, the ViewPager uses setUserVisibleHint on the selected fragment. See:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#193
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java#313

Comment 13

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #11)

> > I'm also a bit confused about what the performEmptySearch parameter is used
> > for. Could you explain this a bit more?
> 
> IIRC, this was added by sriram in context of the PinSiteDialog. It's used to
> avoid triggering empty search in BrowserSearch. To be honest, this is not
> strictly necessary as we guard against that in BrowserApp, see
> filterEditingMode().
> 
> This is unrelated to this patch though. I can file a follow-up if you want.

I think that would be a good idea. This code can be a bit difficult to follow without any context, so any simplification would be helpful.

Comment 14

[:Margaret Leibovic]

Comment on attachment 815437 [details] [diff] [review]
2: Move loader init() implementation to SearchLoader

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

Thanks for addressing my questions.

Comment 15

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/0fb839beb7e4
https://hg.mozilla.org/integration/fx-team/rev/b301518fc843
https://hg.mozilla.org/integration/fx-team/rev/759187c7a9a9

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 815436 [details] [diff] [review]
1: Factor out method to create SearchLoader arguments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: first letter or first pasted text on URL bar doesn't not trigger a frecency search.
Testing completed (on m-c, etc.): This should land in m-c soon. Let's wait for a couple of days and then uplift to Fx26. 
Risk to taking this patch (and alternatives if risky): Fairly low, just makes sure we actually perform a query when the browser search UI is initially shown. 
String or IDL/UUID changes made by this patch: n/a

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 815437 [details] [diff] [review]
2: Move loader init() implementation to SearchLoader

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: first letter or first pasted text on URL bar doesn't not trigger a frecency search.
Testing completed (on m-c, etc.): This should land in m-c soon. Let's wait for a couple of days and then uplift to Fx26. 
Risk to taking this patch (and alternatives if risky): Fairly low, just makes sure we actually perform a query when the browser search UI is initially shown. 
String or IDL/UUID changes made by this patch: n/a

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 815438 [details] [diff] [review]
3: Enable loading unconditionally in BrowserSearch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: first letter or first pasted text on URL bar doesn't not trigger a frecency search.
Testing completed (on m-c, etc.): This should land in m-c soon. Let's wait for a couple of days and then uplift to Fx26. 
Risk to taking this patch (and alternatives if risky): Fairly low, just makes sure we actually perform a query when the browser search UI is initially shown. 
String or IDL/UUID changes made by this patch: n/a

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #13)
> (In reply to Lucas Rocha (:lucasr) from comment #11)
> 
> > > I'm also a bit confused about what the performEmptySearch parameter is used
> > > for. Could you explain this a bit more?
> > 
> > IIRC, this was added by sriram in context of the PinSiteDialog. It's used to
> > avoid triggering empty search in BrowserSearch. To be honest, this is not
> > strictly necessary as we guard against that in BrowserApp, see
> > filterEditingMode().
> > 
> > This is unrelated to this patch though. I can file a follow-up if you want.
> 
> I think that would be a good idea. This code can be a bit difficult to
> follow without any context, so any simplification would be helpful.

Filed bug 927371.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fb839beb7e4
https://hg.mozilla.org/mozilla-central/rev/b301518fc843
https://hg.mozilla.org/mozilla-central/rev/759187c7a9a9

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e866af000e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0abcb6e1fff
https://hg.mozilla.org/releases/mozilla-aurora/rev/225a25c01d09