Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2119373002:
Creates coordinators for displaying alerts (Closed)
DescriptionCreates coordinator for displaying alerts
This CL creates an AlertControllerCoordinator which can handle displaying alerts.
This coordinator can display action sheets and modal alerts.
BUG=629515
Committed: https://crrev.com/b61a9611880a5e5cfdb9780985df88810cf657ef
Cr-Commit-Position: refs/heads/master@{#407431}
Patch Set 1 #
Total comments: 51
Patch Set 2 : AlertCoordinator uses delegates #Patch Set 3 : Add tests #Patch Set 4 : Add comment for StubDelegate #Patch Set 5 : Add dependencies + sort headers #Patch Set 6 : Split CL #
Total comments: 22
Patch Set 7 : Revisions #Patch Set 8 : Remove the use of a delegate, use only one class. #Patch Set 9 : Remove stub delegate #Patch Set 10 : cleanup #
Total comments: 1
Patch Set 11 : Fix tests #Patch Set 12 : Fix init #
Total comments: 4
Patch Set 13 : Nits #
Total comments: 6
Patch Set 14 : Fix tests #Dependent Patchsets: Messages
Total messages: 44 (20 generated)
Description was changed from ========== Creates coordinator for displaying alerts This CL creates a common abstract AlertControllerCoordinator which can handle the two types of AlertController: action sheets (context menus) and standard alerts. As this coordinator is abstract and should not be instanciated, they are handled by its subclasses: ActionSheetCoordinator and AlertCoordinator. BUG=none ========== to ========== Creates coordinator for displaying alerts This CL creates a common abstract AlertControllerCoordinator which can handle the two types of AlertController: action sheets (context menus) and standard alerts. As this coordinator is abstract and should not be instanciated, they are handled by its subclasses: ActionSheetCoordinator and AlertCoordinator. These classes should replace the current ContextMenuCoordinator once the downstream changes are made. BUG=none ==========
gambard@chromium.org changed reviewers: + jyquinn@chromium.org, marq@chromium.org
+marq for owners approval. +lpromero FYI. This CL should replace the ContextMenuCoordinator. The tests are yet to come, but I would love an opinion on the implementation. It is a proposal do not hesitate to discuss it.
marq@chromium.org changed reviewers: + michaeldo@chromium.org
+michaeldo Cool, I'm keen to look a this, but don't expect much (if any) feedback from anyone until after the hackathon.
lpromero@chromium.org changed reviewers: + lpromero@chromium.org
I don't think subclasses are the best way to achieve this. Granted, UIKit UIAlertController is a weird mix between alerts and action sheets, but I think we can do better. We went the subclassing way with Bookmarks, and it clearly is too tight a coupling, as it would be here. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/OWNERS (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/OWNERS:1: jyquinn@chromium.org Rather add yourself as OWNER? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h (left): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h:18: @interface ContextMenuCoordinator : NSObject Add a TODO in this controller that it is deprecated and clients should use ActionSheetCoordinator. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm:16: web::ContextMenuParams _params; // weak? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm:50: return alert; If renamed newAlertController, you need to return [alert retain]. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:9: @interface AlertControllerCoordinator (subclassing) This looks more like delegate calls. Prefer delegation over subclassing. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:13: // Creates the AlertController. s/AlertController/UIAlertController. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:14: - (UIAlertController*)createAlertController; Here too, I'd say createUIAlertController. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:14: - (UIAlertController*)createAlertController; Traditionally, methods starting with `create` returned an object with a retain count of +1. I am not sure how the static analyzer and ARC interpret it nowadays. To be on the safe side, prefer renaming it newAlertController, returning an object with a retain count of +1. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:18: - (BOOL)shouldCreateCancelButtonAtCreation; Is the button only created, not added? If added, what about: shouldDisplayCancelButton? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:17: @interface AlertControllerCoordinator : ChromeCoordinator How do we make sure we don't end up with a huge tree of subclasses? The issue being if we want to share traits between two different subclasses. That was a mess with GoogleKit content views. Can't we compose somehow? Or just share the common interface with a protocol? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:25: // Adds an item at the end of the menu if |visible| is false. Instead of silently fail, should it DCHECK? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:28: destructive:(BOOL)isDestructive; Pass a UIAlertActionStyle instead. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:30: // Creates a cancel button. This button will be displayed at the end. I'd remove the word "Creates". What about: // Adds a cancel item with a default title. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:33: // Returns the number of action attached to the current alert. s/action/actions https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:34: - (NSUInteger)actionCount; s/actionCount/actionsCount https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:58: actionBlock(); DCHECK actionBlock, or only execute if non-nil. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:77: _alertController.reset(); These 3 lines are -alertDismissed. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:80: - (void)addCancelButton { Implement by calling to addItemWithTitle:action:style:. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:103: _alertController.reset([[self createAlertController] retain]); If renamed newAlertController, you need to remove the retain here. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator_unittest.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator_unittest.mm:5: #import "ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator_unittest.h" Remove the _unittest. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:8: #import "ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h" It feels weird to call the whole folder alert_coordinator, as it is only one specialization of the more generic alert_controller_coordinator. What about just alert_controller? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:14: - (instancetype)initWithViewController:(UIViewController*)viewController Why loosing the "Base"? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:35: } Pass nil as action block, and accept nil in the API. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:46: return alert; If renamed newAlertController, you need to return [alert retain]. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:5: #import "ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.h" A limit of the boilerplate script: remove the _unittest. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/ios_chrome.gyp File ios/chrome/ios_chrome.gyp (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/ios_chrome.gyp#n... ios/chrome/ios_chrome.gyp:599: 'browser/ui/UIView+SizeClassSupport.mm', Can you sort these two files?
On 2016/07/06 09:04:00, lpromero wrote: > I don't think subclasses are the best way to achieve this. Granted, UIKit > UIAlertController is a weird mix between alerts and action sheets, but I think > we can do better. > We went the subclassing way with Bookmarks, and it clearly is too tight a > coupling, as it would be here. > > To avoid the subclassing issues Louis brings up, perhaps rather than having two coordinator subclasses for each style you could use one coordinator for both (as UIAlertController is one controller class for both styles)?
Thanks for the input. PTAL! I now use one class with two delegates (one for each alert type). jyquinn@ : I have added myself as owner of this repository. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/OWNERS (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/OWNERS:1: jyquinn@chromium.org On 2016/07/06 09:03:58, lpromero wrote: > Rather add yourself as OWNER? I copied the directory. Sorry. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h (left): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h:18: @interface ContextMenuCoordinator : NSObject On 2016/07/06 09:03:58, lpromero wrote: > Add a TODO in this controller that it is deprecated and clients should use > ActionSheetCoordinator. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm:16: web::ContextMenuParams _params; On 2016/07/06 09:03:59, lpromero wrote: > // weak? It is not pointer. Should it be weak? https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.mm:50: return alert; On 2016/07/06 09:03:58, lpromero wrote: > If renamed newAlertController, you need to return [alert retain]. Acknowledged. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:9: @interface AlertControllerCoordinator (subclassing) On 2016/07/06 09:03:59, lpromero wrote: > This looks more like delegate calls. Prefer delegation over subclassing. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:13: // Creates the AlertController. On 2016/07/06 09:03:59, lpromero wrote: > s/AlertController/UIAlertController. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:14: - (UIAlertController*)createAlertController; On 2016/07/06 09:03:59, lpromero wrote: > Traditionally, methods starting with `create` returned an object with a retain > count of +1. I am not sure how the static analyzer and ARC interpret it > nowadays. To be on the safe side, prefer renaming it newAlertController, > returning an object with a retain count of +1. I have change it to a method delegate name alertCoordinatorUIAlertController and so it does not give ownership to the caller. Please comment if you think it could be better. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator+subclassing.h:18: - (BOOL)shouldCreateCancelButtonAtCreation; On 2016/07/06 09:03:59, lpromero wrote: > Is the button only created, not added? If added, what about: > shouldDisplayCancelButton? Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:17: @interface AlertControllerCoordinator : ChromeCoordinator On 2016/07/06 09:03:59, lpromero wrote: > How do we make sure we don't end up with a huge tree of subclasses? The issue > being if we want to share traits between two different subclasses. That was a > mess with GoogleKit content views. > Can't we compose somehow? Or just share the common interface with a protocol? I used delegates for Model Alert and Action Sheet. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:25: // Adds an item at the end of the menu if |visible| is false. On 2016/07/06 09:03:59, lpromero wrote: > Instead of silently fail, should it DCHECK? It does not fail, it does nothing. I think it is better than a DCHECK, but if you think otherwise, please tell me. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:28: destructive:(BOOL)isDestructive; On 2016/07/06 09:03:59, lpromero wrote: > Pass a UIAlertActionStyle instead. Ok but it crashes if two UIAlertActionStyleCancel are passed. It will do nothing if a cancel button has been added. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:30: // Creates a cancel button. This button will be displayed at the end. On 2016/07/06 09:03:59, lpromero wrote: > I'd remove the word "Creates". What about: > // Adds a cancel item with a default title. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:33: // Returns the number of action attached to the current alert. On 2016/07/06 09:03:59, lpromero wrote: > s/action/actions Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h:34: - (NSUInteger)actionCount; On 2016/07/06 09:03:59, lpromero wrote: > s/actionCount/actionsCount Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:58: actionBlock(); On 2016/07/06 09:03:59, lpromero wrote: > DCHECK actionBlock, or only execute if non-nil. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:77: _alertController.reset(); On 2016/07/06 09:03:59, lpromero wrote: > These 3 lines are -alertDismissed. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:80: - (void)addCancelButton { On 2016/07/06 09:03:59, lpromero wrote: > Implement by calling to addItemWithTitle:action:style:. Acknowledged. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.mm:103: _alertController.reset([[self createAlertController] retain]); On 2016/07/06 09:03:59, lpromero wrote: > If renamed newAlertController, you need to remove the retain here. Acknowledged. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator_unittest.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator_unittest.mm:5: #import "ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator_unittest.h" On 2016/07/06 09:03:59, lpromero wrote: > Remove the _unittest. It is here for preventing compilation until I write test. It will then be removed. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:8: #import "ios/chrome/browser/ui/alert_coordinator/alert_controller_coordinator.h" On 2016/07/06 09:03:59, lpromero wrote: > It feels weird to call the whole folder alert_coordinator, as it is only one > specialization of the more generic alert_controller_coordinator. What about just > alert_controller? See new names. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:14: - (instancetype)initWithViewController:(UIViewController*)viewController On 2016/07/06 09:03:59, lpromero wrote: > Why loosing the "Base"? Changed. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:35: } On 2016/07/06 09:03:59, lpromero wrote: > Pass nil as action block, and accept nil in the API. Done. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:46: return alert; On 2016/07/06 09:03:59, lpromero wrote: > If renamed newAlertController, you need to return [alert retain]. Acknowledged. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/browser/ui/alert... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:5: #import "ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.h" On 2016/07/06 09:03:59, lpromero wrote: > A limit of the boilerplate script: remove the _unittest. It is here for preventing compilation until I write test. It will then be removed. https://codereview.chromium.org/2119373002/diff/1/ios/chrome/ios_chrome.gyp File ios/chrome/ios_chrome.gyp (right): https://codereview.chromium.org/2119373002/diff/1/ios/chrome/ios_chrome.gyp#n... ios/chrome/ios_chrome.gyp:599: 'browser/ui/UIView+SizeClassSupport.mm', On 2016/07/06 09:04:00, lpromero wrote: > Can you sort these two files? Done.
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. I recommend using ActionSheetCoordinator from within ContextMenuCoordinator rather than replacing it. For more discussion/information please see this doc: https://docs.google.com/a/google.com/document/d/19A74_UOxv_tGRKWdumCfNmajUoSe...
Description was changed from ========== Creates coordinator for displaying alerts This CL creates a common abstract AlertControllerCoordinator which can handle the two types of AlertController: action sheets (context menus) and standard alerts. As this coordinator is abstract and should not be instanciated, they are handled by its subclasses: ActionSheetCoordinator and AlertCoordinator. These classes should replace the current ContextMenuCoordinator once the downstream changes are made. BUG=none ========== to ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying the alerts using a specific delegate. The delegate is responsible for the creationg of the alert, determining its style. BUG=none ==========
Description was changed from ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying the alerts using a specific delegate. The delegate is responsible for the creationg of the alert, determining its style. BUG=none ========== to ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying the alerts using a specific delegate. The delegate is responsible for the creation of the alert, determining its style. BUG=none ==========
PTAL, I have split this CL into two CLs. michaeldo@, from what I understand of your comment, ContextMenu will evolve to handle more functionalities. I have modified the class to let it use AlertCoordinator in https://codereview.chromium.org/2154163002#ps1 . Do you think the current uses of ContextMenu will use those new functionalities (downstream CL)? Or should they use an AlertCoordinator? More generally, for the new classes using an action sheet, should they use a ContextMenu or AlertCoordinator directly?
https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:31: // Calling stop on this coordinator destroy the current alert. s/destroy/destroys. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:31: // Calling stop on this coordinator destroy the current alert. Does it dismiss it with animation? https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:37: // Whether a cancel has been added. +button https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:41: // Whether the context menu is visible. This will be true after |-start| is s/context menu/alert https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:80: [_alertController dismissViewControllerAnimated:NO completion:nil]; start is animated, but stop is not? https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:97: [self addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL) More than one line, please add curly braces to this if section. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:173: // Cleanup. Why Cleanup? https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:215: for (UIAlertAction* action in alertController.actions) { Feels weird to iterate on one item. Is there a reason to? https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:267: // Tests that a cancel button is added is the delegate ask for it. s/is/if for the second.
https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:15: @protocol AlertCoordinatorDelegate<NSObject> The preferred way to construct delegate protocols is for all methods to be optional, and for the delegating class to still be functional without a delegate. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:18: - (BOOL)alertCoordinatorShouldStart:(AlertCoordinator*)alertCoordinator; This should default to something (probably YES) if the delegate doesn't implement this method. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:20: - (UIAlertController*)alertCoordinatorUIAlertController: As per my comments in the other CL, I would prefer that instead of the delegate providing the alert controller, instead they provide the information this class needs to create the alert controller, again with reasonable defaults for delegate implementations that don't implement some or all of these methods.
I have done the changes from your comments. From the comments on this CL and on its depend CL, it might be better to refactor everything into one class and remove delegates. I am making a patch to see if it works. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:31: // Calling stop on this coordinator destroy the current alert. On 2016/07/18 12:29:33, lpromero wrote: > s/destroy/destroys. Done. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:31: // Calling stop on this coordinator destroy the current alert. On 2016/07/18 12:29:33, lpromero wrote: > Does it dismiss it with animation? No. Should it? https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:37: // Whether a cancel has been added. On 2016/07/18 12:29:33, lpromero wrote: > +button Done. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:41: // Whether the context menu is visible. This will be true after |-start| is On 2016/07/18 12:29:33, lpromero wrote: > s/context menu/alert Done. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:80: [_alertController dismissViewControllerAnimated:NO completion:nil]; On 2016/07/18 12:29:33, lpromero wrote: > start is animated, but stop is not? This is the current behavior. It don't know if we should change it. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:97: [self addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL) On 2016/07/18 12:29:33, lpromero wrote: > More than one line, please add curly braces to this if section. Done. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:173: // Cleanup. On 2016/07/18 12:29:33, lpromero wrote: > Why Cleanup? To prevent the console from logging a warning because the alert has not been displayed. It could be removed. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:215: for (UIAlertAction* action in alertController.actions) { On 2016/07/18 12:29:33, lpromero wrote: > Feels weird to iterate on one item. Is there a reason to? Nope. Done. https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator_unittest.mm:267: // Tests that a cancel button is added is the delegate ask for it. On 2016/07/18 12:29:33, lpromero wrote: > s/is/if for the second. Done.
PTAL, I have put everything in one class and remove the use of delegate. The inits are a bit weird but the rest seems fine to me. The tests are not updated yet. I will update them if we decide to go with this design.
https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:80: [_alertController dismissViewControllerAnimated:NO completion:nil]; On 2016/07/18 14:03:03, gambard wrote: > On 2016/07/18 12:29:33, lpromero wrote: > > start is animated, but stop is not? > > This is the current behavior. It don't know if we should change it. Then don't change. Let's keep the behavior the same. Thanks for checking!
https://codereview.chromium.org/2119373002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:40: params:(const web::ContextMenuParams&)params; Per the comment for this class, this is a generic coordinator to handle alerts, contextMenuParams is too specific to pass in data to this class. See details in my comment on https://codereview.chromium.org/2156913002/. A similar struct could be created if needed to pass in the values which are used, but link_url, src_url, and referrer_policy shouldn't be there. The view should be accessible via the viewcontroller passed into this coordinator. The title and location are the only params left, so maybe a struct isn't even needed. Ref: https://cs.corp.google.com/clankium/src/ios/web/public/web_state/context_menu...
On 2016/07/18 15:40:09, michaeldo wrote: > https://codereview.chromium.org/2119373002/diff/180001/ios/chrome/browser/ui/... > File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): > > https://codereview.chromium.org/2119373002/diff/180001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:40: params:(const > web::ContextMenuParams&)params; > Per the comment for this class, this is a generic coordinator to handle alerts, > contextMenuParams is too specific to pass in data to this class. See details in > my comment on https://codereview.chromium.org/2156913002/. A similar struct > could be created if needed to pass in the values which are used, but link_url, > src_url, and referrer_policy shouldn't be there. The view should be accessible > via the viewcontroller passed into this coordinator. The title and location are > the only params left, so maybe a struct isn't even needed. > > Ref: > https://cs.corp.google.com/clankium/src/ios/web/public/web_state/context_menu... Yep thanks. I use the code from ContextMenu, thinking it was only presenting a popover. I will update this code once we decide on a architecture.
PTAL. I have fixed the tests and remove the dependency on ContextMenuParams.
LGTM with nits. https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:32: // -configureForActionSheetWithRect:popoverView: is not call, it will be a modal s/call/called/ https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:59: - (void)dealloc { The superclass dealloc does exactly this already.
Thanks. https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h (right): https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.h:32: // -configureForActionSheetWithRect:popoverView: is not call, it will be a modal On 2016/07/20 12:07:43, marq wrote: > s/call/called/ Done. https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm (right): https://codereview.chromium.org/2119373002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/alert_coordinator/alert_coordinator.mm:59: - (void)dealloc { On 2016/07/20 12:07:43, marq wrote: > The superclass dealloc does exactly this already. Done.
Description was changed from ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying the alerts using a specific delegate. The delegate is responsible for the creation of the alert, determining its style. BUG=none ========== to ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying alerts. This coordinator can display action sheets and modal alerts. BUG=629515 ==========
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2119373002/#ps240001 (title: "Nits")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/BUILD.gn File ios/chrome/BUILD.gn (right): https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/BUILD.gn#ne... ios/chrome/BUILD.gn:123: "//ui/strings:ui_strings_grit", Use "//ui/strings:ui_strings" instead ("//ui/strings:ui_strings_grit" is an implementation detail). https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome.gyp File ios/chrome/ios_chrome.gyp (right): https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome.... ios/chrome/ios_chrome.gyp:136: '../../ui/gfx/gfx.gyp:gfx', You should add the dependencies you added for gn here too. https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome_... File ios/chrome/ios_chrome_tests.gyp (right): https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome_... ios/chrome/ios_chrome_tests.gyp:34: '../../ui/gfx/gfx.gyp:gfx_test_support', and here
Patchset #14 (id:260001) has been deleted
The CQ bit was checked by gambard@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...
Thanks. https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/BUILD.gn File ios/chrome/BUILD.gn (right): https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/BUILD.gn#ne... ios/chrome/BUILD.gn:123: "//ui/strings:ui_strings_grit", On 2016/07/22 14:16:57, sdefresne wrote: > Use "//ui/strings:ui_strings" instead ("//ui/strings:ui_strings_grit" is an > implementation detail). Done. https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome.gyp File ios/chrome/ios_chrome.gyp (right): https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome.... ios/chrome/ios_chrome.gyp:136: '../../ui/gfx/gfx.gyp:gfx', On 2016/07/22 14:16:57, sdefresne wrote: > You should add the dependencies you added for gn here too. I did not add dependencies for chrome_browser. https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome_... File ios/chrome/ios_chrome_tests.gyp (right): https://codereview.chromium.org/2119373002/diff/240001/ios/chrome/ios_chrome_... ios/chrome/ios_chrome_tests.gyp:34: '../../ui/gfx/gfx.gyp:gfx_test_support', On 2016/07/22 14:16:57, sdefresne wrote: > and here Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2119373002/#ps280001 (title: "Fix tests")
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 ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying alerts. This coordinator can display action sheets and modal alerts. BUG=629515 ========== to ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying alerts. This coordinator can display action sheets and modal alerts. BUG=629515 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying alerts. This coordinator can display action sheets and modal alerts. BUG=629515 ========== to ========== Creates coordinator for displaying alerts This CL creates an AlertControllerCoordinator which can handle displaying alerts. This coordinator can display action sheets and modal alerts. BUG=629515 Committed: https://crrev.com/b61a9611880a5e5cfdb9780985df88810cf657ef Cr-Commit-Position: refs/heads/master@{#407431} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b61a9611880a5e5cfdb9780985df88810cf657ef Cr-Commit-Position: refs/heads/master@{#407431} |
