Filter redundant JumpList favicons' fetching and related

Filter redundant JumpList favicons' fetching and related

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
Review-Url: https://codereview.chromium.org/2859693002
Cr-Commit-Position: refs/heads/master@{#470091}
Committed: https://chromium.googlesource.com/chromium/src/+/9c0c398442ed5dc4d3c52f735b57cb84a22c977d

[chengx, 2017-05-02 21:45:59]

Description was changed from

==========
Delay updating URLs

BUG=
==========

to

==========
Filter redundant ShellLinkItem creation and favicon loading for JumpList

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:00:40]

Description was changed from

==========
Filter redundant ShellLinkItem creation and favicon loading for JumpList

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant ShellLinkItem creation and favicon loading for JumpList

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update includes 1) retrieve updated URLs, create ShellLinkItem 
and load the favicons for these URLs; 2) schedule a RunUpdateJumpList 
call to update icons in the jumplisticon* folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
important to optimize them.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:01:10]

Description was changed from

==========
Filter redundant ShellLinkItem creation and favicon loading for JumpList

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update includes 1) retrieve updated URLs, create ShellLinkItem 
and load the favicons for these URLs; 2) schedule a RunUpdateJumpList 
call to update icons in the jumplisticon* folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
important to optimize them.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant ShellLinkItem creation and favicon loading for JumpList

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update includes 1) retrieve updated URLs, create ShellLinkItem 
and load the favicons for these URLs; 2) schedule a RunUpdateJumpList 
call to update icons in the jumplisticon* folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:01:21]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 22:02:02]

Dry run: CQ is trying da patch.

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

[chengx, 2017-05-02 22:08:58]

Description was changed from

==========
Filter redundant ShellLinkItem creation and favicon loading for JumpList

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update includes 1) retrieve updated URLs, create ShellLinkItem 
and load the favicons for these URLs; 2) schedule a RunUpdateJumpList 
call to update icons in the jumplisticon* folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update includes 1) retrieve updated URLs, create ShellLinkItem 
and load the favicons for these URLs; 2) schedule a RunUpdateJumpList 
call to update icons in the jumplisticon* folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:10:15]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update includes 1) retrieve updated URLs, create ShellLinkItem 
and load the favicons for these URLs; 2) schedule a RunUpdateJumpList 
call to update icons in the jumplisticon* folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps 
1) retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:10:29]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps 
1) retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:10:40]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 22:14:50]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of very close jumplist updates. 

However, we're not coalescing step 1 of these very close jumplist 
updates, which makes some of them completely wasted. Therefore, we 
should apply this delay and coalesce strategy to step 1 as well. 

Moreover, in OnMostVisitedURLsAvailable(), we're processing more URLs 
(typically 24) (ShellLinkItem creation and favicon loading) than 
needed (between 5 to 9). This is also a waste of resources. 

Btw, all these tasks are taking place on UI thread. Therefore, it's 
very important to optimize them.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 23:53:27]

chengx@chromium.org changed reviewers:
+ grt@chromium.org

[chengx, 2017-05-02 23:53:28]

PTAL, thanks!

Hi Greg, you may have noticed that I've sent you another CL
crrev.com/2860573005, namely "Log runtime of StartLoadingFavicon and
OnFaviconDataAvailable for JumpList".

The reason why I didn't merge them into one is that I want to land that CL
sooner than this one by one day or two, so that I can gather data before and
after landing this CL. Considering that this CL may need more rounds of review
feedback, I am sending it out now rather than waiting for the other one to land.

[chengx, 2017-05-03 00:30:57]

Patchset #1 (id:1) has been deleted

[chengx, 2017-05-03 00:31:04]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:32:24]

Dry run: 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-05-03 01:28:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 01:28:41]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-03 06:47:05]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
    for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
    folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-03 06:47:14]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
    for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
    folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
    folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-03 06:47:24]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
    folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-03 06:48:15]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The 
