Fix stability and data racing issues, coalesce more updates for JumpList
This CL contains the following five main changes plus lots of updates
on variable/method names and comments.
1. Fix the JumpList data-racing issue by separating different thread
tasks.
There are two threads involved in updating the JumpList. The UI thread
lives the module to collect JumpList data for update, and a non-UI
thread lives the RunUpdateJumpList module which updates the JumpList
and notifies the shell. Previously, the two modules were interleaved
to some extent, which required the JumpList data to be shared between
the two threads. This caused the data racing issue and was solved by
introducing a lock. The interleaving of these two modules complicates
the code and makes it very difficult to add unit tests. This CL
separates these two modules. In more details, the JumpList data is now
only accessible to the UI thread. Local copies of the data are made
and sent to the non-UI thread to run the update task. Once that update
task finishes on the non-UI thread, the update results are transferred
to the UI thread via PostTaskAndReply. The UI thread then decides how
to update the JumpList data according to the update results.
2. Fix the JumpList stability issue by making the RunUpdateJumpList
API static so it outlives the JumpList instance.
When a JumpList::RunUpdateJumpList task is posted on a non-UI thread
and about to run, the JumpList instance may have already been
destructed. This caused stability issues as the RunUpdateJumpList API
was non-static. This CL fixes it by making this API and other APIs it
calls static.
3. Coalesce JumpList updates from two services by keeping only one
timer.
Previously, there were two timers to coalesce updates from the
TopSites service and the TabRestore service, respectively. When
notifications from both services came, even at the same time, multiple
RunUpdateJumpList tasks were posted where they should be coalesced
into one. This caused a waste of many shell notification runs. Note
that there was no wasted work on icon operations as different JumpList
categories were processed on demand. This CL trims out this waste by
keeping only one timer to coalesce updates triggered by different
services.
4. Cancel fetching most-visited URLs when Jumplist is destroyed.
This CL makes it possible to cancel the tasks of fetching most-visited
URLs when JumpList is destroyed, which otherwise is wasted. This also
avoids unnecessary favicon loading triggered upon the completion of
the URL fetch.
5. Make JumpList not refcounted.
JumpList is owned by BrowserView. It is RefCounted but there doesn't
appear to be a reason for that. This CL changes JumpList to be not
refcounted by making it a regular keyed service, rather than a
refcounted keyed service as it was.
BUG=40407, 179576, 498987, 717236, 720028, 732586
Review-Url: https://codereview.chromium.org/2931573003
Cr-Original-Commit-Position: refs/heads/master@{#479095}
Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd93686de54adbf
Review-Url: https://codereview.chromium.org/2931573003
Cr-Commit-Position: refs/heads/master@{#479848}
6 files changed
