Convert JumpList to a KeyedService.

Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile,
avoiding multiple jumplist updates.

BUG=40407
Committed: https://crrev.com/98b794500118af1b0e098f88854da71b6bd4aba2
Cr-Commit-Position: refs/heads/master@{#418276}

[Ilya Kulshin, 2016-09-08 03:33:25]

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

[commit-bot: I haz the power, 2016-09-08 03:33:57]

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, 2016-09-08 04:48:49]

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

[commit-bot: I haz the power, 2016-09-08 04:48:49]

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

[Ilya Kulshin, 2016-09-08 21:18:22]

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

[commit-bot: I haz the power, 2016-09-08 21:19:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Kulshin, 2016-09-08 22:36:20]

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

[commit-bot: I haz the power, 2016-09-08 22:37:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Kulshin, 2016-09-08 23:08:27]

Description was changed from

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile, avoiding multiple
jumplist updates.


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

to

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile, avoiding multiple
jumplist updates.


BUG=40407
==========

[Ilya Kulshin, 2016-09-08 23:14:30]

Description was changed from

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile, avoiding multiple
jumplist updates.


BUG=40407
==========

to

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile,
avoiding multiple jumplist updates.

BUG=40407
==========

[Ilya Kulshin, 2016-09-08 23:15:35]

kulshin@chromium.org changed reviewers:
+ gab@chromium.org, sky@chromium.org

[Ilya Kulshin, 2016-09-08 23:15:35]

ptal