ShellLinkItem creation and favicon loading for these URLs are also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The work of
ShellLinkItem creation and favicon loading for these URLs is also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[grt (UTC plus 2), 2017-05-03 10:51:48]

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:442: void JumpList::Terminate() {
stop the timers in here, no?

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:470: // There're at most 9 JumpList items can be
displayed for the "Most Visited"
nit: "At most 9..."

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:508: timer_recently_closed_.Start(FROM_HERE,
kDelayForJumplistUpdate, this,
i suggest making the bind explicit so that you're not relying on a method in
BaseTimerMethodPointer (which is documented as an implementation detail):
base::Bind(&JumpList::DeferredTabRestoreServiceChanged, base::Unretained(this))

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:537: for (const auto& entry :
tab_restore_service->entries()) {
should this loop terminate early if recently_closed_pages_ fills up?

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:714: // Initialize the one-shot timer to update
the the "Most visited" category in
CancelPendingUpdate here, or is there a reason to let it continue while waiting
for the quiet period to pass? same comment for
JumpList::TabRestoreServiceChanged.

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:724: base::Unretained(this), top_sites));
rather than holding a reference to TopSites through the call, i think it's
better to use TopSitesFactory::GetForProfile to get the instance anew in the
callback.

[chengx, 2017-05-03 20:41:09]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 20:41:52]

Dry run: CQ is trying da patch.

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

[chengx, 2017-05-03 20:45:14]

PTAL, thanks!

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:442: void JumpList::Terminate() {
On 2017/05/03 10:51:48, grt (UTC plus 2) wrote:
> stop the timers in here, no?

Done. They should be stopped.

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:470: // There're at most 9 JumpList items can be
displayed for the "Most Visited"
On 2017/05/03 10:51:48, grt (UTC plus 2) wrote:
> nit: "At most 9..."

Done.

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:508: timer_recently_closed_.Start(FROM_HERE,
kDelayForJumplistUpdate, this,
On 2017/05/03 10:51:48, grt (UTC plus 2) wrote:
> i suggest making the bind explicit so that you're not relying on a method in
> BaseTimerMethodPointer (which is documented as an implementation detail):
> base::Bind(&JumpList::DeferredTabRestoreServiceChanged,
base::Unretained(this))

Done.

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:537: for (const auto& entry :
tab_restore_service->entries()) {
On 2017/05/03 10:51:48, grt (UTC plus 2) wrote:
> should this loop terminate early if recently_closed_pages_ fills up?

Yes, it should. Early return is enabled in both AddTab() and AddWindow() if this
fill-up check passes. It should be enabled for this for-loop to save the efforts
even more.

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:714: // Initialize the one-shot timer to update
the the "Most visited" category in
On 2017/05/03 10:51:48, grt (UTC plus 2) wrote:
> CancelPendingUpdate here, or is there a reason to let it continue while
waiting
> for the quiet period to pass? same comment for
> JumpList::TabRestoreServiceChanged.

Yes, it should be called here. I've made the updates. Thanks!

https://codereview.chromium.org/2859693002/diff/20001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:724: base::Unretained(this), top_sites));
On 2017/05/03 10:51:48, grt (UTC plus 2) wrote:
> rather than holding a reference to TopSites through the call, i think it's
> better to use TopSitesFactory::GetForProfile to get the instance anew in the
> callback.

Done.

[commit-bot: I haz the power, 2017-05-03 21:29:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 21:29:19]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-05-04 11:32:43]

lgtm w/nits

https://codereview.chromium.org/2859693002/diff/40001/chrome/browser/win/jump...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859693002/diff/40001/chrome/browser/win/jump...
chrome/browser/win/jumplist.h:105: // Called on a timer to update the "Recently
Closed" category of JumpList
nit: it's generally nice to group overrides together based on the originating
class. please move this method declaration (and its defn in the .cc file) so
that it isn't sandwiched between these overrides. also, it should be private if
possible. maybe put it down near DeferredTopSitesChanged.

https://codereview.chromium.org/2859693002/diff/40001/chrome/browser/win/jump...
chrome/browser/win/jumplist.h:163: // available. This function updates the
ShellLinkItemList objects and sen
"...and sen" -> "...and begins the process of fetching favicons for the URLs."
or something like that

[chengx, 2017-05-04 17:34:41]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon loading 

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and load the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that any other RunUpdateJumpList calls requested in this 
period will be collapsed into one which can prevent update storms. 
This says that we are coalescing step 2 of jumplist updates that are
close to each other.

However, we're not coalescing step 1 of these jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (between 5 to 9). The work of
ShellLinkItem creation and favicon loading for these URLs is also
wasted.

Note that all these tasks are taking place on UI thread. Therefore, 
it's very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon fetch

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work ofShellLinkItem creation and favicon loading for these URLs is 
also wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-04 20:07:00]

Patchset #3 (id:60001) has been deleted

[chengx, 2017-05-04 20:38:10]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 20:38:47]

Dry run: CQ is trying da patch.

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

[chengx, 2017-05-04 20:45:36]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon fetch

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work ofShellLinkItem creation and favicon loading for these URLs is 
also wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItem creation and favicon fetch

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-04 20:46:19]

