Cache JumpList icons to avoid unnecessary creation and deletion

Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

Note that some sites change the favicon to indicate status. Since 
caching is done based on the URL rather than the image itself, the 
jumplist won't pick up these stats updates. Given the fact that this 
cache mechanism improves the performance nicely, this is an okay 
compromise. 

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
Review-Url: https://codereview.chromium.org/2859193005
Cr-Commit-Position: refs/heads/master@{#473078}
Committed: https://chromium.googlesource.com/chromium/src/+/8127957d51be9220f6d5f64c637cd3a9d5566ea8

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

Description was changed from

==========
Change to JumpList class member functions so that they can access class member
variables

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

to

==========
Cache JumpList icons to remove redundant creation and deletion

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

[chengx, 2017-05-05 19:02:29]

Patchset #3 (id:40001) has been deleted

[chengx, 2017-05-05 22:04:43]

Patchset #3 (id:60001) has been deleted

[chengx, 2017-05-05 23:27:04]

Patchset #2 (id:20001) has been deleted

[chengx, 2017-05-05 23:27:09]

Patchset #2 (id:60002) has been deleted

[chengx, 2017-05-05 23:30:57]

Patchset #2 (id:90001) has been deleted

[chengx, 2017-05-05 23:35:22]

Patchset #2 (id:110001) has been deleted

[chengx, 2017-05-05 23:51:35]

Patchset #2 (id:130001) has been deleted

[chengx, 2017-05-05 23:59:01]

Description was changed from

==========
Cache JumpList icons to remove redundant creation and deletion

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

to

==========
Cache JumpList icons to avoid duplicated creation and deletion

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 00:35:20]

Description was changed from

==========
Cache JumpList icons to avoid duplicated creation and deletion

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current implementation, anytime when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there a (big) overlap between 
the new and old icons. This means that we're deleting a lot of icons 
and then create exactly the same ones, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, we should create one 
icon (if it's not existed) only for this tab and delete the least 
recent one, rather than deleting all 3 existing icons and creating 3 
new ones.

To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are now able to avoid deleting and 
creating the same icons.

The JumpList icon folders will only be cleaned entirely in the 
following situations:
1) "Most visited" category updates for the 1st time after Chrome is 
launched. This actually happens right after Chrome is launched.
2) "Recently closed" category updates for the 1st time after Chrome is launched.
This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folder has exceeded the 
limit, which is currently set to 12 for both categories. This is very 
important as it prevents the jumplist folder from being occupied 
endlessly.

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 00:35:30]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current implementation, anytime when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there a (big) overlap between 
the new and old icons. This means that we're deleting a lot of icons 
and then create exactly the same ones, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, we should create one 
icon (if it's not existed) only for this tab and delete the least 
recent one, rather than deleting all 3 existing icons and creating 3 
new ones.

To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are now able to avoid deleting and 
creating the same icons.

The JumpList icon folders will only be cleaned entirely in the 
following situations:
1) "Most visited" category updates for the 1st time after Chrome is 
launched. This actually happens right after Chrome is launched.
2) "Recently closed" category updates for the 1st time after Chrome is launched.
This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folder has exceeded the 
limit, which is currently set to 12 for both categories. This is very 
important as it prevents the jumplist folder from being occupied 
endlessly.

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current implementation, anytime when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there a (big) overlap between 
the new and old icons. This means that we're deleting a lot of icons 
and then create exactly the same ones, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, we should create one 
icon (if it's not existed) only for this tab and delete the least 
recent one, rather than deleting all 3 existing icons and creating 3 
new ones.

To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are now able to avoid deleting and 
creating the same icons.

The JumpList icon folders will only be cleaned entirely in the 
following situations:
1) "Most visited" category updates for the 1st time after Chrome is 
launched. This actually happens right after Chrome is launched.
2) "Recently closed" category updates for the 1st time after Chrome is 
launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folder has exceeded the 
limit, which is currently set to 12 for both categories. This is very 
important as it prevents the jumplist folder from being occupied 
endlessly.

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 00:52:15]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current implementation, anytime when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there a (big) overlap between 
the new and old icons. This means that we're deleting a lot of icons 
and then create exactly the same ones, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, we should create one 
icon (if it's not existed) only for this tab and delete the least 
recent one, rather than deleting all 3 existing icons and creating 3 
new ones.

To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are now able to avoid deleting and 
creating the same icons.

The JumpList icon folders will only be cleaned entirely in the 
following situations:
1) "Most visited" category updates for the 1st time after Chrome is 
launched. This actually happens right after Chrome is launched.
2) "Recently closed" category updates for the 1st time after Chrome is 
launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folder has exceeded the 
limit, which is currently set to 12 for both categories. This is very 
important as it prevents the jumplist folder from being occupied 
endlessly.

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this category is
updated. Currently, we delete all 3 existing icons and then create 3 new ones.
However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 00:52:32]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this category is
updated. Currently, we delete all 3 existing icons and then create 3 new ones.
However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and then create 3
new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 00:52:48]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and then create 3
new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 01:03:55]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these two hash maps, which are member variables of 
the JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. Patch set 1 does this change. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 01:04:08]

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

