Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931573003:
Fix stability and data racing issues, coalesce more updates for JumpList (Closed)
DescriptionFix 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}
Committed: https://chromium.googlesource.com/chromium/src/+/f1c120eb244a567ec94bec0e8f43e076963bf550
Patch Set 1 #
Total comments: 106
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Remove refcounting for jumplist KeyedService, fix nits. #
Total comments: 20
Patch Set 4 : Fix nits and update comments #
Total comments: 6
Patch Set 5 : Address comments from grt@ #Patch Set 6 : Git pull #
Total comments: 4
Patch Set 7 : Obtain the pointer value before calling base::Passed() #
Total comments: 4
Patch Set 8 : Remove null check #
Messages
Total messages: 122 (102 generated)
Description was changed from ========== Use posttaskandreply BUG= ========== to ========== Use PostTaskAndReply for jumplist update BUG= ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use PostTaskAndReply for jumplist update BUG= ========== to ========== New design of jumplist workflow 1. One timer 2. Separate different states 3. Remove lock BUG= ==========
Description was changed from ========== New design of jumplist workflow 1. One timer 2. Separate different states 3. Remove lock BUG= ========== to ========== Fix data racing issue and stability issue for jumplist 1. Separate different jumplist states to fix data racing issue. 2. Make RunUpdateJumpList static so it outlives the jumplist instance. 3. Keep only one timer to remove redundant updates and to make code easier to understand. BUG= ==========
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
Description was changed from ========== Fix data racing issue and stability issue for jumplist 1. Separate different jumplist states to fix data racing issue. 2. Make RunUpdateJumpList static so it outlives the jumplist instance. 3. Keep only one timer to remove redundant updates and to make code easier to understand. BUG= ========== to ========== Fix stability/data-racing issues and simply update states for jumplist 1. Separate different jumplist states to fix data racing issue. 2. Make RunUpdateJumpList static so it outlives the jumplist instance. 3. Keep only one timer to remove redundant updates and to make code easier to understand. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and simply update states for jumplist 1. Separate different jumplist states to fix data racing issue. 2. Make RunUpdateJumpList static so it outlives the jumplist instance. 3. Keep only one timer to remove redundant updates and to make code easier to understand. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive the jumplist instance. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalescing more jumplist updates by keeping only one timer. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive the jumplist instance. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalescing more jumplist updates by keeping only one timer. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive the jumplist instance. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalescing more jumplist updates by keeping only one timer. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive the jumplist instance. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalescing more jumplist updates by keeping only one timer. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive the jumplist instance. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive the jumplist instance. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for jumplist This CL contains the following changes. 1. Fix the jumplist stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the jumplist data-racing issue by completely separating different thread tasks. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API and a few other API it calls static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG= ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG= ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 715008 ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 715008 ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of comment updates. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of comment updates. 1. Fix the JumpList stability issue by make RunUpdateJumpList API static so they outlive 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 causes stability issues if this RunUpdateJumpList API is non-static. This CL fixes it by making this API and other APIs it calls static. 2. Fix the JumpList data-racing issue by completely 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 so 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. 3. Coalesce more jumplist updates by keeping only one timer. Previously, there were two timers to coalesce updates from TopSites service and 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of comment updates. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of comment updates. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of comment updates. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
chengx@chromium.org changed reviewers: + grt@chromium.org
Hi Greg, this CL is ready for review now. PTAL, thanks! I want to specifically point out the following changes, which should make it easier for you to track the code changes. In jumplist.h 1) Struct JumpListData is deleted. All its variables except the lock are moved towards the end of this header file. 2) has_tab_closed_, most_visited_icons_, recently_closed_icons_ are not deleted. They are simply moved up a little bit for better organization. 3) Struct JumpListUpdateResults is introduced. 4) UpdateJumpList() is removed. Most of its content is moved into RunUpdateJumpList(). 5) A OnRunUpdateCompletion() method is added. 6) A DeferredChanged() method is added. In jumplist.cc 1) For some methods where the lock is removed, "side-by-side-diffs" is not able to recognize the changes. It just shows the deletion of a code block followed by adding another code block. The actual changes in these methods are much less than what the review tool shows. These methods include OnMostVisitedURLsAvailable(), OnFaviconDataAvailable(), DeferredTabRestoreServiceChanged().
round one! https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:229: scoped_refptr<history::TopSites> top_sites = nit: move this down just after the comment below with no blank lines between: // The comment scoped_refptr<>... = ...; if (top_sites) ...; https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:238: tab_restore_service->AddObserver(this); please add a doc comment explaining why the TRS is being observed. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:239: pref_change_registrar_.reset(new PrefChangeRegistrar); please add a doc comment explaining why kIncognitoModeAvailability is being monitored for changes. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:243: base::Bind(&JumpList::OnIncognitoAvailabilityChanged, this)); base::Unretained(this) since you're guaranteed that |this| will outlive pref_change_registrar_ -- the member's dtor will unregister the pref observer. please add a comment to this effect as well. for example: // base::Unretained is safe since |this| is guaranteed to outlive pref_change_registrar_. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:256: void JumpList::CancelPendingUpdate() { i think this should also cancel a pending MostVisitedURLs fetch via: weak_ptr_factory_.InvalidateWeakPtrs(); otherwise, one or more OnMostVisitedURLsAvailable calls could arrive, kicking off another favicon fetch. i don't think this is desired. see comment in DeferredTopSitesChanged for more details. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:268: if (profile_) { update_in_progress_ = false; just before this line https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:279: profile_ = NULL; nit: nullptr throughout https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:297: const size_t num_items = std::min(urls.size(), kMostVisitedItems); most_visited_pages_.reserve(num_items); icon_urls_.reserve(icon_urls_.size() + num_items); https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:298: for (size_t i = 0; i < urls.size() && i < kMostVisitedItems; i++) { ...; i < num_items; ++i) { (note: always prefer pre-increment) https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:309: icon_urls_.push_back(std::make_pair(url_string, link)); icon_urls_.emplace_back(std::move(url_string), std::move(link)); https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:323: if (update_in_progress_) please document this. for example: // Postpone handling this notification until a pending update completes. and put the same comment in TopSitesChanged https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:333: if (timer_.IsRunning()) { this block is repeated three times. please pull it out into its own function. ResetQuietPeriodTimer or something like that would have a nice ring to it. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:338: base::Bind(&JumpList::DeferredChanged, base::Unretained(this))); please add a comment such as: // base::Unretained is safe since |this| is guaranteed to outlive timer_. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:364: icon_urls_.push_back(std::make_pair(std::move(url), std::move(link))); icon_urls_.emplace_back(std::move(url), std::move(link)); https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:373: // This code enumerates all the tabs in the given window object and add their this comment adds nothing over the doc comment in the .h and the code below. please remove it. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:386: base::ElapsedTimer timer; do the early return before this line: if (icon_urls_.empty()) { // No more favicons are needed by the application JumpList. Schedule an // update on a background thread. PostRunUpdate(); return; } then the rest of the code is a bit simpler: base::ElapsedTimer timer; GURL url = GURL(icon_urls_.front().first); ... https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:409: base::Bind(&JumpList::OnFaviconDataAvailable, base::Unretained(this)), please add a comment such as: // base::Unretained is safe since |this| is guaranteed to outlive cancelable_task_tracker_. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:437: if (!icon_urls_.empty()) this condition is checked twice. how about: if (!icon_urls_.empty()) { if (!image_result.image.IsEmpty() && icon_urls_.front().second.get()) { ... } icon_urls_.pop_front(); } https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:456: if (!waiting_for_icons) if (icon_urls_.empty())) https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:464: if (!profile_) can this ever be true? as long as Terminate() really terminates everything, i'm not sure how it could be. please remove it unless you can determine an odd sequence of events that could lead to it being true. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:482: JumpListUpdateResults* update_results = new JumpListUpdateResults(); auto update_results = base::MakeUnique<JumpListUpdateResults>(); and std::move it as needed below. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:488: update_jumplist_task_runner_->PostTaskAndReply( PTAR returns a bool indicating whether or not the task may run at some point in the future. to handle this case, i suggest: if (!update_jumplist_task_runner_->PostTaskAndReply(...)) { OnRunUpdateCompletion(base::MakeUnique<JumpListUpdateResults>()); } so that the instance resets its state properly. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:495: base::Bind(&JumpList::OnRunUpdateCompletion, base::Unretained(this), base::Unretained -> weak_ptr_factory_.GetWeakPtr() https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:498: // Post a task to delete folders JumpListIcons and JumpListIconsOld as they wdyt of moving this stuff down into OnRunUpdateCompletion so that it doesn't contend with the update? https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:498: // Post a task to delete folders JumpListIcons and JumpListIconsOld as they nit: "...to delete the Foo and Bar folders as they..." https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:520: most_visited_has_pending_notification_ = true; DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:520: most_visited_has_pending_notification_ = true; "Most Visited" is the name of the jumplist section, whereas "Top Sites" is the thing in the browser that is being observed. I would rename this variable to top_sites_has_pending_notification_ or top_sites_updated_ (short and sweet). if you like that, please change recently_closed_has_pending_notification_ to recently_closed_updated_, too. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:542: if (updates_to_skip_ > 0) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:576: top_sites->GetMostVisitedURLs( i think you need to do something to deal with an outstanding GetMostVisitedURLs call so that subsequent TabRestoreServiceChanged or TopSitesChanged notifications can be handled properly. then, of course, you need to decide how to handle this case. i can think of a few ways: - clear most_visited_has_pending_notification_ in OnMostVisitedURLsAvailable rather than in DeferredChanged. you're then clear to use weak_ptr_factory_.InvalidateWeakPtrs() in CancelPendingUpdate() (as i suggested elsewhere) to cancel a previous call to GetMostVisitedURLs. it won't stop the call, but it'll ignore the result so that you don't end up with two calls to OnMostVisitedURLsAvailable. this is probably the simplest thing. - maintain state that there's an outstanding call to GetMostVisitedURLs so that subsequent calls to DeferredTopSitesChanged don't re-issue a new one. i'm less hot about this idea. - maybe there are other ways? https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:811: return; omit https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:22: #include "base/synchronization/lock.h" unused! hooray! https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:27: #include "components/history/core/browser/history_service.h" move to .cc https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:33: #include "content/public/browser/browser_thread.h" unused https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:64: // Note. base::CancelableTaskTracker is not thread safe, so we since all class methods are now required to run on the UI thread, i think this comment can disappear. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:68: public RefcountedKeyedService { can this be a regular KeyedService? i don't see any evidence of refcounting being used. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:73: struct JumpListUpdateResults { nit: just UpdateResults since this is within JumpList. also, can this type be private? https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:90: bool update_success_ = true; initialize to false and set it to true in the one success case rather than initializing to true and setting to false in the N failure cases. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:97: // Observer callback for TabRestoreService::Observer to notify when a tab is nit: move these overrides down into the private: section just before the TopSitesObserver overrides. the inconsistency here has always bugged me. :-) https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:97: // Observer callback for TabRestoreService::Observer to notify when a tab is nit: remove the comments on these two overrides. the only comment needed is: // sessions::TabRestoreServiceObserver: just above the two of them. please use the same notation for the other overrides as well. thanks. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:107: void CancelPendingUpdate(); this should be private: https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:112: void Terminate(); private as well https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:155: // Posts tasks to update the JumpList and delete any obsolete JumpList related nit: move this down below DeferredTabRestoreServiceChanged so that the class's methods are laid out more-or-less in the order they're invoked, grouped by thread. i would put the static methods below all other methods since they are their own unit and could potentially live somewhere else (e.g., all in an unnamed namespace or in a separate class that could be unit-tested independently). https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:166: void DeferredChanged(); nit: i think this should either be named after when it is called (e.g., OnDelayTimer) or for what it does (e.g., HandlePendingNotifications). https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:168: // Update the most visited URLs, called in DeferredChanged(). i don't think it's necessary to document who calls the function. it doesn't really matter. if the function has only one job, then maybe it's suitable to call it from multiple places some day. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:169: void DeferredTopSitesChanged(); since the "deferred" part is now taken care of by the method above, this could now aptly be named ProcessTopSitesNotification, or something like that. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:172: void DeferredTabRestoreServiceChanged(); ProcessTabRestoreServiceNotification? https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:223: // Lives on the UI thread. all members live on the UI thread now, so this comment is no longer needed. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:233: typedef std::pair<std::string, scoped_refptr<ShellLinkItem>> URLPair; wdyt of: using UrlAndLinkItem = ... https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:273: base::CancelableTaskTracker::TaskId task_id_; = base::CancelableTaskTracker::kBadTaskId and remove from the initializer list in the .cc
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Greg, thanks a lot for the highly detailed feedback! I've addressed all the 49 comments in the new patch set. I only reordered a few methods in the header file, not the cc file at all. The massive code movement will make it very hard to track the changes in the new patch set from the previous one. So I will reorder the methods later on. PTAL, thanks! https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:229: scoped_refptr<history::TopSites> top_sites = On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > nit: move this down just after the comment below with no blank lines between: > // The comment > scoped_refptr<>... = ...; > if (top_sites) > ...; Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:238: tab_restore_service->AddObserver(this); On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > please add a doc comment explaining why the TRS is being observed. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:239: pref_change_registrar_.reset(new PrefChangeRegistrar); On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > please add a doc comment explaining why kIncognitoModeAvailability is being > monitored for changes. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:243: base::Bind(&JumpList::OnIncognitoAvailabilityChanged, this)); On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > base::Unretained(this) since you're guaranteed that |this| will outlive > pref_change_registrar_ -- the member's dtor will unregister the pref observer. > please add a comment to this effect as well. for example: > // base::Unretained is safe since |this| is guaranteed to outlive > pref_change_registrar_. Changed to base::Unretained(this) and comments added. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:256: void JumpList::CancelPendingUpdate() { On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > i think this should also cancel a pending MostVisitedURLs fetch via: > weak_ptr_factory_.InvalidateWeakPtrs(); > otherwise, one or more OnMostVisitedURLsAvailable calls could arrive, kicking > off another favicon fetch. i don't think this is desired. see comment in > DeferredTopSitesChanged for more details. Thanks for the reminder! I've updated code to invalidate the weak pointer. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:268: if (profile_) { On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > update_in_progress_ = false; > just before this line Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:279: profile_ = NULL; On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > nit: nullptr throughout Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:297: On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > const size_t num_items = std::min(urls.size(), kMostVisitedItems); > most_visited_pages_.reserve(num_items); > icon_urls_.reserve(icon_urls_.size() + num_items); I introduced num_items to avoid duplicated value comparison in the for-loop. However, most_visited_pages_ and icon_urls_ are std::list rather than std::vector, so reserve() is not applicable here. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:298: for (size_t i = 0; i < urls.size() && i < kMostVisitedItems; i++) { On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > ...; i < num_items; ++i) { > > (note: always prefer pre-increment) Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:309: icon_urls_.push_back(std::make_pair(url_string, link)); On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > icon_urls_.emplace_back(std::move(url_string), std::move(link)); Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:323: if (update_in_progress_) On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > please document this. for example: > // Postpone handling this notification until a pending update completes. > > and put the same comment in TopSitesChanged Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:333: if (timer_.IsRunning()) { On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > this block is repeated three times. please pull it out into its own function. > ResetQuietPeriodTimer or something like that would have a nice ring to it. I have added a new InitializeTimerForUpdate method and put everything in it. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:338: base::Bind(&JumpList::DeferredChanged, base::Unretained(this))); On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > please add a comment such as: > // base::Unretained is safe since |this| is guaranteed to outlive timer_. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:364: icon_urls_.push_back(std::make_pair(std::move(url), std::move(link))); On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > icon_urls_.emplace_back(std::move(url), std::move(link)); Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:373: // This code enumerates all the tabs in the given window object and add their On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > this comment adds nothing over the doc comment in the .h and the code below. > please remove it. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:386: base::ElapsedTimer timer; On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > do the early return before this line: > > if (icon_urls_.empty()) { > // No more favicons are needed by the application JumpList. Schedule an > // update on a background thread. > PostRunUpdate(); > return; > } > > then the rest of the code is a bit simpler: > base::ElapsedTimer timer; > GURL url = GURL(icon_urls_.front().first); > ... Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:409: base::Bind(&JumpList::OnFaviconDataAvailable, base::Unretained(this)), On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > please add a comment such as: > // base::Unretained is safe since |this| is guaranteed to outlive > cancelable_task_tracker_. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:437: if (!icon_urls_.empty()) On 2017/06/09 10:42:55, grt (UTC plus 2) wrote: > this condition is checked twice. how about: > if (!icon_urls_.empty()) { > if (!image_result.image.IsEmpty() && icon_urls_.front().second.get()) { > ... > } > icon_urls_.pop_front(); > } Agreed this is the right thing to do. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:456: if (!waiting_for_icons) On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > if (icon_urls_.empty())) Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:464: if (!profile_) On 2017/06/09 10:42:55, grt (UTC plus 2) wrote: > can this ever be true? as long as Terminate() really terminates everything, i'm > not sure how it could be. please remove it unless you can determine an odd > sequence of events that could lead to it being true. I've removed it. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:482: JumpListUpdateResults* update_results = new JumpListUpdateResults(); On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > auto update_results = base::MakeUnique<JumpListUpdateResults>(); > and std::move it as needed below. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:488: update_jumplist_task_runner_->PostTaskAndReply( On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > PTAR returns a bool indicating whether or not the task may run at some point in > the future. to handle this case, i suggest: > if (!update_jumplist_task_runner_->PostTaskAndReply(...)) { > OnRunUpdateCompletion(base::MakeUnique<JumpListUpdateResults>()); > } > so that the instance resets its state properly. Thanks for the reminder! This is cool! https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:495: base::Bind(&JumpList::OnRunUpdateCompletion, base::Unretained(this), On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > base::Unretained -> weak_ptr_factory_.GetWeakPtr() Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:498: // Post a task to delete folders JumpListIcons and JumpListIconsOld as they On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > nit: "...to delete the Foo and Bar folders as they..." Comments updated. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:498: // Post a task to delete folders JumpListIcons and JumpListIconsOld as they On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > wdyt of moving this stuff down into OnRunUpdateCompletion so that it doesn't > contend with the update? SGTM. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:520: most_visited_has_pending_notification_ = true; On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK added. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:520: most_visited_has_pending_notification_ = true; On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > "Most Visited" is the name of the jumplist section, whereas "Top Sites" is the > thing in the browser that is being observed. I would rename this variable to > top_sites_has_pending_notification_ or top_sites_updated_ (short and sweet). if > you like that, please change recently_closed_has_pending_notification_ to > recently_closed_updated_, too. Thanks for the suggestion. I prefer top_sites_has_pending_notification_ though, because there is already a variable called most_visited_should_update_. TopSites having notifications doesn't mean most-visited category needs to be updated, because TopSites cares about the top 24 urls, while the most-visited category only cares about the top 5. I do need these two sets of flags for different purposes. I've also renamed other variables accordingly. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:542: if (updates_to_skip_ > 0) { On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:576: top_sites->GetMostVisitedURLs( On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > i think you need to do something to deal with an outstanding GetMostVisitedURLs > call so that subsequent TabRestoreServiceChanged or TopSitesChanged > notifications can be handled properly. then, of course, you need to decide how > to handle this case. i can think of a few ways: > > - clear most_visited_has_pending_notification_ in OnMostVisitedURLsAvailable > rather than in DeferredChanged. you're then clear to use > weak_ptr_factory_.InvalidateWeakPtrs() in CancelPendingUpdate() (as i suggested > elsewhere) to cancel a previous call to GetMostVisitedURLs. it won't stop the > call, but it'll ignore the result so that you don't end up with two calls to > OnMostVisitedURLsAvailable. this is probably the simplest thing. > > - maintain state that there's an outstanding call to GetMostVisitedURLs so that > subsequent calls to DeferredTopSitesChanged don't re-issue a new one. i'm less > hot about this idea. > > - maybe there are other ways? Thanks for the detailed suggestions! I'll go with the first idea. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:811: return; On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > omit Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:22: #include "base/synchronization/lock.h" On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > unused! hooray! Done. Sorry I forgot to update the header files. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:27: #include "components/history/core/browser/history_service.h" On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > move to .cc Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:33: #include "content/public/browser/browser_thread.h" On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > unused I think it's used in JumpList constructor. JumpList::JumpList(Profile* profile) : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread( content::BrowserThread::UI)), ... https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:64: // Note. base::CancelableTaskTracker is not thread safe, so we On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > since all class methods are now required to run on the UI thread, i think this > comment can disappear. Agreed. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:68: public RefcountedKeyedService { On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > can this be a regular KeyedService? i don't see any evidence of refcounting > being used. I think refcounting is used when one user (i.e., one profile) has multiple Chrome windows (each window may contain lots of tabs) open. Please see crrev/2323603002 where refcounting was introduced. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:73: struct JumpListUpdateResults { On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > nit: just UpdateResults since this is within JumpList. also, can this type be > private? Done. Renamed to UpdateResults and made private. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:90: bool update_success_ = true; On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > initialize to false and set it to true in the one success case rather than > initializing to true and setting to false in the N failure cases. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:97: // Observer callback for TabRestoreService::Observer to notify when a tab is On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > nit: remove the comments on these two overrides. the only comment needed is: > // sessions::TabRestoreServiceObserver: > just above the two of them. > > please use the same notation for the other overrides as well. thanks. Done. Comments updated. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:97: // Observer callback for TabRestoreService::Observer to notify when a tab is On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > nit: move these overrides down into the private: section just before the > TopSitesObserver overrides. the inconsistency here has always bugged me. :-) Done with moving the TopSitesObserver overrides to the private section down below. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:107: void CancelPendingUpdate(); On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > this should be private: Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:112: void Terminate(); On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > private as well Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:155: // Posts tasks to update the JumpList and delete any obsolete JumpList related On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > nit: move this down below DeferredTabRestoreServiceChanged so that the class's > methods are laid out more-or-less in the order they're invoked, grouped by > thread. i would put the static methods below all other methods since they are > their own unit and could potentially live somewhere else (e.g., all in an > unnamed namespace or in a separate class that could be unit-tested > independently). Sure. I moved some but in this header file. I think the massive code movement, especially in jumplist.cc for sure will mess up the "side-by-side-diff" between patch sets. So it'll be hard for you to track my patchset-wise changes. How about postponing this movement until everything else is settled down? I suggest to reorder the remaining methods using the final patch set. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:166: void DeferredChanged(); On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > nit: i think this should either be named after when it is called (e.g., > OnDelayTimer) or for what it does (e.g., HandlePendingNotifications). I'll go with OnDelayTimer. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:168: // Update the most visited URLs, called in DeferredChanged(). On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > i don't think it's necessary to document who calls the function. it doesn't > really matter. if the function has only one job, then maybe it's suitable to > call it from multiple places some day. I've updated the comments. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:169: void DeferredTopSitesChanged(); On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > since the "deferred" part is now taken care of by the method above, this could > now aptly be named ProcessTopSitesNotification, or something like that. Renamed to ProcessTopSitesNotification https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:172: void DeferredTabRestoreServiceChanged(); On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > ProcessTabRestoreServiceNotification? SGTM. I've renamed the variable. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:223: // Lives on the UI thread. On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > all members live on the UI thread now, so this comment is no longer needed. Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:233: typedef std::pair<std::string, scoped_refptr<ShellLinkItem>> URLPair; On 2017/06/09 10:42:57, grt (UTC plus 2) wrote: > wdyt of: > using UrlAndLinkItem = ... Done. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:273: base::CancelableTaskTracker::TaskId task_id_; On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > = base::CancelableTaskTracker::kBadTaskId > and remove from the initializer list in the .cc Done.
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of comment updates. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
Description was changed from ========== Fix stability/data-racing issues and coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ==========
looking awesome. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:297: On 2017/06/10 05:56:17, chengx wrote: > On 2017/06/09 10:42:56, grt (UTC plus 2) wrote: > > const size_t num_items = std::min(urls.size(), kMostVisitedItems); > > most_visited_pages_.reserve(num_items); > > icon_urls_.reserve(icon_urls_.size() + num_items); > > I introduced num_items to avoid duplicated value comparison in the for-loop. > However, most_visited_pages_ and icon_urls_ are std::list rather than > std::vector, so reserve() is not applicable here. :-) https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:33: #include "content/public/browser/browser_thread.h" On 2017/06/10 05:56:18, chengx wrote: > On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > > unused > > I think it's used in JumpList constructor. > > JumpList::JumpList(Profile* profile) > : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread( > content::BrowserThread::UI)), ... Please move it to the .cc file as per iwyu. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:68: public RefcountedKeyedService { On 2017/06/10 05:56:18, chengx wrote: > On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > > can this be a regular KeyedService? i don't see any evidence of refcounting > > being used. > > I think refcounting is used when one user (i.e., one profile) has multiple > Chrome windows (each window may contain lots of tabs) open. Please see > crrev/2323603002 where refcounting was introduced. Making it a keyed service accomplishes that (one instance per Profile rather than one per browser window). I don't see any reason for it to be refcounted. https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:155: // Posts tasks to update the JumpList and delete any obsolete JumpList related On 2017/06/10 05:56:18, chengx wrote: > On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > > nit: move this down below DeferredTabRestoreServiceChanged so that the class's > > methods are laid out more-or-less in the order they're invoked, grouped by > > thread. i would put the static methods below all other methods since they are > > their own unit and could potentially live somewhere else (e.g., all in an > > unnamed namespace or in a separate class that could be unit-tested > > independently). > > Sure. I moved some but in this header file. I think the massive code movement, > especially in jumplist.cc for sure will mess up the "side-by-side-diff" between > patch sets. So it'll be hard for you to track my patchset-wise changes. How > about postponing this movement until everything else is settled down? I suggest > to reorder the remaining methods using the final patch set. Good plan. https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:449: // Since neither the "Most Visited" category nor the "Recently Closed" i don't understand what this comment means. there's no code below it that marks any flags. https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:543: top_sites_has_pending_notification_ = false; should this not be true until the results are received in OnMostVisitedURLsAvailable?
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Greg, I've removed refcounting for jumplist KeyedService in the new patch set, together with other small fixes. In browser_view.cc, the DCHECK I removed kept failing when I ran chromium.exe locally. I am not sure if it's expected or not when the refcounting is removed. Any thoughts please? PTAL, thanks! https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:33: #include "content/public/browser/browser_thread.h" On 2017/06/12 07:15:33, grt (UTC plus 2) wrote: > On 2017/06/10 05:56:18, chengx wrote: > > On 2017/06/09 10:42:58, grt (UTC plus 2) wrote: > > > unused > > > > I think it's used in JumpList constructor. > > > > JumpList::JumpList(Profile* profile) > > : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread( > > content::BrowserThread::UI)), ... > > Please move it to the .cc file as per iwyu. Done. https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:449: // Since neither the "Most Visited" category nor the "Recently Closed" On 2017/06/12 07:15:33, grt (UTC plus 2) wrote: > i don't understand what this comment means. there's no code below it that marks > any flags. I've removed those obsolete comments. https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:543: top_sites_has_pending_notification_ = false; On 2017/06/12 07:15:33, grt (UTC plus 2) wrote: > should this not be true until the results are received in > OnMostVisitedURLsAvailable? Ah, I forgot to delete this line of code. Now it's removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
down to the nits. maybe do the reordering in its own CL that contains no other changes. wdyt? https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:18: #include "base/memory/ref_counted.h" this is still needed for the scoped_refptr members https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (left): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2181: DCHECK(!jumplist_.get()); this was failing because the member was uninitialized. https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:691: // The custom JumpList for Windows 7. nit: remove "7" since the jumplist applies to all versions of Windows that Chrome runs on. https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:692: JumpList* jumplist_; = nullptr; https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:46: #include "content/public/browser/browser_thread.h" is this now unused? https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:57: using content::BrowserThread; unused https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:71: explicit JumpList(Profile* profile); // Use JumpListFactory instead nit: move these methods down below the types as per https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:85: URLIconCache most_visited_icons_in_update_; nit: omit the trailing underscore for these public struct members. https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:228: using UrlAndLinkItem = std::pair<std::string, scoped_refptr<ShellLinkItem>>; nit: move this up to the top of the private: section as per declaration order https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_factory.h (right): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_factory.h:9: #include "chrome/browser/win/jumplist.h" nit: could this be moved to the .cc file in favor of a forward-decl?
Patchset #4 (id:300001) has been deleted
Patchset #4 (id:320001) has been deleted
Patchset #4 (id:340001) has been deleted
Patchset #4 (id:360001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chengx@chromium.org changed reviewers: + pkasting@chromium.org
Hi Greg, I've address all the comments in the new patch set. PTAL, thanks!
+pkasting@ to cover browser_view.{h,cc}, where grt@ doesn't seem to be an owner.
Thanks Peter!
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> down to the nits. maybe do the reordering in its own CL that contains no other
> changes. wdyt?
I agree with this idea.
https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):
https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:297:
On 2017/06/12 07:15:32, grt (UTC plus 2) wrote:
> On 2017/06/10 05:56:17, chengx wrote:
> > On 2017/06/09 10:42:56, grt (UTC plus 2) wrote:
> > > const size_t num_items = std::min(urls.size(), kMostVisitedItems);
> > > most_visited_pages_.reserve(num_items);
> > > icon_urls_.reserve(icon_urls_.size() + num_items);
> >
> > I introduced num_items to avoid duplicated value comparison in the for-loop.
> > However, most_visited_pages_ and icon_urls_ are std::list rather than
> > std::vector, so reserve() is not applicable here.
>
> :-)
Acknowledged.
https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):
https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:68: public RefcountedKeyedService {
On 2017/06/12 07:15:32, grt (UTC plus 2) wrote:
> On 2017/06/10 05:56:18, chengx wrote:
> > On 2017/06/09 10:42:58, grt (UTC plus 2) wrote:
> > > can this be a regular KeyedService? i don't see any evidence of
refcounting
> > > being used.
> >
> > I think refcounting is used when one user (i.e., one profile) has multiple
> > Chrome windows (each window may contain lots of tabs) open. Please see
> > crrev/2323603002 where refcounting was introduced.
>
> Making it a keyed service accomplishes that (one instance per Profile rather
> than one per browser window). I don't see any reason for it to be refcounted.
I've made it not refcounted.
https://codereview.chromium.org/2931573003/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:155: // Posts tasks to update the JumpList and
delete any obsolete JumpList related
On 2017/06/12 07:15:32, grt (UTC plus 2) wrote:
> On 2017/06/10 05:56:18, chengx wrote:
> > On 2017/06/09 10:42:58, grt (UTC plus 2) wrote:
> > > nit: move this down below DeferredTabRestoreServiceChanged so that the
> class's
> > > methods are laid out more-or-less in the order they're invoked, grouped by
> > > thread. i would put the static methods below all other methods since they
> are
> > > their own unit and could potentially live somewhere else (e.g., all in an
> > > unnamed namespace or in a separate class that could be unit-tested
> > > independently).
> >
> > Sure. I moved some but in this header file. I think the massive code
movement,
> > especially in jumplist.cc for sure will mess up the "side-by-side-diff"
> between
> > patch sets. So it'll be hard for you to track my patchset-wise changes. How
> > about postponing this movement until everything else is settled down? I
> suggest
> > to reorder the remaining methods using the final patch set.
>
> Good plan.
Acknowledged.
https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):
https://codereview.chromium.org/2931573003/diff/260001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:18: #include "base/memory/ref_counted.h"
On 2017/06/12 20:43:38, grt (UTC plus 2) wrote:
> this is still needed for the scoped_refptr members
Done. Added back.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (left):
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:2181: DCHECK(!jumplist_.get());
On 2017/06/12 20:43:38, grt (UTC plus 2) wrote:
> this was failing because the member was uninitialized.
Done.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.h (right):
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.h:691: // The custom JumpList for
Windows 7.
On 2017/06/12 20:43:38, grt (UTC plus 2) wrote:
> nit: remove "7" since the jumplist applies to all versions of Windows that
> Chrome runs on.
Done.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.h:692: JumpList* jumplist_;
On 2017/06/12 20:43:38, grt (UTC plus 2) wrote:
> = nullptr;
Done.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:46: #include
"content/public/browser/browser_thread.h"
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> is this now unused?
Done. Deleted.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:57: using content::BrowserThread;
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> unused
Done.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:71: explicit JumpList(Profile* profile); // Use
JumpListFactory instead
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> nit: move these methods down below the types as per
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
Factory methods and the destructor moved down below "struct UpdateResults".
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:85: URLIconCache most_visited_icons_in_update_;
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> nit: omit the trailing underscore for these public struct members.
Done.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:228: using UrlAndLinkItem = std::pair<std::string,
scoped_refptr<ShellLinkItem>>;
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> nit: move this up to the top of the private: section as per declaration order
Done.
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_factory.h (right):
https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum...
chrome/browser/win/jumplist_factory.h:9: #include
"chrome/browser/win/jumplist.h"
On 2017/06/12 20:43:39, grt (UTC plus 2) wrote:
> nit: could this be moved to the .cc file in favor of a forward-decl?
I don't think so. The return type of GetForProfile is JumpList*. So the
definition of JumpList is needed in this header file.
Description was changed from ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407 ========== to ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 498987 ==========
browser_view.* LGTM with caveat https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:692: JumpList* jumplist_ = nullptr; I'm a fan of this kind of in-declaration initialization. But I'm not a fan of doing it for just one BT member and not the others. If you want to do this, I would write a separate CL that converts all members of this class to use this form of initialization where possible.
Description was changed from ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 498987 ========== to ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 498987, 720028 ==========
Description was changed from ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 498987, 720028 ========== to ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 498987, 720028, 732586 ==========
Description was changed from ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following three main changes plus lots of small updates on variable/method names and comments. 1. 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. 2. 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. 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. BUG=40407, 498987, 720028, 732586 ========== to ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following four main changes plus lots of small 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. 4. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. BUG=40407, 498987, 720028, 732586 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following four main changes plus lots of small 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 steps. Note that there was no wasted work on icon operations as different JumpList categories are processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates from both services. 4. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. BUG=40407, 498987, 720028, 732586 ========== to ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following four main changes plus lots of small 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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, 498987, 720028, 732586 ==========
Description was changed from ========== Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following four main changes plus lots of small 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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, 498987, 720028, 732586 ========== to ========== 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. 5. Cancel fetching most-visited URLs when Jumplist is destryped. This CL makes it possible to cancel the task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. BUG=40407, 498987, 717236, 720028, 732586 ==========
Description was changed from ========== 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. 5. Cancel fetching most-visited URLs when Jumplist is destryped. This CL makes it possible to cancel the task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. BUG=40407, 498987, 717236, 720028, 732586 ========== to ========== 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. 5. Cancel fetching most-visited URLs when Jumplist is destroyed. This CL makes it possible to cancel the task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. BUG=40407, 498987, 717236, 720028, 732586 ==========
Description was changed from ========== 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. 5. Cancel fetching most-visited URLs when Jumplist is destroyed. This CL makes it possible to cancel the task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. BUG=40407, 498987, 717236, 720028, 732586 ========== to ========== 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. 5. Cancel fetching most-visited URLs when Jumplist is destroyed. This CL makes it possible to cancel the task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. BUG=40407, 179576, 498987, 717236, 720028, 732586 ==========
lgtm w/ nit/q below https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_factory.h (right): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_factory.h:9: #include "chrome/browser/win/jumplist.h" On 2017/06/12 22:05:30, chengx wrote: > On 2017/06/12 20:43:39, grt (UTC plus 2) wrote: > > nit: could this be moved to the .cc file in favor of a forward-decl? > > I don't think so. The return type of GetForProfile is JumpList*. So the > definition of JumpList is needed in this header file. A forward decl is fine in that case -- consumers of this .h only need to know that "JumpList" is a class. They will need to include jumplist.h only if they wish to dereference the pointer. https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2182: jumplist_ = JumpListFactory::GetForProfile(browser_->profile()); on the other hand, is this jumplist_ member even needed? could this be simply: // Ensure that this browser's Profile has a JumpList so that the JumpList is kept up to date. JumpListFactory::GetForProfile(browser_->profile()); https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:692: JumpList* jumplist_ = nullptr; On 2017/06/12 22:21:54, Peter Kasting wrote: > I'm a fan of this kind of in-declaration initialization. But I'm not a fan of > doing it for just one BT member and not the others. If you want to do this, I > would write a separate CL that converts all members of this class to use this > form of initialization where possible. I agree with Peter here. Apologies for suggesting initializing it here rather than in the .cc file.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. Make JumpList not refcounted. Owned by BrowserView, JumpList 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. 5. Cancel fetching most-visited URLs when Jumplist is destroyed. This CL makes it possible to cancel the task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. BUG=40407, 179576, 498987, 717236, 720028, 732586 ========== to ========== 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 task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. 5. Make JumpList not refcounted. Owned by BrowserView, JumpList 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 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 task of fetching most-visited URLs when JumpList is destroyed, which otherwise is completely wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. 5. Make JumpList not refcounted. Owned by BrowserView, JumpList 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 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've addressed all the comments in the new patch set. Thanks for reviewing this CL! https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... File chrome/browser/win/jumplist_factory.h (right): https://codereview.chromium.org/2931573003/diff/280001/chrome/browser/win/jum... chrome/browser/win/jumplist_factory.h:9: #include "chrome/browser/win/jumplist.h" On 2017/06/13 07:32:27, grt (UTC plus 2) wrote: > On 2017/06/12 22:05:30, chengx wrote: > > On 2017/06/12 20:43:39, grt (UTC plus 2) wrote: > > > nit: could this be moved to the .cc file in favor of a forward-decl? > > > > I don't think so. The return type of GetForProfile is JumpList*. So the > > definition of JumpList is needed in this header file. > > A forward decl is fine in that case -- consumers of this .h only need to know > that "JumpList" is a class. They will need to include jumplist.h only if they > wish to dereference the pointer. Done. You are right. I only need to tell the header file that JumpList is a class. Thanks! https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2182: jumplist_ = JumpListFactory::GetForProfile(browser_->profile()); On 2017/06/13 07:32:27, grt (UTC plus 2) wrote: > on the other hand, is this jumplist_ member even needed? could this be simply: > // Ensure that this browser's Profile has a JumpList so that the JumpList is > kept up to date. > JumpListFactory::GetForProfile(browser_->profile()); I don't think |jumplist_| is needed, so removed it. https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:692: JumpList* jumplist_ = nullptr; On 2017/06/12 22:21:54, Peter Kasting wrote: > I'm a fan of this kind of in-declaration initialization. But I'm not a fan of > doing it for just one BT member and not the others. If you want to do this, I > would write a separate CL that converts all members of this class to use this > form of initialization where possible. Done. https://codereview.chromium.org/2931573003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:692: JumpList* jumplist_ = nullptr; On 2017/06/13 07:32:27, grt (UTC plus 2) wrote: > On 2017/06/12 22:21:54, Peter Kasting wrote: > > I'm a fan of this kind of in-declaration initialization. But I'm not a fan of > > doing it for just one BT member and not the others. If you want to do this, I > > would write a separate CL that converts all members of this class to use this > > form of initialization where possible. > > I agree with Peter here. Apologies for suggesting initializing it here rather > than in the .cc file. I've landed crrev/2939583002 to apply the in-declaration initialization to all member variables where possible in this class.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2931573003/#ps420001 (title: "Git pull")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1497381374944890,
"parent_rev": "1e56a67c9439c93cda360eb2a95bee5af8b67b62", "commit_rev":
"4a5397999a0f493c47632e1a8bd93686de54adbf"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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-Commit-Position: refs/heads/master@{#479095} Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd9... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:420001) as https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd9...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:420001) has been created in https://codereview.chromium.org/2937993002/ by chengx@chromium.org. The reason for reverting is: Caused crash as recorded in crbug/733319 and crbug/733098.
Message was sent while issue was closed.
https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:480: incognito_availability, update_results.get()), oh, snap! update_results.get() returns nullptr since the std::move() on line 483 has already executed.
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:480: incognito_availability, update_results.get()), On 2017/06/14 20:11:32, grt (UTC plus 2) wrote: > oh, snap! update_results.get() returns nullptr since the std::move() on line 483 > has already executed. Yep! Or, to be more precise "returns nullptr *if* the std::move()" has already executed, which is unspecified. Which is why the code worked fine in the test configuration.
Message was sent while issue was closed.
Patchset #7 (id:440001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#479095} Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd9... ========== to ========== 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-Commit-Position: refs/heads/master@{#479095} Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd9... ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:460001) has been deleted
Patchset #7 (id:480001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL the new patch set which fixes the crash issue. The pattern of obtaining the pointer value before calling base::Passed() is widely used in the code base. So I think the new patch set should fix the crash. Unfortunately, I was not able to test it as I could not repro the crash with 64 bit release unofficial build. The last possible reason I can think of is the official build. I tried to build the official version, but kept getting wired linking errors. I will keep trying to repro the crash locally. To further prevent crashing Canary, I am making the jumplist update call return immediately if the input pointer is null though it is very unlikely now. How do you think please? https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:480: incognito_availability, update_results.get()), On 2017/06/14 20:11:32, grt (UTC plus 2) wrote: > oh, snap! update_results.get() returns nullptr since the std::move() on line 483 > has already executed. Done. https://codereview.chromium.org/2931573003/diff/420001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:480: incognito_availability, update_results.get()), On 2017/06/14 20:16:13, brucedawson wrote: > On 2017/06/14 20:11:32, grt (UTC plus 2) wrote: > > oh, snap! update_results.get() returns nullptr since the std::move() on line > 483 > > has already executed. > > Yep! > > Or, to be more precise "returns nullptr *if* the std::move()" has already > executed, which is unspecified. Which is why the code worked fine in the test > configuration. Done.
lgtm provided that you remove the null check https://codereview.chromium.org/2931573003/diff/500001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/500001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:475: auto* update_results_raw = update_results.get(); this is fine. another way to slice it would be to do the first bind here: auto run_update_jumplist = base::Bind(..., update_results.get(), ...); and then move it into PTAR below: if (!..PostTaskAndReply(FROM_HERE, std::move(run_update_jumplist),...) this avoids having a naked pointer dangling. https://codereview.chromium.org/2931573003/diff/500001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:705: if (!update_results) please don't do this. this will hide a bug leading to a flaky product rather than exposing a crasher which we can quickly fix.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments addressed. Thanks for the suggestions! https://codereview.chromium.org/2931573003/diff/500001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2931573003/diff/500001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:475: auto* update_results_raw = update_results.get(); On 2017/06/15 09:36:44, grt (UTC plus 2) wrote: > this is fine. another way to slice it would be to do the first bind here: > auto run_update_jumplist = base::Bind(..., update_results.get(), ...); > and then move it into PTAR below: > if (!..PostTaskAndReply(FROM_HERE, std::move(run_update_jumplist),...) > > this avoids having a naked pointer dangling. Thanks for the suggestion. I'll go with the former method for now. https://codereview.chromium.org/2931573003/diff/500001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:705: if (!update_results) On 2017/06/15 09:36:44, grt (UTC plus 2) wrote: > please don't do this. this will hide a bug leading to a flaky product rather > than exposing a crasher which we can quickly fix. Done. Removed.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2931573003/#ps490007 (title: "Remove null check")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 490007, "attempt_start_ts": 1497562294878350,
"parent_rev": "5235b8e3a5ef55769b4cb47335beca851a70528b", "commit_rev":
"f1c120eb244a567ec94bec0e8f43e076963bf550"}
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#479095} Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd9... ========== to ========== 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/+/4a5397999a0f493c47632e1a8bd9... Review-Url: https://codereview.chromium.org/2931573003 Cr-Commit-Position: refs/heads/master@{#479848} Committed: https://chromium.googlesource.com/chromium/src/+/f1c120eb244a567ec94bec0e8f43... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:490007) as https://chromium.googlesource.com/chromium/src/+/f1c120eb244a567ec94bec0e8f43... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
