1104985 - Bookmark keyword searches should rely solely on the cache, rather than needing an extra DB query

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As of Nightly 2014-11-25, keyword searches seem to drop the last few characters if I press enter very fast.  I am not using e10s at the moment.

In #fx-team, Unfocused suspected this is related to bug 1067903.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, fallout from bug 1067903.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We detect if a keyword search matches via a synchronous match. But we don't actually add a match until we do an async DB query. We get the current sync match via a cache, and do the DB query to get the title - so we should just be able to also cache the title, and make it all sync.

Longer term, we may wish to implement waiting for the first special result if it hasn't returned yet before the enter key has been pressed (not to block the enter key, but to wait on navigating). But that's tricky, and would rely on the controller rewrite. Thankfully, I don't think we need it yet.

Comment 3

[Marco Bonardo [:mak]]

is this about search aliases or bookmark keywords?
since currently bookmarks keywords are no more a synchronous look up (there is at least one tick, the first query in the session will be slower to build the cache though)

fwiw, there's no way we can do a DB query synchronously, we just shouldn't.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #3)
> is this about search aliases or bookmark keywords?
> since currently bookmarks keywords are no more a synchronous look up (there
> is at least one tick, the first query in the session will be slower to build
> the cache though)
> 
> fwiw, there's no way we can do a DB query synchronously, we just shouldn't.

For me, it's about a bookmark with a keyword, whose URL also uses the "%s" parameter to let it also accept a query.  So, I guess that means "bookmark keyword" (even though I use it for searching...).  Hopefully I'm being specific enough. :)

Comment 5

[Jesse Ruderman]

[Tracking Requested - why for this release]:

It's not just keyword searches that are broken. Typed URLs such as "about:crashes" also have their last few characters dropped.

Comment 6

[Jesse Ruderman]

Apparently Bugzilla doesn't want to submit what I type either. Filed bug 1105155.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Jesse Ruderman from comment #5)
> It's not just keyword searches that are broken. Typed URLs such as
> "about:crashes" also have their last few characters dropped.

yep, confirmed, and this is much worse than keywords.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

The uplift is Friday. Can we please backout bug 1067903?

Comment 9

[Marco Bonardo [:mak]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> The uplift is Friday. Can we please backout bug 1067903?

we will instead disable unifiedcomplete in Nightly until these bad recent regressions are handled, so we can continue working on top of the current code and not risk bitrotting and a dependencies nightmare. bug 1105456.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Not tracking - this is specific to UnifiedComplete, which was disabled via bug 1105456.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #3)
> fwiw, there's no way we can do a DB query synchronously, we just shouldn't.

No, we don't need (or want!) a synchronous query here. We currently use the cache to only detect bookmark keywords, but the DB to fetch info about them - because the cache neither has all the info we want (it lacks the title), or the API. So we modify the cache so we can just use that, instead of a DB query each time.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and I'm treating bug 1105967 as separate. That's a generic issue with the controller/popup, whereas this bug has a specific fix in UnifiedComplete.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Blair McBride [:Unfocused] from comment #13)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #3)
> > fwiw, there's no way we can do a DB query synchronously, we just shouldn't.
> 
> No, we don't need (or want!) a synchronous query here. We currently use the
> cache to only detect bookmark keywords, but the DB to fetch info about them
> - because the cache neither has all the info we want (it lacks the title),
> or the API. So we modify the cache so we can just use that, instead of a DB
> query each time.

I'd like to understand which code you intend to touch, cause I'm rewriting the keywords cache in bug 1083469 and at this point maybe we want a different kind of rewrite? (fwiw that bug is blocked by a fancy BC1 orange on e10s)

The code that is in the tree currently is not what we will use, cause it's currently synchronous.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #15)
> The code that is in the tree currently is not what we will use, cause it's
> currently synchronous.

Heh, I was going to ask you about that, after I looked at that code again. I think your patch in bug 1083469 is what I was thinking of :)

So, looking at that patch, it introduces more asynchronicity to the first result. So my idea here won't really help in the way intended. But it will help in that we can reduce a DB query (_keywordQuery).

Given that, how about we de-prioritise this bug until sometime after bug 1083469 lands. In the mean time, I'll concentrate on bug 1105967.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Blair McBride [:Unfocused] from comment #16)
> Given that, how about we de-prioritise this bug until sometime after bug
> 1083469 lands. In the mean time, I'll concentrate on bug 1105967.

I think it's a good idea.
As well as we can avoid keywordQuery by making the cache smarter, so, I could make bug 1083469 different. My issue is mostly the orange atm, but once that is somehow figured, I can make the code smarter.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Making this clearer, as summarized in comment 17.

Comment 19

[Marco Bonardo [:mak]]

Currently we don't query anymore, we use PlacesUtils.keywords.fetch now, that doesn't have to go through the db.

Comment 20

[Marco Bonardo [:mak]]

I'm marking this as WFM based on comment 19, the keywords API refactoring has basically already solved this.