[commit-bot: I haz the power, 2017-05-06 01:04:41]

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-06 01:08:09]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then create exactly the same ones, which is very 
inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the 1st time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the 1st time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these two hash maps, which are member variables of 
the JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. Patch set 1 does this change. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

BUG=40407, 179576, 715902
==========

[chengx, 2017-05-06 01:19:23]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

BUG=40407, 179576, 715902
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[UI-wise]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather than all
the old icons, we'll still have most of the old icons if the 
OS notification step fails, which clearly looks much better.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-06 01:19:47]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[UI-wise]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather than all
the old icons, we'll still have most of the old icons if the 
OS notification step fails, which clearly looks much better.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[UI-wise]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which clearly looks much better.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-06 01:21:04]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[UI-wise]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which clearly looks much better.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[UI-wise]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-06 01:34:38]

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

[commit-bot: I haz the power, 2017-05-06 01:35:15]

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-06 02:12:01]

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

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

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-06 02:16:56]

Patchset #3 (id:170001) has been deleted

[chengx, 2017-05-06 02:17:35]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[UI-wise]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-06 02:26:52]

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

[chengx, 2017-05-06 02:26:53]

Hi Greg, finally I implemented the icon caching idea. PTAL, thanks!

Since I need to change some functions to JumpList class member functions,
there're quite some changes due to relocation of these functions. I've used
patch set 1 for this piece of change in particular.  

Then for the real code change for implementing the icon caching idea, please
using "delta between patch set 2 from 1". 

The CL description has more information about this.

[chengx, 2017-05-06 18:40:35]

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

[commit-bot: I haz the power, 2017-05-06 18:41:15]

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-06 18:43:01]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to understand how 
this is implemented.

Patch set 3 updates the metric WinJumplist.CreateIconFilesCount 
accordingly. The code diff tool does a poor job when diff this patch 
set to the code repository due to the various changes. Using "Delta 
from patch set 2" makes it easier to understand how this metric is 
updated. 

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to see how this is 
implemented.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-06 18:43:41]

Patchset #3 (id:190001) has been deleted

[chengx, 2017-05-06 18:43:49]

Patchset #2 (id:150001) has been deleted

[commit-bot: I haz the power, 2017-05-06 19:19:26]

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

[commit-bot: I haz the power, 2017-05-06 19:19:27]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-07 22:00:23]

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

[commit-bot: I haz the power, 2017-05-07 22:00:38]

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-07 22:03:57]

Patchset #3 (id:220001) has been deleted

[chengx, 2017-05-07 22:34:27]

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

[commit-bot: I haz the power, 2017-05-07 22:34:35]

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-07 23:14:00]

Patchset #2 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-05-07 23:14:55]

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

[commit-bot: I haz the power, 2017-05-07 23:14:56]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-08 03:57:09]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are 
created and written into the corresponding folder after all old icons 
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting 
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there're usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this 
category is updated. Currently, we delete all 3 existing icons and 
then create 3 new ones. However, the most efficient way is to create 
one icon (if it doesn't exist) only for this closed tab and delete the 
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the 
help of these hash maps, we are able to avoid deleting and creating 
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to see how this is 
implemented.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old 
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to see how this is 
implemented.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[grt (UTC plus 2), 2017-05-08 09:40:59]

hi xi. can you wait a few days for a full review of this?

https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
most_visited_map_;
flat_map may be a better choice here; see base/containers/README.md.

https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
most_visited_map_;
fyi: std::map isn't a hash map

[chengx, 2017-05-08 16:25:05]

On 2017/05/08 09:40:59, grt (UTC plus 2) wrote:
> hi xi. can you wait a few days for a full review of this?
> 
>
https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
> File chrome/browser/win/jumplist.h (right):
> 
>
https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
> chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
> most_visited_map_;
> flat_map may be a better choice here; see base/containers/README.md.
> 
>
https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
> chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
> most_visited_map_;
> fyi: std::map isn't a hash map

Hi Greg, 

Sure, take your time. 

I'll talk about this jumplist bug fix in the Chrome-Windows team meeting next
Wednesday. It would be cool if I can land this CL and get Canary data for 1 day
before that. If we cannot make it, that's okay too.

[chengx, 2017-05-10 20:54:21]

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

[commit-bot: I haz the power, 2017-05-10 20:55:37]

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-10 21:08:18]

