Closed Bug 1523602 Opened 6 years ago Closed 6 years ago

Re-use existing rows when receiving results

Categories

(Firefox :: Address Bar, enhancement, P2)

enhancement
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: perf)

Attachments

(1 file)

Currently rows are always created from scratch when receiving results. We should try to re-use existing rows.

Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED

I'd like to add one detail here, that may not be obvious from how we outlined this bug. Tests must be able to tell if a row is "current" (refers to the current query) or not.
In the old autocomplete, we reused richlistitems, and we didn't hide them until new results arrive (or when the query was done, or on a timer), but tests needed to differentiate between current or old rows. To do that each row had an an "ac-text" attribute, that we used to compare with the current search string, if they are the same the result refers to the running query.
We may want to somehow connect rows to specific queryContexts.

(In reply to Marco Bonardo [::mak] from comment #1)

I'd like to add one detail here, that may not be obvious from how we outlined this bug. Tests must be able to tell if a row is "current" (refers to the current query) or not.
In the old autocomplete, we reused richlistitems, and we didn't hide them until new results arrive (or when the query was done, or on a timer), but tests needed to differentiate between current or old rows. To do that each row had an an "ac-text" attribute, that we used to compare with the current search string, if they are the same the result refers to the running query.
We may want to somehow connect rows to specific queryContexts.

I can see the QuantumBar working the same way. Wouldn't it make sense - since we now have a QueryContext, not just a query text, to use an sequence ID that resets to 1 when it's >= MAX_INT?
This way it'll become a more logical operation, comparing numbers, here. WDYT?

Blocks: 1530338

Another detail that is not documented about the legacy urlbar: we distinguish results coming from the network from local results, because they have different characteristics.

For example, we put search suggestions before historical results, this means that every time the user types a new char, if remote results are slower to arrive, we'd end up adding historical results and then they would be pushed down by search suggestions, and this causes continuous flicker while typing.
What the old urlbar does is replacing same-type results, and once a search is done it removes the "stale" results that don't apply anymore. This prevents flicker of results, because before removing search suggestions the view waits for new ones to arrive (and often they may be the same already there).

This is implemented in UnifiedComplete in the legacy implementation, but in the Quantum Bar we can't reuse that, because the new API inserts new results one by one and lets the sorting to the muxer. Though, the view can likely do something like this using the result type/source.

This flicker prevention is probably more important than any resource saved by reusing elements.

Blocks: 1535656
Blocks: 1524510

(In reply to Marco Bonardo [::mak] from comment #3)

Another detail that is not documented about the legacy urlbar: we distinguish results coming from the network from local results, because they have different characteristics.

For example, we put search suggestions before historical results, this means that every time the user types a new char, if remote results are slower to arrive, we'd end up adding historical results and then they would be pushed down by search suggestions, and this causes continuous flicker while typing.
What the old urlbar does is replacing same-type results, and once a search is done it removes the "stale" results that don't apply anymore. This prevents flicker of results, because before removing search suggestions the view waits for new ones to arrive (and often they may be the same already there).

This is implemented in UnifiedComplete in the legacy implementation, but in the Quantum Bar we can't reuse that, because the new API inserts new results one by one and lets the sorting to the muxer. Though, the view can likely do something like this using the result type/source.

This flicker prevention is probably more important than any resource saved by reusing elements.

My current plan is to make that a followup to this bug in bug 1535656.

Points: --- → 3
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6385899fe7b9 Re-use existing rows when receiving results. r=mak
No longer blocks: 1530338
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f4934ff6383 Re-use existing rows when receiving results. r=mak
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Depends on: 1540104
No longer depends on: 1540104
Regressions: 1540104
Regressions: 1554038
Regressions: 1559179
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: