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

RESOLVED WORKSFORME

Status

()

Toolkit
Places
P3
normal
Rank:
35
RESOLVED WORKSFORME
3 years ago
2 years ago

People

(Reporter: jryans, Unassigned)

Tracking

({dogfood, regression})

unspecified
dogfood, regression
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite ?
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [unifiedautocomplete][fxsearch])

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.
Yea, fallout from bug 1067903.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Points: --- → 5
Flags: firefox-backlog+

Updated

3 years ago
Iteration: --- → 37.1
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
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.
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.
(Reporter)

Comment 4

3 years ago
(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

3 years ago
[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.
tracking-firefox36: --- → ?
Keywords: dogfood, regression

Comment 6

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

Updated

3 years ago
Blocks: 995091
(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.
Summary: Keyword search drops last few characters → Quick Enter just after typing keyword search or url drops last few characters
The uplift is Friday. Can we please backout bug 1067903?
Flags: in-testsuite?
Blocks: 1067903

Updated

3 years ago
Depends on: 1105456
(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.

Updated

3 years ago
Duplicate of this bug: 1105967

Updated

3 years ago
Duplicate of this bug: 1105911
Not tracking - this is specific to UnifiedComplete, which was disabled via bug 1105456.
tracking-firefox36: ? → ---
(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.
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.
(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.
(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.
(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.
Assignee: bmcbride → nobody
Status: ASSIGNED → NEW

Updated

3 years ago
Iteration: 37.1 → ---
Making this clearer, as summarized in comment 17.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Quick Enter just after typing keyword search or url drops last few characters → Bookmark keyword searches should rely solely on the cache, rather than needing an extra DB query

Updated

2 years ago
Depends on: 1125115
Currently we don't query anymore, we use PlacesUtils.keywords.fetch now, that doesn't have to go through the db.
Whiteboard: [fxsearch][unifiedautocomplete]
Priority: -- → P3

Updated

2 years ago
Whiteboard: [fxsearch][unifiedautocomplete] → [unifiedautocomplete][fxsearch]

Updated

2 years ago
Rank: 35
I'm marking this as WFM based on comment 19, the keywords API refactoring has basically already solved this.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.