Patchset #3 (id:260001) has been deleted

[chengx, 2017-05-10 21:08:25]

Patchset #3 (id:280001) has been deleted

[chengx, 2017-05-10 21:09:26]

Patchset #1 (id:1) has been deleted

[chengx, 2017-05-10 21:10:02]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to see how this is 
implemented.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-10 21:10:26]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

Code-wise, since these hash maps, which are member variables of the 
JumpList class, need to be accessed when icons are deleted and 
created, some methods need to be changed to member functions of the 
JumpList class. This change's in patch set 1. There's no functionality 
change for JumpList in this patch set.

The implementation of this icon caching strategy is in patch set 2. 
Using "Delta from patch set 1" makes it much easier to see how this is 
implemented.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-10 21:15:01]

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

[commit-bot: I haz the power, 2017-05-10 21:16:44]

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-10 22:03:59]

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

[commit-bot: I haz the power, 2017-05-10 22:04: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-10 22:05:42]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for both categories. This is 
   very important as it prevents the jumplist folders from getting
   bloated endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-10 22:06:22]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two hash maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-10 22:06:59]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories, respectively. With the
help of these hash maps, we are able to avoid deleting and creating
the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-10 22:07:37]

Patchset #2 (id:300001) has been deleted

[chengx, 2017-05-10 22:13:12]

Patchset #2 (id:320001) has been deleted

[chengx, 2017-05-10 22:13:22]

Patchset #2 (id:340001) has been deleted

[chengx, 2017-05-10 22:21:35]

Hi Greg, 

I've landed crrev.com/2870853002/ to just change 4 functions to JumpList class
member functions, which was part of this CL. Therefore, you can review this CL
directly from patch set 2 now.

Take your time. It's not urgent.

https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
most_visited_map_;
On 2017/05/08 09:40:59, grt (UTC plus 2) wrote:
> flat_map may be a better choice here; see base/containers/README.md.

Thanks for pointing me to this.

https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
most_visited_map_;
On 2017/05/08 09:40:59, grt (UTC plus 2) wrote:
> fyi: std::map isn't a hash map

Ah, I was trying to use std::unordered_map. Sorry I messed up with these two
maps. Anyway, I've changed it to flat_map. Thanks!

[chengx, 2017-05-11 04:45:22]

Patchset #1 (id:240001) has been deleted

[chengx, 2017-05-11 05:03:56]

After a second thought, I decided to delete all previous patch sets to avoid any
possible confusion. I've address your previous comment to use base::flat_map
instead of std::map. The previous email related to this CL has all my replies.
Thanks!

On 2017/05/10 22:21:35, chengx wrote:
> Hi Greg, 
> 
> I've landed crrev.com/2870853002/ to just change 4 functions to JumpList class
> member functions, which was part of this CL. Therefore, you can review this CL
> directly from patch set 2 now.
> 
> Take your time. It's not urgent.
> 
>
https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
> File chrome/browser/win/jumplist.h (right):
> 
>
https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
> chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
> most_visited_map_;
> On 2017/05/08 09:40:59, grt (UTC plus 2) wrote:
> > flat_map may be a better choice here; see base/containers/README.md.
> 
> Thanks for pointing me to this.
> 
>
https://codereview.chromium.org/2859193005/diff/240001/chrome/browser/win/jum...
> chrome/browser/win/jumplist.h:239: std::map<std::string, base::FilePath>
> most_visited_map_;
> On 2017/05/08 09:40:59, grt (UTC plus 2) wrote:
> > fyi: std::map isn't a hash map
> 
> Ah, I was trying to use std::unordered_map. Sorry I messed up with these two
> maps. Anyway, I've changed it to flat_map. Thanks!

