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

VERIFIED FIXED in Firefox 26

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: kbrosnan, Assigned: lucasr)

Tracking

({reproducible})

Trunk
Firefox 27
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 verified, fennec26+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
When you type one letter into the awesomescreen nothing is found for bookmarks or history.
Keywords: reproducible
This sounds similar to bug 917962, I'll take a look.
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 26+
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #813230 - Flags: review?(lucasr.at.mozilla)
Assignee

Comment 3

6 years ago
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.
Attachment #813230 - Flags: review?(lucasr.at.mozilla) → review-
Lucas told me he has a better patch for this bug, so it's all his! :)
Assignee: margaret.leibovic → lucasr.at.mozilla
Assignee

Comment 5

6 years ago
Attachment #813230 - Attachment is obsolete: true
Attachment #815436 - Flags: review?(margaret.leibovic)
Assignee

Comment 6

6 years ago
Attachment #815437 - Flags: review?(margaret.leibovic)
Assignee

Comment 7

6 years ago
Attachment #815438 - Flags: review?(margaret.leibovic)

Updated

6 years ago
Attachment #815436 - Flags: review?(margaret.leibovic) → review+
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 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.
Assignee

Updated

6 years ago
Duplicate of this bug: 917962
Assignee

Comment 11

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

Comment 12

6 years ago
(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
(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 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.
Attachment #815437 - Flags: review?(margaret.leibovic) → review+

Updated

6 years ago
Attachment #815438 - Flags: review?(margaret.leibovic) → review+
Assignee

Comment 16

6 years ago
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
Attachment #815436 - Flags: approval-mozilla-aurora?
Assignee

Comment 17

6 years ago
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
Attachment #815437 - Flags: approval-mozilla-aurora?
Assignee

Comment 18

6 years ago
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
Attachment #815438 - Flags: approval-mozilla-aurora?
Assignee

Comment 19

6 years ago
(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.
Status: RESOLVED → VERIFIED
Attachment #815436 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #815437 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #815438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.