gab@ - chrome/browser/win/*
sky@ - browser_view.cc and BUILD.gn

[sky, 2016-09-08 23:48:08]

LGTM

https://codereview.chromium.org/2323603002/diff/2/chrome/browser/win/jumplist...
File chrome/browser/win/jumplist_factory.cc (right):

https://codereview.chromium.org/2323603002/diff/2/chrome/browser/win/jumplist...
chrome/browser/win/jumplist_factory.cc:6: #include
"chrome/browser/win/jumplist_factory.h"
Style guide says this should be your first include, newline, then rest.

[Ilya Kulshin, 2016-09-09 00:02:05]

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

[commit-bot: I haz the power, 2016-09-09 00:02:42]

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, 2016-09-09 00:06:31]

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

[commit-bot: I haz the power, 2016-09-09 00:06:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Kulshin, 2016-09-09 00:41:25]

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

[commit-bot: I haz the power, 2016-09-09 00:42:09]

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, 2016-09-09 00:57:30]

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

[commit-bot: I haz the power, 2016-09-09 00:57:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-09-09 19:02:30]

Awesome! So this will fix https://crbug.com/40407#c80 "there is still a bug
where the entire jumplist icon writing process is repeated once for every
top-level Chrome window that is open.", right?! :-)

lgtm w/ comments, thanks!

PS: I'm not an owner in chrome/browser/win/* so sky still has to approve the
whole thing, not just the files initially assigned to him for review.

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

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:276: sessions::TabRestoreService*
tab_restore_service =
Add dependency with TabRestoreService

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:286: TopSitesFactory::GetForProfile(profile_);
Add dependency with TopSites service.

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:487:
FaviconServiceFactory::GetForProfile(profile_,
Add dependency on FaviconService

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
File chrome/browser/win/jumplist_factory.cc (right):

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist_factory.cc:24:
BrowserContextDependencyManager::GetInstance()) {}
Dependencies are declared here in constructor through the DependsOn() method
inherited from KeyedServiceBaseFactory:
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist_factory.cc:30: return new
JumpList(static_cast<Profile*>(context));
Profile::FromBrowserContext() instead of static_cast.

[Ilya Kulshin, 2016-09-12 23:50:11]

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

[commit-bot: I haz the power, 2016-09-12 23:50:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Kulshin, 2016-09-13 00:01:01]

Yes, this will fix the bug where the jumplist is written once per top level
window. The jumplist code will still get a notification for every top level
window, but because they will now use the same object the notifications will get
batched up into a single update. I may at some point come back and see if we can
eliminate multiple notifications, but that isn't the expensive part.

Re: owners: gab@ is listed as an owner in chrome/browser/win/OWNERS - are you ok
with signing off with that in mind? Can one of you please confirm owners
approval for the chrome/browser/win/* changes?

https://codereview.chromium.org/2323603002/diff/2/chrome/browser/win/jumplist...
File chrome/browser/win/jumplist_factory.cc (right):

https://codereview.chromium.org/2323603002/diff/2/chrome/browser/win/jumplist...
chrome/browser/win/jumplist_factory.cc:6: #include
"chrome/browser/win/jumplist_factory.h"
On 2016/09/08 23:48:08, sky wrote:
> Style guide says this should be your first include, newline, then rest.

Done.

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

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:276: sessions::TabRestoreService*
tab_restore_service =
On 2016/09/09 19:02:30, gab wrote:
> Add dependency with TabRestoreService

Done.

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:286: TopSitesFactory::GetForProfile(profile_);
On 2016/09/09 19:02:30, gab wrote:
> Add dependency with TopSites service.

Done.

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist.cc:487:
FaviconServiceFactory::GetForProfile(profile_,
On 2016/09/09 19:02:30, gab wrote:
> Add dependency on FaviconService

Done.

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
File chrome/browser/win/jumplist_factory.cc (right):

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist_factory.cc:24:
BrowserContextDependencyManager::GetInstance()) {}
On 2016/09/09 19:02:30, gab wrote:
> Dependencies are declared here in constructor through the DependsOn() method
> inherited from KeyedServiceBaseFactory:
>
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

Done.

https://codereview.chromium.org/2323603002/diff/90001/chrome/browser/win/jump...
chrome/browser/win/jumplist_factory.cc:30: return new
JumpList(static_cast<Profile*>(context));
On 2016/09/09 19:02:30, gab wrote:
> Profile::FromBrowserContext() instead of static_cast.

Done.

[commit-bot: I haz the power, 2016-09-13 01:02:41]

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

[commit-bot: I haz the power, 2016-09-13 01:02:42]

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

[Ilya Kulshin, 2016-09-13 02:36:09]

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

[commit-bot: I haz the power, 2016-09-13 02:36:38]

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, 2016-09-13 03:51:52]

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

[commit-bot: I haz the power, 2016-09-13 03:51:54]

Dry run: This issue passed the CQ dry run.

[gab, 2016-09-13 14:54:39]

On 2016/09/13 00:01:01, Ilya Kulshin wrote:
> Yes, this will fix the bug where the jumplist is written once per top level
> window. The jumplist code will still get a notification for every top level
> window, but because they will now use the same object the notifications will
get
> batched up into a single update. I may at some point come back and see if we
can
> eliminate multiple notifications, but that isn't the expensive part.
> 
> Re: owners: gab@ is listed as an owner in chrome/browser/win/OWNERS - are you
ok
> with signing off with that in mind? Can one of you please confirm owners
> approval for the chrome/browser/win/* changes?

Ah, interesting, looks like I was added as an OWNER there without my knowledge
(http://crrev.com/396184) but that's okay.

OWNER lgtm then :-), thanks!

[Ilya Kulshin, 2016-09-13 16:56:51]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2016-09-13 16:56:51]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2323603002/#ps130001
(title: "Clang fixes")

[commit-bot: I haz the power, 2016-09-13 16:57:07]

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, 2016-09-13 17:03:13]

Description was changed from

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile,
avoiding multiple jumplist updates.

BUG=40407
==========

to

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile,
avoiding multiple jumplist updates.

BUG=40407
==========

[commit-bot: I haz the power, 2016-09-13 17:03:14]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-09-13 17:05:10]

Description was changed from

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile,
avoiding multiple jumplist updates.

BUG=40407
==========

to

==========
Convert JumpList to a KeyedService.

This ensures that exactly one JumpList exists per profile,
avoiding multiple jumplist updates.

BUG=40407

Committed: https://crrev.com/98b794500118af1b0e098f88854da71b6bd4aba2
Cr-Commit-Position: refs/heads/master@{#418276}
==========

[commit-bot: I haz the power, 2016-09-13 17:05:14]

Patchset 8 (id:??) landed as
https://crrev.com/98b794500118af1b0e098f88854da71b6bd4aba2
Cr-Commit-Position: refs/heads/master@{#418276}