[grt (UTC plus 2), 2017-05-11 20:29:13]

I love the level of detail in the CL description. One suggestion: consider that
the reader will be some future developer curious about the CL. While it's
totally clear to me now that "In the current JumpList implementation..." means
"In the JumpList implementation preceding this CL...", I think it will be more
clear to future-grt if this said simply "Previously, ...".

As for the impl, it looks like this fetches all favicons from the history db and
then drops/ignores the ones with the same urls as ones that are cached. Is it
possible to avoid the round-trip to the history service for these urls?

One thought I had was that some sites change the favicon to indicate status.
Since caching is done based on the URL rather than based on the image itself,
the jumplist won't pick up these stats updates. I think this is an okay
compromise. If I'm not mistaken about this, it's probably worth mentioning this
in the CL description.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:573: std::set<base::FilePath> iconpath_set;
iconpath_set -> cached_files here, too

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:575: if (category ==
JumpListCategory::MOST_VISITED) {
this is a great place for a case statement. if you do this:
  switch (category) {
    case JumpListCategory::kMostVisited:
      ...
      break;
    case JumpListCategory::kRecentlyClosed:
      ...
      break;
  }
then the code is protected against a change 3 years from now when someone adds a
third category -- they'll get a compile failure here rather than silently end up
with an empty set for the delete call.

you can also get rid of the code duplication below with:

  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
  switch (category) {
    case JumpListCategory::kMostVisited:
      source_map = &most_visited_map_;
      break;
    case JumpListCategory::kRecentlyClosed:
      source_map = &recently_closed_map_;
      break;
  }
  ...pull from source_map here...

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:576: for (auto url_path_pair : most_visited_map_)
const auto& url_path_pair

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:577: iconpath_set.insert(url_path_pair.second);
flat_set seems like a good choice here -- you can reserve() exactly the space
needed and then cached_files.insert(url_path_pair.second, cached_files.end()) to
build the set in O(N).

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:579: for (auto url_path_pair :
recently_closed_map_)
const auto&

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:596: if (category ==
JumpListCategory::MOST_VISITED) {
please use the technique above to collapse the duplicated code blocks here

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:36: enum JumpListCategory { MOST_VISITED = 0,
RECENTLY_CLOSED };
nit: "enum class"

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:36: enum JumpListCategory { MOST_VISITED = 0,
RECENTLY_CLOSED };
nit: Chromium no longer deviates from the Google style for enumerator names, so
please make these kLikeAConstant rather than LIKE_A_MACRO
(https://google.github.io/styleguide/cppguide.html#Enumerator_Names).

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:36: enum JumpListCategory { MOST_VISITED = 0,
RECENTLY_CLOSED };
move this type into the "private:" section of the class.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_file_util.h (right):

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist_file_util.h:50: const std::set<base::FilePath>&
iconpath_set);
nit: iconpath_set -> cached_files since these are the files not deleted (the
"cached files")

[chengx, 2017-05-11 21:45:16]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

In the current JumpList implementation, when a JumpList category 
("Mostly visited" or "Recently closed") is updated, new icons are
created and written into the corresponding folder after all old icons
in the same folder are deleted. However, there's usually a (big) 
overlap between the new and old icons. This means that we're deleting
a lot of icons and then creating exactly the same ones in each update, 
which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-11 23:29:18]

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

[commit-bot: I haz the power, 2017-05-11 23:30:13]

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-11 23:36:53]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Currently, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-11 23:44:18]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

Note that some sites change the favicon to indicate status. Since 
caching is done based on the URL rather than the image itself, the 
jumplist won't pick up these stats updates. Given the fact that this 
cache mechanism improves the performance nicely, this is an okay 
compromise. 

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

[chengx, 2017-05-12 00:00:16]

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

[commit-bot: I haz the power, 2017-05-12 00:01:28]

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-12 00:37:52]

Patchset #2 (id:380001) has been deleted

[chengx, 2017-05-12 01:04:25]

Thanks a lot for the feedback! I've addressed all the comments in new patch set.
PTAL~

On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> I love the level of detail in the CL description. One suggestion: consider
that
> the reader will be some future developer curious about the CL. While it's
> totally clear to me now that "In the current JumpList implementation..." means
> "In the JumpList implementation preceding this CL...", I think it will be more
> clear to future-grt if this said simply "Previously, ...".
> 