Description was changed from

==========
Filter redundant JumpList ShellLinkItem creation and favicon fetch

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList ShellLinkItems' creation and favicons' 
fetching

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-04 20:47:08]

Description was changed from

==========
Filter redundant JumpList ShellLinkItems' creation and favicons' 
fetching

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList favicons' fetching and related

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-04 20:48:04]

I've fixed all the nits in the new patch set. Thanks!

https://codereview.chromium.org/2859693002/diff/40001/chrome/browser/win/jump...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859693002/diff/40001/chrome/browser/win/jump...
chrome/browser/win/jumplist.h:105: // Called on a timer to update the "Recently
Closed" category of JumpList
On 2017/05/04 11:32:43, grt (UTC plus 2) wrote:
> nit: it's generally nice to group overrides together based on the originating
> class. please move this method declaration (and its defn in the .cc file) so
> that it isn't sandwiched between these overrides. also, it should be private
if
> possible. maybe put it down near DeferredTopSitesChanged.

Done.

https://codereview.chromium.org/2859693002/diff/40001/chrome/browser/win/jump...
chrome/browser/win/jumplist.h:163: // available. This function updates the
ShellLinkItemList objects and sen
On 2017/05/04 11:32:43, grt (UTC plus 2) wrote:
> "...and sen" -> "...and begins the process of fetching favicons for the URLs."
> or something like that

Done.

[chengx, 2017-05-04 21:26:53]

chengx@chromium.org changed reviewers:
+ brucedawson@chromium.org

[chengx, 2017-05-04 21:26:53]

Hi Bruce, FYI this is the CL where I extend your "delay and coalesce" idea to
cover the entire jumplist update.

[commit-bot: I haz the power, 2017-05-04 21:27:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 21:27:15]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-08 16:15:12]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-05-08 16:15:13]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2859693002/#ps100001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-05-08 16:15:58]

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-05-08 16:35:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 16:35:39]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chengx, 2017-05-08 16:56:49]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 16:57:16]

Dry run: 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-05-08 17:13:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:13:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chengx, 2017-05-08 17:29:38]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 17:30:10]

Dry run: CQ is trying da patch.

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

[chengx, 2017-05-08 17:42:58]

Patchset #5 (id:120001) has been deleted

[chengx, 2017-05-08 17:43:32]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-05-08 18:43:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 18:43:20]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-08 18:43:31]

The CQ bit was unchecked by chengx@chromium.org

[chengx, 2017-05-08 18:43:32]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-05-08 18:43:32]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2859693002/#ps140001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-05-08 18:44:49]

CQ is trying da patch.

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

[chengx, 2017-05-08 18:44:50]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-05-08 18:44:51]

The CQ bit was unchecked by chengx@chromium.org

[chengx, 2017-05-08 19:01:15]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-05-08 19:02:01]

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-05-08 19:57:41]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494270075183330,
"parent_rev": "c487f12607e6cde6cbe17854013e355a18e71b5d", "commit_rev":
"9c0c398442ed5dc4d3c52f735b57cb84a22c977d"}

[commit-bot: I haz the power, 2017-05-08 19:58:02]

Description was changed from

==========
Filter redundant JumpList favicons' fetching and related

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236
==========

to

==========
Filter redundant JumpList favicons' fetching and related

In the current JumpList implementation, each time when the TopSites 
service or the TabRestore service has updates, the JumpList being an 
observer of them will start an update. 

This update consists of the following steps: 
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons 
   for these URLs; 
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon 
   folders and notify the shell. 

Currently, there's a delay of 3500 ms before each RunUpdateJumpList 
call so that other RunUpdateJumpList calls requested in this period 
will be collapsed into one which can prevent update storms. This says 
that we are coalescing step 2 of jumplist updates which are close to 
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which 
makes some of them completely wasted. Therefore, we should apply this 
"delay and coalesce" strategy to step 1 as well. 

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing 
more URLs (typically 24) than needed (typically between 5 to 9). The 
work of creating ShellLinkItems and fetching favicons for these URLs 
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's 
very important to optimize them which otherwise can possibly hang 
Chrome.

BUG=40407, 179576, 717236

Review-Url: https://codereview.chromium.org/2859693002
Cr-Commit-Position: refs/heads/master@{#470091}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c0c398442ed5dc4d3c52f735b57...
==========

[commit-bot: I haz the power, 2017-05-08 19:58:03]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/9c0c398442ed5dc4d3c52f735b57...