Closed Bug 1677408 Opened 1 year ago Closed 1 year ago

Reduce history notifications traffic for bookmark views

Categories

(Toolkit :: Places, task, P2)

task
Points:
5

Tracking

()

RESOLVED FIXED
85 Branch
Iteration:
85.1 - Nov 16 - Nov 29
Tracking Status
firefox85 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

With some refactoring, we should be able to:

  1. remove OnPageChanged
  2. remove onItemVisited
  3. stop observing history from the bookmarks service
  4. Allow bookmark views that don't use nor show history details to not listen for new visits and not notify history details changes to the view

This should allow us to remove one history listener, and reduce the weight of history on bookmark views.

Use the new notifications system to notify about favicon changes, that is the only
part left of onPageChanged.

Rather than fetching bookmarks from the disk for each page-visited notification,
we just let the result to recursively find and update children in the memory
structure.

Depends on D97276

RunInBatchMode has been removed some time ago, the only thing still sending
these notifications is maintenance to force a UI update, and it can send both.

Depends on D97277

The result can directly listen for page-visited and recursively update children.
With onItemVisited gone, all the reasons for the bookmarks service to listen for
history notifications are gone, thus it can stop observing history.

Depends on D97278

In most cases bookmarks views don't need to be notified of history changes.
The only cases where they need that, is when history columns are visible in a
tree view, or the result is sorted by visits or frecency.
Thus, the menu and toolbar views can often save some work by not listening.

Depends on D97279

Blocks: 1473530
Duplicate of this bug: 1511344
Duplicate of this bug: 1511346
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/f7d6cc5e4457
Part 1 - Replace onPageChanged with a new favicon-changed notification. r=Standard8,emilio
https://hg.mozilla.org/integration/autoland/rev/b0c23854e117
Part 2 - Update consumers of onPageChanged. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/996cbd111957
Part 3 - Clean up bookmarks observer tracking in nsNavHistoryResult. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/0f2d72d2ed5a
Part 4 - Replace onItemVisited with page-visited. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/ff93f264138b
Part 5 - Remove no more used onBegin/EndUpdateBatch notifications forwarding. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/4198bc35198c
Part 6 - Remove onItemVisited and the history observer in the bookmarks service. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/1e0dc9aaa979
Part 7 - Allow the view to opt-out of history details updates. r=Standard8

Thank you, I'm already verifying fixes on Try.

Flags: needinfo?(mak)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/49f8c976469e
Part 1 - Replace onPageChanged with a new favicon-changed notification. r=Standard8,emilio
https://hg.mozilla.org/integration/autoland/rev/d739ef17da5f
Part 2 - Update consumers of onPageChanged. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/8684e7486a10
Part 3 - Clean up bookmarks observer tracking in nsNavHistoryResult. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/f7c9fb064c37
Part 4 - Replace onItemVisited with page-visited. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/51d608074a74
Part 5 - Remove no more used onBegin/EndUpdateBatch notifications forwarding. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/bcaf47876119
Part 6 - Remove onItemVisited and the history observer in the bookmarks service. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/9785de91c4fc
Part 7 - Allow the view to opt-out of history details updates. r=Standard8
You need to log in before you can comment on or make changes to this bug.