I've made the change in the CL description according to your suggestion. Thanks!

> As for the impl, it looks like this fetches all favicons from the history db
and
> then drops/ignores the ones with the same urls as ones that are cached. Is it
> possible to avoid the round-trip to the history service for these urls?

This is actually the first CL I am going to write after this one is landed. So
it's entirely possible. Since I've filed a number of bugs for optimizing
different sections of jumplist code, I feel like it's better to break a big CL
into a few smaller ones, so each smaller one can go to its dedicated bug rather
than a big CL goes to all the bugs. In short, I'll use another CL for this.

> One thought I had was that some sites change the favicon to indicate status.
> Since caching is done based on the URL rather than based on the image itself,
> the jumplist won't pick up these stats updates. I think this is an okay
> compromise. If I'm not mistaken about this, it's probably worth mentioning
this
> in the CL description.

I've added this to the CL description. Thanks.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:573: std::set<base::FilePath> iconpath_set;
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> iconpath_set -> cached_files here, too

Done.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:575: if (category ==
JumpListCategory::MOST_VISITED) {
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> this is a great place for a case statement. if you do this:
>   switch (category) {
>     case JumpListCategory::kMostVisited:
>       ...
>       break;
>     case JumpListCategory::kRecentlyClosed:
>       ...
>       break;
>   }
> then the code is protected against a change 3 years from now when someone adds
a
> third category -- they'll get a compile failure here rather than silently end
up
> with an empty set for the delete call.
> 
> you can also get rid of the code duplication below with:
> 
>   base::flat_map<std::string, base::FilePath>* source_map = nullptr;
>   switch (category) {
>     case JumpListCategory::kMostVisited:
>       source_map = &most_visited_map_;
>       break;
>     case JumpListCategory::kRecentlyClosed:
>       source_map = &recently_closed_map_;
>       break;
>   }
>   ...pull from source_map here...

Done. Thanks so so so much for this suggestion!

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:576: for (auto url_path_pair : most_visited_map_)
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> const auto& url_path_pair

Done.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:577: iconpath_set.insert(url_path_pair.second);
On 2017/05/11 20:29:12, grt (UTC plus 2) wrote:
> flat_set seems like a good choice here -- you can reserve() exactly the space
> needed and then cached_files.insert(url_path_pair.second, cached_files.end())
to
> build the set in O(N).

Thanks. I've changed to flat_set. 
fyi, it should be the position to insert as the first argument, the the value to
insert, i.e.,
cached_files.insert(cached_files.end(), url_path_pair.second)

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:579: for (auto url_path_pair :
recently_closed_map_)
On 2017/05/11 20:29:12, grt (UTC plus 2) wrote:
> const auto&

Done.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:596: if (category ==
JumpListCategory::MOST_VISITED) {
On 2017/05/11 20:29:12, grt (UTC plus 2) wrote:
> please use the technique above to collapse the duplicated code blocks here

Done. Again, thanks so much for this suggestion!

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:36: enum JumpListCategory { MOST_VISITED = 0,
RECENTLY_CLOSED };
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> nit: Chromium no longer deviates from the Google style for enumerator names,
so
> please make these kLikeAConstant rather than LIKE_A_MACRO
> (https://google.github.io/styleguide/cppguide.html#Enumerator_Names).

Done with changing names to kLikeAConstant alike. Thanks for pointing me to
this.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:36: enum JumpListCategory { MOST_VISITED = 0,
RECENTLY_CLOSED };
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> move this type into the "private:" section of the class.

Done with moving to private section of the class.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:36: enum JumpListCategory { MOST_VISITED = 0,
RECENTLY_CLOSED };
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> nit: "enum class"

Done. Using "enum class" now.

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_file_util.h (right):

https://codereview.chromium.org/2859193005/diff/360001/chrome/browser/win/jum...
chrome/browser/win/jumplist_file_util.h:50: const std::set<base::FilePath>&
iconpath_set);
On 2017/05/11 20:29:13, grt (UTC plus 2) wrote:
> nit: iconpath_set -> cached_files since these are the files not deleted (the
> "cached files")

Done.

[chengx, 2017-05-12 01:07:51]

Patchset #3 (id:420001) has been deleted

[chengx, 2017-05-12 01:09:10]

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

[commit-bot: I haz the power, 2017-05-12 01:10:17]

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-12 02:07:55]

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

[commit-bot: I haz the power, 2017-05-12 02:07:56]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-05-15 11:28:19]

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:584: if (!source_map)
remove this check -- it can't be hit under normal circumstances, and a crash
down below is preferable to silently proceeding under crazy circumstances.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:592: cached_files.insert(cached_files.end(),
url_path_pair.second);
oops, i was wrong about this being O(N) -- that would only be the case if
source_map was sorted on path, which it isn't. you can go two ways here: 1) add
a little more complexity by populating a vector then constructing the set from
the vector (this is optimal according to base/containers/README.md), or 2)
figure that the container is small enough and paths are movable so the simple
insert is good enough. i would go with the latter, so just remove the hint and
let insert to its thing.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:604: // If the current URL is cached which means
there's already an icon for this
maybe this can be simplified by removing the idea of a cache:

