Creates alertCoordinatorDelegate for alerts

Creates alertCoordinatorDelegate for alerts

This CL creates alertCoordinatorDelegates for handling the action sheet alerts
and the modal alerts.

BUG=none

[gambard, 2016-07-18 08:47:02]

gambard@chromium.org changed reviewers:
+ jyquinn@chromium.org, lpromero@chromium.org, michaeldo@chromium.org

[gambard, 2016-07-18 08:47:02]

PTAL.

[lpromero, 2016-07-18 12:11:36]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.h:19: @interface
ActionSheet : NSObject<AlertCoordinatorDelegate>
I don't like the names of these new classes. What do you think of
ActionSheetDelegate and AlertDelegate?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.mm:10: // Parameters that
define what is shown in the context menu.
This comment should rather be in the header.

Moreover, it defines what *and how it* is shown, right?

And maybe s/context menu/action sheet?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.mm:26: #pragma mark -
Private Methods
These are AlertCoordinatorDelegate methods, not private methods.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.mm:30: // don't show the
context menu.
Idem s/context menu/action sheet.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.mm:44:
CGRectMake(_params.location.x, _params.location.y, 1.0, 1.0);
Is the location passed in params already localized for RTL?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm:38: ;
Remove.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm:44:
ASSERT_TRUE([actionSheet alertCoordinatorShouldStart:nil]);
s/ASSERT/EXPECT

ASSERT stops the test execution, while EXPECT prints out a log and continues.
Here, it's the last call in the tests so there might not be much difference.
Yet, I find the difference useful when reading: ASSERT is something that must be
true after setup and before testing, while EXPECT is the testing phase.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm:59: ;
Remove.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm:65:
ASSERT_FALSE([actionSheet alertCoordinatorShouldStart:nil]);
s/ASSERT/EXPECT

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:39: [alertCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_APP_OK)
Why here and not when creating the UIAlertController?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:55: -
(BOOL)alertCoordinatorShouldAddCancelButton:
Reading the previous methods implementations, should we introduce a
shouldAddOKButtons?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:18: // Tests
that ActionSheet ask for a default cancel button.
s/ActionSheet/ModalAlert, or AlertDelegate.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:47:
ASSERT_TRUE([modalAlert alertCoordinatorShouldStart:alertCoordinator]);
EXPECT here and below.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:50: // Tests
that the ModalAlert let the Alert start and do not add a button if it
Update the comments. It tests that it adds.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:76:
ASSERT_OCMOCK_VERIFY(alertCoordinator);
Isn't is automatically done? I was under that impression that exiting the scope,
thus deallocating the mocks, called verify.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:99: // has at
least one button.
Update comment?

[gambard, 2016-07-18 12:40:20]

I am making the changes. Here are some answers.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.h:19: @interface
ActionSheet : NSObject<AlertCoordinatorDelegate>
On 2016/07/18 12:11:35, lpromero wrote:
> I don't like the names of these new classes. What do you think of
> ActionSheetDelegate and AlertDelegate?

I was concerned about misunderstanding "ActionSheetDelegate" for "a delegate for
an ActionSheet". If you think it is OK, I will change them.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.mm:44:
CGRectMake(_params.location.x, _params.location.y, 1.0, 1.0);
On 2016/07/18 12:11:35, lpromero wrote:
> Is the location passed in params already localized for RTL?

I am looking into it.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:39: [alertCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_APP_OK)
On 2016/07/18 12:11:36, lpromero wrote:
> Why here and not when creating the UIAlertController?

This code adds an OK button only if there is no other button. It is here to
prevent a situation where you display an alert with no button (which the user
cannot dismiss).

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:55: -
(BOOL)alertCoordinatorShouldAddCancelButton:
On 2016/07/18 12:11:36, lpromero wrote:
> Reading the previous methods implementations, should we introduce a
> shouldAddOKButtons?

No, the OK button is only here if there is no other button.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:76:
ASSERT_OCMOCK_VERIFY(alertCoordinator);
On 2016/07/18 12:11:36, lpromero wrote:
> Isn't is automatically done? I was under that impression that exiting the
scope,
> thus deallocating the mocks, called verify.

No. You have to call verify (I just tested it).

[lpromero, 2016-07-18 12:51:43]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.h:19: @interface
ActionSheet : NSObject<AlertCoordinatorDelegate>
On 2016/07/18 12:40:20, gambard wrote:
> On 2016/07/18 12:11:35, lpromero wrote:
> > I don't like the names of these new classes. What do you think of
> > ActionSheetDelegate and AlertDelegate?
> 
> I was concerned about misunderstanding "ActionSheetDelegate" for "a delegate
for
> an ActionSheet". If you think it is OK, I will change them.

As per a comment here:
https://codereview.chromium.org/2154163002/diff/1/ios/chrome/browser/ui/conte...
could we even hide these classes from clients?

[lpromero, 2016-07-18 12:55:31]

Thanks!

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:28: #pragma mark -
Private Methods
s/Private/AlertCoordinatorDelegate

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:39: [alertCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_APP_OK)
On 2016/07/18 12:40:20, gambard wrote:
> On 2016/07/18 12:11:36, lpromero wrote:
> > Why here and not when creating the UIAlertController?
> 
> This code adds an OK button only if there is no other button. It is here to
> prevent a situation where you display an alert with no button (which the user
> cannot dismiss).

I misunderstood the flow with alertCoordinatorUIAlertController:. Ack!

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:55: -
(BOOL)alertCoordinatorShouldAddCancelButton:
On 2016/07/18 12:40:20, gambard wrote:
> On 2016/07/18 12:11:36, lpromero wrote:
> > Reading the previous methods implementations, should we introduce a
> > shouldAddOKButtons?
> 
> No, the OK button is only here if there is no other button.

