Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2154163002:
Change ContextMenu to use AlertCoordinator (Closed)
|
Created:
4 years, 5 months ago by gambard Modified:
4 years, 4 months ago Reviewers:
michaeldo, pkl (ping after 24h if needed), lpromero, Jackie Quinn, rohitrao (ping after 24h) CC:
chromium-reviews, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@alertCoordinatorDelegate Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange ContextMenu to use AlertCoordinator
This CL changes the ContextMenu class to use AlertCoordinator for handling the
display of the alerts.
BUG=none
Committed: https://crrev.com/d5c97e1190df02c1887cbdafc1f483062fdb9100
Cr-Commit-Position: refs/heads/master@{#408379}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Downstream compatibility #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : sort header #
Total comments: 6
Patch Set 5 : Revision #
Total comments: 2
Patch Set 6 : Typo #
Messages
Total messages: 31 (12 generated)
gambard@chromium.org changed reviewers: + jyquinn@chromium.org, michaeldo@chromium.org
PTAL.
The comment describing the use of this class should be updated. As I am not sure about its final use, any suggestion welcome.
lpromero@chromium.org changed reviewers: + lpromero@chromium.org
https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:19: // Once this coordinator is stopped, any menu items which have been added is s/is/are https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:47: [_alertCoordinator addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL) Is this needed? Isn't a cancel button always added anyway?
https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:32: _alertCoordinatorDelegate.reset( This is unused, isn't it? I guess you need to set it as delegate of the alert coordinator. I was more thinking of internalizing this delegate. When creating an AlertCoordinator, pass a UIAlertControllerStyle, and in the initializer, create the appropriate delegate class. https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:47: [_alertCoordinator addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL) On 2016/07/18 12:35:54, lpromero wrote: > Is this needed? Isn't a cancel button always added anyway? And once this is gone, this class is just a dumb wrapper around AlertCoordinator. Why not removing it?
My answers. https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:32: _alertCoordinatorDelegate.reset( On 2016/07/18 12:40:08, lpromero wrote: > This is unused, isn't it? > > I guess you need to set it as delegate of the alert coordinator. > I was more thinking of internalizing this delegate. When creating an > AlertCoordinator, pass a UIAlertControllerStyle, and in the initializer, create > the appropriate delegate class. Each delegate needs different arguments for init (params for ActionSheet, title for ModalAlert). I found it complicated to create the two different delegates in the same init. One solution could be to create one init method in AlertCoordinator for each delegate. I will do a tentative CL for this. https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:47: [_alertCoordinator addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL) On 2016/07/18 12:40:08, lpromero wrote: > On 2016/07/18 12:35:54, lpromero wrote: > > Is this needed? Isn't a cancel button always added anyway? > > And once this is gone, this class is just a dumb wrapper around > AlertCoordinator. Why not removing it? michaeldo@ from https://codereview.chromium.org/2119373002#ps100001 : "The goal of the ContextMenuCoordinator was to put the logic (action blocks) for shared items into that class. It hasn't quite happened yet but the goal should still be to move the shared logic there." So it seems that this class will have more functionalities later. For now, yes, it is only a wrapper.
https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:32: _alertCoordinatorDelegate.reset( On 2016/07/18 12:54:16, gambard wrote: > On 2016/07/18 12:40:08, lpromero wrote: > > This is unused, isn't it? > > > > I guess you need to set it as delegate of the alert coordinator. > > I was more thinking of internalizing this delegate. When creating an > > AlertCoordinator, pass a UIAlertControllerStyle, and in the initializer, > create > > the appropriate delegate class. > > Each delegate needs different arguments for init (params for ActionSheet, title > for ModalAlert). I found it complicated to create the two different delegates in > the same init. > One solution could be to create one init method in AlertCoordinator for each > delegate. I will do a tentative CL for this. Maybe two factory methods instead of two initializers? They would init then configure the instances. https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:47: [_alertCoordinator addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL) On 2016/07/18 12:54:16, gambard wrote: > On 2016/07/18 12:40:08, lpromero wrote: > > On 2016/07/18 12:35:54, lpromero wrote: > > > Is this needed? Isn't a cancel button always added anyway? > > > > And once this is gone, this class is just a dumb wrapper around > > AlertCoordinator. Why not removing it? > > michaeldo@ from https://codereview.chromium.org/2119373002#ps100001 : "The goal > of the ContextMenuCoordinator was to put the logic (action blocks) for shared > items into that class. It hasn't quite happened yet but the goal should still be > to move the shared logic there." > So it seems that this class will have more functionalities later. For now, yes, > it is only a wrapper. Sounds good. Thanks for the reference!
https://codereview.chromium.org/2154163002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:33: [[ActionSheet alloc] initWithParams:params]); I believe this will need to be updated based on https://codereview.chromium.org/2119373002. It appears that the delegate there has been removed. We need to determine if we can use AlertCoordinator here without the delegate. Also note the ContextMenuParams object should be used here, but Not used for the generic Alerts.
Patchset #4 (id:60001) has been deleted
gambard@chromium.org changed reviewers: + rohitrao@chromium.org
PTAL.
gambard@chromium.org changed reviewers: + pkl@chromium.org
+pkl@ for owner lgtm
https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:20: // cleared. What does this comment mean? Specifically, "...are cleared". Where is that happening? Is that supposed to happen in the implementation of the -stop method? https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:62: [_alertCoordinator start]; I can see that the base class -start method does nothing, but are you still supposed to call [super start] from here? Same for -stop. Is it supposed to call [super stop]?
lgtm with one comment https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:18: base::scoped_nsobject<AlertCoordinator> _alertCoordinator; Any reason not to have this specifically be of the ActionSheetCoordinator type?
Thanks, PTAL. https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:20: // cleared. On 2016/07/26 16:24:25, pklpkl wrote: > What does this comment mean? Specifically, "...are cleared". Where is that > happening? Is that supposed to happen in the implementation of the -stop method? Yes, it happens in the alertCoordinator's |stop| method. I updated the comment. https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:18: base::scoped_nsobject<AlertCoordinator> _alertCoordinator; On 2016/07/26 16:56:17, michaeldo wrote: > Any reason not to have this specifically be of the ActionSheetCoordinator type? Not really. The only difference between ActionSheetCoordinator and AlertCoordinator is the init. But yes, it might be clearer to let it be ActionSheetCoordinator. https://codereview.chromium.org/2154163002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:62: [_alertCoordinator start]; On 2016/07/26 16:24:25, pklpkl wrote: > I can see that the base class -start method does nothing, but are you still > supposed to call [super start] from here? > Same for -stop. Is it supposed to call [super stop]? From ChromeCoordinator.h: // The basic lifecycle methods for coordinators are -start and -stop. These // are blank template methods; child classes are expected to implement them and // do not need to invoke the superclass methods. Subclasses of ChromeCoordinator // that expect to be subclassed should not build functionality into these // methods.
https://codereview.chromium.org/2154163002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/2154163002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:20: // which have been added are delete. s/delete/deleted
lgtm
On 2016/07/27 15:44:46, pklpkl wrote: > lgtm lgtm
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2154163002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/2154163002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:20: // which have been added are delete. On 2016/07/27 15:37:47, michaeldo wrote: > s/delete/deleted Done.
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, pkl@chromium.org, jyquinn@chromium.org Link to the patchset: https://codereview.chromium.org/2154163002/#ps120001 (title: "Typo")
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.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Change ContextMenu to use AlertCoordinator This CL changes the ContextMenu class to use AlertCoordinator for handling the display of the alerts. BUG=none ========== to ========== Change ContextMenu to use AlertCoordinator This CL changes the ContextMenu class to use AlertCoordinator for handling the display of the alerts. BUG=none Committed: https://crrev.com/d5c97e1190df02c1887cbdafc1f483062fdb9100 Cr-Commit-Position: refs/heads/master@{#408379} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d5c97e1190df02c1887cbdafc1f483062fdb9100 Cr-Commit-Position: refs/heads/master@{#408379} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