"Reuse icons for urls that were already present in the jumplist for this
category."

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:618: if (!source_map)
remove as above

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:625: if (source_map->find((*item)->url()) !=
source_map->end()) {
nit: (*item)-> everywhere is a bit ugly. how about changing |item| in the loop
to |iter| and introducing:
    ShellLinkItem* item = iter->get();

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:626: base::FilePath path =
(*source_map)[(*item)->url()];
rather than indexing into the map a second time, capture the result of find()
above and use it here:
    auto cache_iter = source_map->find(item->url());
    if (cache_iter != source_map->end()) {
      item->set_icon(cache_iter->second.value(), 0);
      updated_map[item->url()] = cache_iter->second.value();
    } else {
      ...
    }

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:631: if (CreateIconFile((*item)->icon_image(),
icon_dir, &icon_path)) {
rather than creating new icons as you go here and then deleting old ones
sometime later, how about:
- figure out which items in item_list need new icons and which entries in
source_map are to be retained (add them to updated_map)
- delete files on disk that are not represented in updated_map
- create new icons and add them to updated_map

will this help prevent another file explosion in the future?

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:665: CreateIconFiles(icon_dir, page_list,
slot_limit, category);
should this check that the max # of files isn't being exceeded? if no, could
this potentially lead to the dir-with-too-many-files problem again?

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:132: enum class JumpListCategory { kMostVisited =
0, kRecentlyClosed };
nit: omit " = 0"

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:185: // Deletes icon files in |icon_dir| which are
not in the cache anymore.
nit: mention |category| somehow here and in comments below.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:248: // Cache of the mappings from JumpList items'
URLs to icon file paths for
please document which task runner(s) may access these members. it looks like all
accesses must be on the update_jumplist_task_runner_. is this correct?

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:250: base::flat_map<std::string, base::FilePath>
most_visited_map_;
based on how this is used, i think an accurate description is:

// The icon file paths of the most visited links in the current jumplist,
indexed by tab url.

naming nit: "map" in the name isn't really needed. how about most_visited_icons_
or something like that?

[chengx, 2017-05-15 17:07:34]

Patchset #2 (id:400001) has been deleted

[chengx, 2017-05-17 21:52:14]

Patchset #4 (id:480001) has been deleted

[chengx, 2017-05-17 21:59:36]

Hi Greg, I've addressed all the comments in the new patch set. PTAL, thanks!

I was travelling and busy preparing for the presentation, so adding this patch
set got delayed.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:584: if (!source_map)
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> remove this check -- it can't be hit under normal circumstances, and a crash
> down below is preferable to silently proceeding under crazy circumstances.

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:592: cached_files.insert(cached_files.end(),
url_path_pair.second);
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> oops, i was wrong about this being O(N) -- that would only be the case if
> source_map was sorted on path, which it isn't. you can go two ways here: 1)
add
> a little more complexity by populating a vector then constructing the set from
> the vector (this is optimal according to base/containers/README.md), or 2)
> figure that the container is small enough and paths are movable so the simple
> insert is good enough. i would go with the latter, so just remove the hint and
> let insert to its thing.

I would go with the latter one too, as the size of these sets should be either 3
or 5, which is small enough.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:604: // If the current URL is cached which means
there's already an icon for this
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> maybe this can be simplified by removing the idea of a cache:
> 
> "Reuse icons for urls that were already present in the jumplist for this
> category."

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:618: if (!source_map)
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> remove as above

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:625: if (source_map->find((*item)->url()) !=
source_map->end()) {
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> nit: (*item)-> everywhere is a bit ugly. how about changing |item| in the loop
> to |iter| and introducing:
>     ShellLinkItem* item = iter->get();

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:626: base::FilePath path =
(*source_map)[(*item)->url()];
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> rather than indexing into the map a second time, capture the result of find()
> above and use it here:
>     auto cache_iter = source_map->find(item->url());
>     if (cache_iter != source_map->end()) {
>       item->set_icon(cache_iter->second.value(), 0);
>       updated_map[item->url()] = cache_iter->second.value();
>     } else {
>       ...
>     }

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:631: if (CreateIconFile((*item)->icon_image(),
icon_dir, &icon_path)) {
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> rather than creating new icons as you go here and then deleting old ones
> sometime later, how about:
> - figure out which items in item_list need new icons and which entries in
> source_map are to be retained (add them to updated_map)
> - delete files on disk that are not represented in updated_map
> - create new icons and add them to updated_map
> 
> will this help prevent another file explosion in the future?

I understand what you suggest here, but IMHO it doesn't make a difference from
the current implementation where deletion comes after creation. The
UpdateIconFiles calls FilesExceedLimitInDir() in L657 before CreateIconFiles()
and DeleteIconFiles() to make sure the jumplisticon folders are healthy before
any operations. So file explosion won't happen in the current implementation.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:665: CreateIconFiles(icon_dir, page_list,
slot_limit, category);
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> should this check that the max # of files isn't being exceeded? if no, could
> this potentially lead to the dir-with-too-many-files problem again?

No, this won't cause the dir-with-too-many-files problem.
There's a FilesExceedLimitInDir(icon_dir, icon_limit) check above in L657. If
the file limit is reached, this "else if" section of code won't be hit.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:132: enum class JumpListCategory { kMostVisited =
0, kRecentlyClosed };
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> nit: omit " = 0"

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:185: // Deletes icon files in |icon_dir| which are
not in the cache anymore.
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> nit: mention |category| somehow here and in comments below.

Done.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:248: // Cache of the mappings from JumpList items'
URLs to icon file paths for
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> please document which task runner(s) may access these members. it looks like
all
> accesses must be on the update_jumplist_task_runner_. is this correct?

Documentation added. Yes, only update_jumplist_task_runner_ should have access
to them. The update_jumplist_task_runner_ is dealing with
JumpListIcons{MostVistied,RecentClosed} folders which are newly introduced. As
to delete_jumplisticons_task_runner_, it is used to delete the retired
JumpListIcons{,Old} folders. Once these two folders are gone for all Chrome
users, the task runner should be deleted.

https://codereview.chromium.org/2859193005/diff/440001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:250: base::flat_map<std::string, base::FilePath>
most_visited_map_;
On 2017/05/15 11:28:19, grt (UTC plus 2) wrote:
> based on how this is used, i think an accurate description is:
> 
> // The icon file paths of the most visited links in the current jumplist,
> indexed by tab url.
> 
> naming nit: "map" in the name isn't really needed. how about
most_visited_icons_
> or something like that?

Done. Thanks for the suggestion!

[chengx, 2017-05-17 22:06:29]

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

[commit-bot: I haz the power, 2017-05-17 22:07:50]

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-18 00:14:35]

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

