|
|
Created:
4 years, 3 months ago by Ilya Kulshin Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert 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}
Patch Set 1 #Patch Set 2 : Remove excess file #Patch Set 3 : Revert unintended change #Patch Set 4 : Uninline a function #Patch Set 5 : Remove notification observer and force destruction on UI thread #
Total comments: 2
Patch Set 6 : Fix includes #
Total comments: 10
Patch Set 7 : Add dependencies #Patch Set 8 : Clang fixes #
Messages
Total messages: 39 (31 generated)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/...)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 ==========
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 ==========
kulshin@chromium.org changed reviewers: + gab@chromium.org, sky@chromium.org
ptal gab@ - chrome/browser/win/* sky@ - browser_view.cc and BUILD.gn
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.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
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.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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!
The CQ bit was checked by kulshin@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/98b794500118af1b0e098f88854da71b6bd4aba2 Cr-Commit-Position: refs/heads/master@{#418276} |