[iOS Clean] Added HTTP authentication dialog support.

[iOS Clean] Added HTTP authentication dialog support.

This CL adds a coordinator/mediator to present a DialogViewController
for HTTP authentication dialogs.

BUG=none

[kkhorimoto, 2017-06-07 21:09:21]

kkhorimoto@chromium.org changed reviewers:
+ edchin@chromium.org, marq@chromium.org

[kkhorimoto, 2017-06-07 21:11:57]

Removed old consumer classes

[kkhorimoto, 2017-06-08 00:40:47]

rebased, subclassed DialogMediator

[marq (ping after 24h), 2017-06-14 11:19:17]

Having seen this, I think I'd like to codify how we request user input using the
consumer approach. I'd like to consider having mutable input request objects
that the consumer object updates.

Also some more tweaking of what goes into coordinators and mediators.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.h
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.h:14:
// A coordinator for the UI elements used to present JavaScript dialogs.
JavaScript dialogs, or HTTP auth dialogs?

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.mm
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.mm:86:
[self.state runCallbackWithUserName:username password:password];
I'd rather have the mediator handle stuff like running callbacks the web layer
exposes.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm:79:
NSMutableArray<DialogButtonItem*>* items = [[NSMutableArray alloc] init];
I'd prefer just a static array return here:
   
return @[
  [DialogButtonItem ..
  [DialogButtonItem ...
];

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm:91:
NSMutableArray<DialogTextFieldItem*>* items = [[NSMutableArray alloc] init];
Static array return here too.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm:112:
NSString* username = inputStrings[USER_NAME_IDX];
Hmm.

Should the text field items be mutable objects that the mediator owns? They get
passed into the dialog view controller, and then it can populate a 'userString'
property when the user provides a value. Then the mediator can just use the
item's property. 

That gets rid of any synchronization of the ordering of strings sent via the
dismiss commands; the expectation is that whoever owns the items associated with
the dialog identified with |id| will check them for populated strings.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:17:
// A container object encapsulating all the state necessary to support an
Name quibbles -- I'm not wild about "State", because it implies the state may
change at some point. Maybe HTTPAuthDialogRequest?

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:21:
// the lifetime of every JavaScriptDialogState.
HTTPAuthDialogState, here an elsewhere. Careful with that (copy/paste errors) in
general.

Should this class and JSDialogState share a parent class?

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:34:
@property(nonatomic, readonly) NSString* title;
You're marking other readonly string properties as strong elsewhere; be
consistent here.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:42:
// Calls the callback passed on initialization with the provided parameters.
I regard the callback as an implementation detail. Can it be
'completeAuthenticationWith...'.

The semantics of nil username and password should be documented here.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm:45:
DCHECK(webState);
Document these assertions in the header.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm:61:
DCHECK(_callbackHasBeenExecuted);
Would it be correct to run the callback (nil, nil) if it hasn't run at this
point?

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm:66:
+ (instancetype)stateWithWebState:(web::WebState*)webState
Ordering nit: all initializers before dealloc.

[kkhorimoto, 2017-06-23 06:25:28]

rebase && Mark's comments

[kkhorimoto, 2017-06-23 06:25:42]

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.h
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.h:14:
// A coordinator for the UI elements used to present JavaScript dialogs.
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> JavaScript dialogs, or HTTP auth dialogs?
> 

HTTP auth!

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.mm
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_coordinator.mm:86:
[self.state runCallbackWithUserName:username password:password];
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> I'd rather have the mediator handle stuff like running callbacks the web layer
> exposes.

Done.  We'll still need to run the callback here for |-cancelOverlay|, but that
greatly HTTPAuthDialogDismissalCommands, since we don't need to send the
parameters anymore.  It'll help even more for JavaScript dialogs.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm:79:
NSMutableArray<DialogButtonItem*>* items = [[NSMutableArray alloc] init];
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> I'd prefer just a static array return here:
>    
> return @[
>   [DialogButtonItem ..
>   [DialogButtonItem ...
> ];

Done.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm:91:
NSMutableArray<DialogTextFieldItem*>* items = [[NSMutableArray alloc] init];
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> Static array return here too.

Done.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_mediator.mm:112:
NSString* username = inputStrings[USER_NAME_IDX];
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> Hmm.
> 
> Should the text field items be mutable objects that the mediator owns? They
get
> passed into the dialog view controller, and then it can populate a
'userString'
> property when the user provides a value. Then the mediator can just use the
> item's property. 
> 
> That gets rid of any synchronization of the ordering of strings sent via the
> dismiss commands; the expectation is that whoever owns the items associated
with
> the dialog identified with |id| will check them for populated strings.

As we discussed in the weekly meeting, I just implemented the text fields using
a dictionary with identifiers, similarly to how we do button identifiers.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:17:
// A container object encapsulating all the state necessary to support an
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> Name quibbles -- I'm not wild about "State", because it implies the state may
> change at some point. Maybe HTTPAuthDialogRequest?

Done.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:21:
// the lifetime of every JavaScriptDialogState.
On 2017/06/14 11:19:16, marq (ping after 24h) wrote:
> HTTPAuthDialogState, here an elsewhere. Careful with that (copy/paste errors)
in
> general.
> 
> Should this class and JSDialogState share a parent class?

Fixed.  I don't think they should share a superclass since the thing shared
between them is that they both have a WebState.  They both have similar
purposes, but are different enough that forcing them to have a superclass would
add complexity in the object hierarchy without actually improving it IMO.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:34:
@property(nonatomic, readonly) NSString* title;
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> You're marking other readonly string properties as strong elsewhere; be
> consistent here.

Done.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.h:42:
// Calls the callback passed on initialization with the provided parameters.
On 2017/06/14 11:19:15, marq (ping after 24h) wrote:
> I regard the callback as an implementation detail. Can it be
> 'completeAuthenticationWith...'.
> 
> The semantics of nil username and password should be documented here.

Done.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm
(right):

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm:45:
DCHECK(webState);
On 2017/06/14 11:19:16, marq (ping after 24h) wrote:
> Document these assertions in the header.

Done.  Should we start adding nullablity annotations to objc selector arguments?

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm:61:
DCHECK(_callbackHasBeenExecuted);
On 2017/06/14 11:19:16, marq (ping after 24h) wrote:
> Would it be correct to run the callback (nil, nil) if it hasn't run at this
> point?

We shouldn't be using both DCHECKs and defensive programming, only one or the
other.  I prefer to keep it as a DCHECK because silently executing the callback
might hide bugs where these coordinators might be deallocated before their
completion blocks are explicitly executed.  Having the DCHECK here is helpful
because it's easier to understand its semantic purpose than if the exception was
thrown by a deallocated WebKit completion block.

https://codereview.chromium.org/2930763003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/dialogs/http_auth_dialogs/http_auth_dialog_state.mm:66:
+ (instancetype)stateWithWebState:(web::WebState*)webState
On 2017/06/14 11:19:17, marq (ping after 24h) wrote:
> Ordering nit: all initializers before dealloc.

The initializer is before |-dealloc|.  This is the factory method, which is part
of the public interface that uses our private designated initializer declared at
the top of this class.