[commit-bot: I haz the power, 2017-05-18 00:14:36]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-05-18 09:00:00]

lgtm w/ a question: do you want to add metrics to see how many icons are reused
on each update?

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:661: // Maximum number of icon files that each
JumpList icon folder allows to hold.
"allows to" -> "may"

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:255: // They should be only accessed by
update_jumplist_task_runner_.
nit: "They may only be accessed on update_jumplist_task_runner_."

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_updater.h (right):

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
chrome/browser/win/jumplist_updater.h:62: // The URL from which the favicon is
retrieved.
nit: is this the url of the favicon itself, or is it the url of the page to
which the favicon corresponds? if the latter, i propose:

The tab URL corresponding to this link's favicon.

[chengx, 2017-05-18 17:14:02]

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

[commit-bot: I haz the power, 2017-05-18 17:15:28]

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-18 17:21:56]

On 2017/05/18 09:00:00, grt (UTC plus 2) wrote:
> lgtm w/ a question: do you want to add metrics to see how many icons are
reused
> on each update?

Yes, I will. Actually I once added those metrics in this CL. However, since
quite a few functions need to be changed to return int rather than void, "Delta
from patch set" doesn't work well. To make the code changes in this CL easier to
track, I decided to use another CL for the metrics after this one is landed.
I'll also start anther CL to reduce the amount of favicons to fetch using the
icon cache.

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:661: // Maximum number of icon files that each
JumpList icon folder allows to hold.
On 2017/05/18 08:59:59, grt (UTC plus 2) wrote:
> "allows to" -> "may"

