Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769323002:
mash: Update shelf pin prefs in ShelfModelObserver overrides. (Closed)
Descriptionmash: Update shelf pin prefs in ShelfModelObserver overrides.
Do not update prefs in ChromeLauncherController's ShelfDelegate overrides.
Instead, update prefs in CLC's ShelfModelObserver overrides.
This will help move ShelfDelegate responsibilities out of Chrome.
Refactor do-not-sync flag for internal and ARC disabling support.
Do not sync the initial browser shortcut pin position (see comment).
Extract ItemTypeIsPinned helper for ChromeLauncherController.
Limit the crbug.com/654622 workaround to affected test fixtures.
(this workaround breaks some other fixtures like ArcMultiUser)
BUG=557406
TEST=Automated; no shelf item behavior changes.
R=khmel@chromium.org,jamescook@chromium.org
Review-Url: https://codereview.chromium.org/2769323002
Cr-Commit-Position: refs/heads/master@{#460658}
Committed: https://chromium.googlesource.com/chromium/src/+/ac32fbee0938078f84eea9524c1e6d99e1949f94
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : Debugging ARC... #Patch Set 4 : Add a hack for ARC behavior. #Patch Set 5 : Respect |ignore_persist_pinned_state_change_| and cleanup. #Patch Set 6 : Debugging... #Patch Set 7 : Rebase on https://codereview.chromium.org/2784613002 #Patch Set 8 : Cleanup; fix tests by ignoring initial browser shortcut pin position syncing. #
Total comments: 17
Patch Set 9 : Address comments; reduce the reach of our crbug.com/654622 workaround. #
Total comments: 10
Patch Set 10 : Address comments. #Patch Set 11 : Use ScopedPinSyncDisabler name suggestion. #
Messages
Total messages: 48 (34 generated)
Description was changed from ========== Update shelf pin preferences in ChromeLauncherController's observer overrides. BUG=557406 ========== to ========== mash: Update shelf pin prefs ShelfModelObserver overrides. BUG=557406 ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by msw@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== mash: Update shelf pin prefs ShelfModelObserver overrides. BUG=557406 ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. BUG=557406 ==========
Description was changed from ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. BUG=557406 ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. (instead, update prefs in CLC's ShelfModelObserver overrides) This will help move ShelfDelegate responsibilities out of Chrome. Pass a ShelfItem struct copy to ShelfModelObserver::ShelfItemRemoved. (lets observers check the state of items removed from the shelf model) Add LauncherAppUpdater::Delegate::OnAppDisabl[ing|ed] and CLC support. Avoid ChromeLauncherController removing pin positions when ARC is disabled. Remove ScheduleUpdateAppLaunchersFromPref (inline retry task posting). Extract ItemTypeIsPinned helper for ChromeLauncherController. BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ==========
msw@chromium.org changed reviewers: + khmel@chromium.org, sky@chromium.org
The CQ bit was checked by msw@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. (instead, update prefs in CLC's ShelfModelObserver overrides) This will help move ShelfDelegate responsibilities out of Chrome. Pass a ShelfItem struct copy to ShelfModelObserver::ShelfItemRemoved. (lets observers check the state of items removed from the shelf model) Add LauncherAppUpdater::Delegate::OnAppDisabl[ing|ed] and CLC support. Avoid ChromeLauncherController removing pin positions when ARC is disabled. Remove ScheduleUpdateAppLaunchersFromPref (inline retry task posting). Extract ItemTypeIsPinned helper for ChromeLauncherController. BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. (instead, update prefs in CLC's ShelfModelObserver overrides) This will help move ShelfDelegate responsibilities out of Chrome. Add LauncherAppUpdater::Delegate::OnAppDisabl[ing|ed] and CLC support. Avoid CLC removing sync model pin positions when ARC is disabled. Skip syncing the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ==========
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Patchset #8 (id:160001) has been deleted
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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
Hey Scott and Yury, please take a look; thanks! https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:934: // Avoid removing pin positions of ARC apps when ARC itself is disabled. This is unfortunate and I'm receptive to better approaches. We generally want CLC to remove items from the sync model when they are removed from the shelf model, but we can't do that when ARC is being disabled (and ARC apps are being removed from the ShelfModel), because we want those apps to remain in the sync model for other devices (and to restore if ARC is re-enabled). https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1277: // Do not sync the pin position of the browser shortcut item when it is added; This threw me for a loop... I tried other approaches in earlier patch sets (delaying SyncPinPosition if AppListSyncableService is not created/initialized), but everything I tried caused weird test failures (adding extensions later on would not update the sync model, etc.), even though that delay mechanism was not even triggered in that case! I seriously spent too long on this already, but if anyone has a better solution, I'm willing to try something else.
sky@chromium.org changed reviewers: + jamescook@chromium.org
I'm a bit at a loss for this one. Would James be a better reviewer than myself?
On 2017/03/29 03:09:15, sky wrote: > I'm a bit at a loss for this one. Would James be a better reviewer than myself? James, please take a look at your convenience, no rush. Yury, is there a better way to respect arc [app] disabling?
How's the test coverage for the pin/unpin stuff? https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:81: #include "extensions/browser/extension_system.h" Are these new includes needed? https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1379: if (ItemTypeIsPinned(item) && !ignore_persist_pinned_state_change_) SyncPinPosition() checks ignore_persist_pinned_state_change_. Do you need to do it in both places? It might be clearer to always do it outside the call, since RemovePinPosition can't check it. Or make a version of RemovePinPosition inside this class that checks. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1396: old_item.app_launch_id.launch_id()); Is it valid to recycle the launch_id when GetShelfAppIdFromArcAppId() might have changed the app_id? https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:316: std::set<std::string> keep_pin_positions_; Could this be handled with ignore_persistent_pinned_state_change_? If not, and this just exists for arc, either the name or the comment should say something about arc. Also, it seems like this only works for apps without a launch id, which probably depends on ARC never using a launch id. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_app_updater.h (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_app_updater.h:17: class LauncherAppUpdater { Aside: This class is weird. It seems to exist mostly to be a delegate interface, but I don't understand why the delegation happens. All the code is in chrome/browser/ui. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_arc_app_updater.cc (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_arc_app_updater.cc:29: // When disabling ARC, apps are removed, but sync items should not be deleted. I suggest explaining why the items should not be deleted, like it your CL comments. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_arc_app_updater.cc:37: delegate()->OnAppDisabled(app_id); It seems like these OnAppDisabling() etc. functions should also be called from LauncherExtensionAppUpdater, or these shouldn't be delegate methods and LauncherArcAppUpdater should be calling directly into CLC.
https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:316: std::set<std::string> keep_pin_positions_; On 2017/03/29 14:28:02, James Cook wrote: > Could this be handled with ignore_persistent_pinned_state_change_? > > If not, and this just exists for arc, either the name or the comment should say > something about arc. > > Also, it seems like this only works for apps without a launch id, which probably > depends on ARC never using a launch id. It seems that based on logic we don't have situation when multiple apps' ids comes to this set. Would it be possible to use something like std::string arc_app_id_keep_pin_? In .cc we can use DCHECK(arc_app_id_keep_pin.empty()) on OnAppDisabling and arc_app_id_keep_pin = std::string(). WDYT?
The CQ bit was checked by msw@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 ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. (instead, update prefs in CLC's ShelfModelObserver overrides) This will help move ShelfDelegate responsibilities out of Chrome. Add LauncherAppUpdater::Delegate::OnAppDisabl[ing|ed] and CLC support. Avoid CLC removing sync model pin positions when ARC is disabled. Skip syncing the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. (instead, update prefs in CLC's ShelfModelObserver overrides) This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ==========
Description was changed from ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. (instead, update prefs in CLC's ShelfModelObserver overrides) This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. Instead, update prefs in CLC's ShelfModelObserver overrides. This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ==========
Description was changed from ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. Instead, update prefs in CLC's ShelfModelObserver overrides. This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,sky@chromium.org ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. Instead, update prefs in CLC's ShelfModelObserver overrides. This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,jamescook@chromium.org ==========
msw@chromium.org changed reviewers: - sky@chromium.org
Comments addressed; please take another look, thanks! The code and CL are simpler thanks to your great feedback. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:81: #include "extensions/browser/extension_system.h" On 2017/03/29 14:28:02, James Cook wrote: > Are these new includes needed? Nope, thanks for catching that; removed. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1379: if (ItemTypeIsPinned(item) && !ignore_persist_pinned_state_change_) On 2017/03/29 14:28:02, James Cook wrote: > SyncPinPosition() checks ignore_persist_pinned_state_change_. Do you need to do > it in both places? > > It might be clearer to always do it outside the call, since RemovePinPosition > can't check it. Or make a version of RemovePinPosition inside this class that > checks. > I changed the check inside SyncPinPosition to a DCHECK. I'd like to avoid adding a RemovePinPosition helper for now. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1396: old_item.app_launch_id.launch_id()); On 2017/03/29 14:28:02, James Cook wrote: > Is it valid to recycle the launch_id when GetShelfAppIdFromArcAppId() might have > changed the app_id? Yes, this translation is only done for arc::kPlayStoreAppId / ArcSupportHost::kHostAppId, which is only pinned with an empty launch id. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:316: std::set<std::string> keep_pin_positions_; On 2017/03/29 15:28:53, khmel wrote: > On 2017/03/29 14:28:02, James Cook wrote: > > Could this be handled with ignore_persistent_pinned_state_change_? > > > > If not, and this just exists for arc, either the name or the comment should > say > > something about arc. > > > > Also, it seems like this only works for apps without a launch id, which > probably > > depends on ARC never using a launch id. > > It seems that based on logic we don't have situation when multiple apps' ids > comes to this set. Would it be possible to use something like std::string > arc_app_id_keep_pin_? In .cc we can use DCHECK(arc_app_id_keep_pin.empty()) on > OnAppDisabling and arc_app_id_keep_pin = std::string(). WDYT? I switched to using a refactored sync flag, which seems sufficient for now. I can revise the approach if we encounter any issues. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_app_updater.h (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_app_updater.h:17: class LauncherAppUpdater { On 2017/03/29 14:28:02, James Cook wrote: > Aside: This class is weird. It seems to exist mostly to be a delegate interface, > but I don't understand why the delegation happens. All the code is in > chrome/browser/ui. I suppose it's because c/b/ui/app_list can't depend on c/b/ui/ash/launcher. (Chrome's handling of extensions vs arc apps shouldn't be known to app_list?) https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_arc_app_updater.cc (right): https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_arc_app_updater.cc:29: // When disabling ARC, apps are removed, but sync items should not be deleted. On 2017/03/29 14:28:02, James Cook wrote: > I suggest explaining why the items should not be deleted, like it your CL > comments. Done. https://codereview.chromium.org/2769323002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_arc_app_updater.cc:37: delegate()->OnAppDisabled(app_id); On 2017/03/29 14:28:03, James Cook wrote: > It seems like these OnAppDisabling() etc. functions should also be called from > LauncherExtensionAppUpdater, or these shouldn't be delegate methods and > LauncherArcAppUpdater should be calling directly into CLC. Good call! Changed this to directly call into ChromeLauncherController.
That version is much better, thanks! lgtm https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (left): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1376: LOG(ERROR) << "Unexpected removal of shelf item, id: " << old_item.id; nit: may be leave error logging?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (left): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1376: LOG(ERROR) << "Unexpected removal of shelf item, id: " << old_item.id; On 2017/03/29 21:54:10, khmel wrote: > nit: may be leave error logging? This is not really unexpected anymore; shelf items may be removed by the shelf components in Ash directly from here on out. We should also remove the map...
Code looks good, but one clarity comment and one question about tests. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:239: std::unique_ptr<base::AutoReset<bool>> DoNotSyncPinChanges(); hrm. I like the idea of returning a RAII-style "lock" but it's confusing that you're returning an AutoReset. This might be clearer if the AutoReset was typedef'd to something like a SyncPinBlocker or BlockSyncPinsLock. Or you could use a trivial ScopedDoNotSyncPins class. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1098: auto do_not_sync_pin_changes = DoNotSyncPinChanges(); nit: don't use auto, here or below. "auto" masks the fact that this is a scoped lock. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1997: ChromeLauncherControllerImpl::set_instance_for_test(nullptr); Could this be related to tests polluting global state? Like the test before or after this one needs instance_ in a different state?
Please take another look; thanks! https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:239: std::unique_ptr<base::AutoReset<bool>> DoNotSyncPinChanges(); On 2017/03/29 23:23:56, James Cook wrote: > hrm. I like the idea of returning a RAII-style "lock" but it's confusing that > you're returning an AutoReset. This might be clearer if the AutoReset was > typedef'd to something like a SyncPinBlocker or BlockSyncPinsLock. Or you could > use a trivial ScopedDoNotSyncPins class. Elliot suggests adding move semantics to AutoReset, but that would require a change to base (I could do that in a follow-up). For now, I added |using PinSyncingDisabler = std::unique_ptr<base::AutoReset<bool>>;| to ChromeLauncherController, in tribute to the beautiful snowflake that is SigninScreenPolicyProvider::GetScopedSigninScreenPolicyProviderDisablerForTesting() https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/signi... I also considered these type names: MoveableAutoResetBool (too verbose?), CLC::PinSyncLock (not really a typical lock acquired for a shared resource, etc.?), None of these solutions are particularly appealing to me, but I'll take whichever approach you like best. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1098: auto do_not_sync_pin_changes = DoNotSyncPinChanges(); On 2017/03/29 23:23:56, James Cook wrote: > nit: don't use auto, here or below. "auto" masks the fact that this is a scoped > lock. Done. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1997: ChromeLauncherControllerImpl::set_instance_for_test(nullptr); On 2017/03/29 23:23:56, James Cook wrote: > Could this be related to tests polluting global state? Like the test before or > after this one needs instance_ in a different state? No, the tests still fail even when run in isolation. I suspect it has something to do with multiple calls to SwitchActiveUser within a single test fixture.
The CQ bit was checked by msw@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...
LGTM with a name suggestion Thanks for fighting through this. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:239: std::unique_ptr<base::AutoReset<bool>> DoNotSyncPinChanges(); On 2017/03/30 00:33:29, msw wrote: > On 2017/03/29 23:23:56, James Cook wrote: > > hrm. I like the idea of returning a RAII-style "lock" but it's confusing that > > you're returning an AutoReset. This might be clearer if the AutoReset was > > typedef'd to something like a SyncPinBlocker or BlockSyncPinsLock. Or you > could > > use a trivial ScopedDoNotSyncPins class. > > Elliot suggests adding move semantics to AutoReset, but that would require a > change to base (I could do that in a follow-up). For now, I added |using > PinSyncingDisabler = std::unique_ptr<base::AutoReset<bool>>;| to > ChromeLauncherController, in tribute to the beautiful snowflake that is > SigninScreenPolicyProvider::GetScopedSigninScreenPolicyProviderDisablerForTesting() > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/signi... > > I also considered these type names: MoveableAutoResetBool (too verbose?), > CLC::PinSyncLock (not really a typical lock acquired for a shared resource, > etc.?), None of these solutions are particularly appealing to me, but I'll take > whichever approach you like best. I think "using" is fine for now. optional name idea: ScopedPinSyncDisabler?
Thanks! Landing now. https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right): https://codereview.chromium.org/2769323002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:239: std::unique_ptr<base::AutoReset<bool>> DoNotSyncPinChanges(); On 2017/03/30 02:05:28, James Cook wrote: > On 2017/03/30 00:33:29, msw wrote: > > On 2017/03/29 23:23:56, James Cook wrote: > > > hrm. I like the idea of returning a RAII-style "lock" but it's confusing > that > > > you're returning an AutoReset. This might be clearer if the AutoReset was > > > typedef'd to something like a SyncPinBlocker or BlockSyncPinsLock. Or you > > could > > > use a trivial ScopedDoNotSyncPins class. > > > > Elliot suggests adding move semantics to AutoReset, but that would require a > > change to base (I could do that in a follow-up). For now, I added |using > > PinSyncingDisabler = std::unique_ptr<base::AutoReset<bool>>;| to > > ChromeLauncherController, in tribute to the beautiful snowflake that is > > > SigninScreenPolicyProvider::GetScopedSigninScreenPolicyProviderDisablerForTesting() > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/signi... > > > > I also considered these type names: MoveableAutoResetBool (too verbose?), > > CLC::PinSyncLock (not really a typical lock acquired for a shared resource, > > etc.?), None of these solutions are particularly appealing to me, but I'll > take > > whichever approach you like best. > > I think "using" is fine for now. > > optional name idea: ScopedPinSyncDisabler? Done.
The CQ bit was checked by msw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2769323002/#ps240001 (title: "Use ScopedPinSyncDisabler name suggestion.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490840538511090,
"parent_rev": "6dc0588a9863eaed97f1ee554180ad7d21a811de", "commit_rev":
"ac32fbee0938078f84eea9524c1e6d99e1949f94"}
Message was sent while issue was closed.
Description was changed from ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. Instead, update prefs in CLC's ShelfModelObserver overrides. This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,jamescook@chromium.org ========== to ========== mash: Update shelf pin prefs in ShelfModelObserver overrides. Do not update prefs in ChromeLauncherController's ShelfDelegate overrides. Instead, update prefs in CLC's ShelfModelObserver overrides. This will help move ShelfDelegate responsibilities out of Chrome. Refactor do-not-sync flag for internal and ARC disabling support. Do not sync the initial browser shortcut pin position (see comment). Extract ItemTypeIsPinned helper for ChromeLauncherController. Limit the crbug.com/654622 workaround to affected test fixtures. (this workaround breaks some other fixtures like ArcMultiUser) BUG=557406 TEST=Automated; no shelf item behavior changes. R=khmel@chromium.org,jamescook@chromium.org Review-Url: https://codereview.chromium.org/2769323002 Cr-Commit-Position: refs/heads/master@{#460658} Committed: https://chromium.googlesource.com/chromium/src/+/ac32fbee0938078f84eea9524c1e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/ac32fbee0938078f84eea9524c1e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