Acknowledged.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert_unittest.mm:76:
ASSERT_OCMOCK_VERIFY(alertCoordinator);
On 2016/07/18 12:40:20, gambard wrote:
> On 2016/07/18 12:11:36, lpromero wrote:
> > Isn't is automatically done? I was under that impression that exiting the
> scope,
> > thus deallocating the mocks, called verify.
> 
> No. You have to call verify (I just tested it).

Acknowledged.

[marq (ping after 24h), 2016-07-18 13:02:40]

marq@chromium.org changed reviewers:
+ marq@chromium.org

[marq (ping after 24h), 2016-07-18 13:02:42]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.h:19: @interface
ActionSheet : NSObject<AlertCoordinatorDelegate>
On 2016/07/18 12:40:20, gambard wrote:
> On 2016/07/18 12:11:35, lpromero wrote:
> > I don't like the names of these new classes. What do you think of
> > ActionSheetDelegate and AlertDelegate?
> 
> I was concerned about misunderstanding "ActionSheetDelegate" for "a delegate
for
> an ActionSheet". If you think it is OK, I will change them.

ObjC *classes* shouldn't be named 'Delegate', since that's conventionally used
for a particular kind of protocol.

I think this class is really an 'ActionSheetContentAdaptor', since it's exposing
web::ContextmenuParams in a way that another object that consumes ActionSheet
content information can use.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.h:22: -
(instancetype)initWithParams:(const web::ContextMenuParams&)params;
 NS_DESIGNATED_INITIALIZER, and mark the regular -init NS_UNAVAILABLE.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.mm:38: UIAlertController*
alert = [UIAlertController
I don't like the idea of a coordinator being handed the view controller it's
going to manage. I'd prefer that the coordinator delegate protocol allow it to
collect the title, style, and source view/rect but have the coordinator itself
handle creating the view controller.

Consider: Suppose we have a "drop-in" MDC action sheet replacement we want to
try out. If the coordinator creates and owns the view controller it defines,
then that change can be encapsulated inside the view controller. With this API,
we have to update the delegate protocol and anything that implements it.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet_unittest.mm:44:
ASSERT_TRUE([actionSheet alertCoordinatorShouldStart:nil]);
On 2016/07/18 12:11:35, lpromero wrote:
> s/ASSERT/EXPECT
> 
> ASSERT stops the test execution, while EXPECT prints out a log and continues.
> Here, it's the last call in the tests so there might not be much difference.
> Yet, I find the difference useful when reading: ASSERT is something that must
be
> true after setup and before testing, while EXPECT is the testing phase.

ASSERT should generally only be used in cases where further execution would
cause a crash.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.h:12: @interface ModalAlert
: NSObject<AlertCoordinatorDelegate>
ModalAlertContentAdaptor?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.h:15: -
(instancetype)initWithTitle:(NSString*)title;
 NS_DESIGNATED_INITIALIZER, and mark the regular -init NS_UNAVAILABLE.

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:33: if
(![alertCoordinator.baseViewController.view window] &&
The coordinator shouldn't need to hand off this check to a delegate, should it?

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:39: [alertCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_APP_OK)
I don't like that UI configuration is being done here as a side effect of
-alertCoordinatorShouldStart. Again, can't the coordinator just check this?

[lpromero, 2016-07-18 13:21:26]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.h:12: @interface ModalAlert
: NSObject<AlertCoordinatorDelegate>
On 2016/07/18 13:02:41, marq wrote:
> ModalAlertContentAdaptor?

I'd go with AlertContentAdaptor.

[lpromero, 2016-07-18 13:25:19]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:39: [alertCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_APP_OK)
On 2016/07/18 13:02:41, marq wrote:
> I don't like that UI configuration is being done here as a side effect of
> -alertCoordinatorShouldStart. Again, can't the coordinator just check this?

The problem is that there are 2 cases we are handling: alert and action sheet
and I guess they don't have the same behavior. This is to separate the code
paths.

[marq (ping after 24h), 2016-07-18 13:37:14]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/modal_alert.mm (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/modal_alert.mm:39: [alertCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_APP_OK)
On 2016/07/18 13:25:19, lpromero wrote:
> On 2016/07/18 13:02:41, marq wrote:
> > I don't like that UI configuration is being done here as a side effect of
> > -alertCoordinatorShouldStart. Again, can't the coordinator just check this?
> 
> The problem is that there are 2 cases we are handling: alert and action sheet
> and I guess they don't have the same behavior. This is to separate the code
> paths.

Yes, but coordinators are allowed to know things about the view controllers they
manage. It can even be abstracted a bit -- if the coordinator is going to
present something using the popover style, then it doesn't care if there are no
actions; otherwise, if there are no actions, it adds an 'OK' item by default.

[gambard, 2016-07-18 14:55:29]

Waiting for an answer on architecture of
https://codereview.chromium.org/2119373002/ before making changes here.

[michaeldo, 2016-07-18 15:29:07]

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
File ios/chrome/browser/ui/alert_coordinator/action_sheet.h (right):

https://codereview.chromium.org/2156913002/diff/1/ios/chrome/browser/ui/alert...
ios/chrome/browser/ui/alert_coordinator/action_sheet.h:17: // all device form
factors. Will show a sheet on the phone and use a popover on
ContextMenuParams shouldn't be used anywhere in this alert_coordinator
directory. Only the specialized ContextMenuCoordinator should use it. There is
some conflicting terminology here. web::ContextMenuParams is specifically used
to store values needed to display a "web site context menu" that is triggered
when the user long presses on a link.

It sounds like AlertCoordinator is a much more generic class than that so we
shouldn't make it dependent on the ContextMenuParams type.