Done.

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.h (right):

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
chrome/browser/win/jumplist.h:255: // They should be only accessed by
update_jumplist_task_runner_.
On 2017/05/18 08:59:59, grt (UTC plus 2) wrote:
> nit: "They may only be accessed on update_jumplist_task_runner_."

Done.

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
File chrome/browser/win/jumplist_updater.h (right):

https://codereview.chromium.org/2859193005/diff/500001/chrome/browser/win/jum...
chrome/browser/win/jumplist_updater.h:62: // The URL from which the favicon is
retrieved.
On 2017/05/18 09:00:00, grt (UTC plus 2) wrote:
> nit: is this the url of the favicon itself, or is it the url of the page to
> which the favicon corresponds? if the latter, i propose:
> 
> The tab URL corresponding to this link's favicon.

Done. It's the latter one, i.e., the url of the page to which the favicon
corresponds.

[commit-bot: I haz the power, 2017-05-18 19:23:14]

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

[commit-bot: I haz the power, 2017-05-18 19:23:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chengx, 2017-05-18 19:23:46]

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

[commit-bot: I haz the power, 2017-05-18 19:25: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-18 21:25:42]

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

[commit-bot: I haz the power, 2017-05-18 21:27:19]

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-19 00:53:20]

Patchset #6 (id:530001) has been deleted

[chengx, 2017-05-19 00:53:36]

The CQ bit was unchecked by chengx@chromium.org

[chengx, 2017-05-19 00:53:41]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-05-19 00:53:41]

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/2859193005/#ps510007
(title: "Fix nits.")

[commit-bot: I haz the power, 2017-05-19 00:54:48]

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-19 04:05:18]

CQ is committing da patch.

Bot data: {"patchset_id": 510007, "attempt_start_ts": 1495155221133270,
"parent_rev": "f1a94f90a612655f8deedeab50df2522d718c9fc", "commit_rev":
"8127957d51be9220f6d5f64c637cd3a9d5566ea8"}

[commit-bot: I haz the power, 2017-05-19 04:05:32]

Description was changed from

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

Note that some sites change the favicon to indicate status. Since 
caching is done based on the URL rather than the image itself, the 
jumplist won't pick up these stats updates. Given the fact that this 
cache mechanism improves the performance nicely, this is an okay 
compromise. 

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115
==========

to

==========
Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently 
closed") is updated, new icons are created and written into the 
corresponding folder after all old icons in the same folder are 
deleted. However, there's usually a (big) overlap between the new and 
old icons. This means that we're deleting a lot of icons and then 
creating the same ones in each update, which is very inefficient. 

For example, JumpList can show at most 3 items for "Recently closed" 
category. Therefore, there are usually 3 icons existed for this 
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.
 
To fix this, we introduce two maps to cache the URL-IconFilePath 
information of the two JumpList categories. With the help of these 
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the 
following situations:
1) The "Most visited" category updates for the first time after Chrome 
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after 
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the 
   limit, which is currently set to 12 for "most visited" category and 
   6 for "recently closed" category, respectively. This is very 
   important as it prevents the jumplist folders from getting bloated 
   endlessly.

Note that some sites change the favicon to indicate status. Since 
caching is done based on the URL rather than the image itself, the 
jumplist won't pick up these stats updates. Given the fact that this 
cache mechanism improves the performance nicely, this is an okay 
compromise. 

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather 
than all the old icons, we'll still have most of the old icons if the 
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG=40407, 179576, 715902, 716115

Review-Url: https://codereview.chromium.org/2859193005
Cr-Commit-Position: refs/heads/master@{#473078}
Committed:
https://chromium.googlesource.com/chromium/src/+/8127957d51be9220f6d5f64c637c...
==========

[commit-bot: I haz the power, 2017-05-19 04:05:36]

Committed patchset #5 (id:510007) as
https://chromium.googlesource.com/chromium/src/+/8127957d51be9220f6d5f64c637c...