Revert of Fix stability and data racing issues, coalesce more updates for JumpList

Revert of Fix stability and data racing issues, coalesce more updates for JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ )

Reason for revert:
Caused crash as recorded in crbug/733319 and crbug/733098

Original issue's description:
> 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/+/4a5397999a0f493c47632e1a8bd93686de54adbf

TBR=grt@chromium.org,pkasting@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=40407, 179576, 498987, 717236, 720028, 732586, 733319, 733098
Review-Url: https://codereview.chromium.org/2937993002
Cr-Commit-Position: refs/heads/master@{#479434}
Committed: https://chromium.googlesource.com/chromium/src/+/184a0e32ca40eac7c5f046aa840b31845e06afc0

[chengx, 2017-06-14 17:27:38]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-06-14 17:27:39]

Created Revert of Fix stability and data racing issues, coalesce more updates
for JumpList

[commit-bot: I haz the power, 2017-06-14 17:27:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 17:28:20]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1497461258729420, "parent_rev":
"31b4c18609a59916ac3d97ee75403a94f62ced94", "commit_rev":
"184a0e32ca40eac7c5f046aa840b31845e06afc0"}

[commit-bot: I haz the power, 2017-06-14 17:28:33]

Description was changed from

==========
Revert of Fix stability and data racing issues, coalesce more updates for
JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ )

Reason for revert:
Caused crash as recorded in crbug/733319 and crbug/733098

Original issue's description:
> 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...

TBR=grt@chromium.org,pkasting@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=40407, 179576, 498987, 717236, 720028, 732586
==========

to

==========
Revert of Fix stability and data racing issues, coalesce more updates for
JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ )

Reason for revert:
Caused crash as recorded in crbug/733319 and crbug/733098

Original issue's description:
> 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...

TBR=grt@chromium.org,pkasting@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=40407, 179576, 498987, 717236, 720028, 732586

Review-Url: https://codereview.chromium.org/2937993002
Cr-Commit-Position: refs/heads/master@{#479434}
Committed:
https://chromium.googlesource.com/chromium/src/+/184a0e32ca40eac7c5f046aa840b...
==========

[commit-bot: I haz the power, 2017-06-14 17:28:36]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/184a0e32ca40eac7c5f046aa840b...

[Will Harris, 2017-06-14 17:37:03]

Description was changed from

==========
Revert of Fix stability and data racing issues, coalesce more updates for
JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ )

Reason for revert:
Caused crash as recorded in crbug/733319 and crbug/733098

Original issue's description:
> 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...

TBR=grt@chromium.org,pkasting@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=40407, 179576, 498987, 717236, 720028, 732586

Review-Url: https://codereview.chromium.org/2937993002
Cr-Commit-Position: refs/heads/master@{#479434}
Committed:
https://chromium.googlesource.com/chromium/src/+/184a0e32ca40eac7c5f046aa840b...
==========

to

==========
Revert of Fix stability and data racing issues, coalesce more updates for
JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ )

Reason for revert:
Caused crash as recorded in crbug/733319 and crbug/733098

Original issue's description:
> 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...

TBR=grt@chromium.org,pkasting@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=40407, 179576, 498987, 717236, 720028, 732586, 733319, 733098

Review-Url: https://codereview.chromium.org/2937993002
Cr-Commit-Position: refs/heads/master@{#479434}
Committed:
https://chromium.googlesource.com/chromium/src/+/184a0e32ca40eac7c5f046aa840b...
==========