Add public API for handling Javascript alerts and prompt.

Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

Proposal Link:
https://docs.google.com/document/d/1d3PxHcDGyZlrgEBa58oh-eFFOWUYLOv8kIiuftFjli8/edit

BUG=620739
Committed: https://crrev.com/e9e6159b0f99159b11f7d74d32bfeb81d14cbb1f
Cr-Commit-Position: refs/heads/master@{#402617}

[michaeldo, 2016-06-16 15:08:31]

michaeldo@chromium.org changed reviewers:
+ eugenebut@chromium.org

[michaeldo, 2016-06-16 20:17:07]

Description was changed from

==========
Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

BUG=620739
==========

to

==========
Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

Proposal Link:
https://docs.google.com/document/d/1d3PxHcDGyZlrgEBa58oh-eFFOWUYLOv8kIiuftFjl...

BUG=620739
==========

[marq (ping after 24h), 2016-06-24 14:09:11]

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

[Eugene But (OOO till 7-30), 2016-06-24 17:04:53]

Sorry I missed something big in design doc. HTTP Authentication does not
necessarily require the dialog (please see specific comment below).

https://codereview.chromium.org/2074733002/diff/80001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/BUILD.gn#newcod...
ios/web/BUILD.gn:110: "public/javascript_message_type.h",
s/message/dialog ?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
File ios/web/public/dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:24: // responded with OK, |false| if the
prompt was cancelled. The |user_input|
s/prompt/prompt or confirm ?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:28: // Displays a Javascript dialog.  If a
dialog is already being presented, the
Please do no assume here how presentation is implemented (queued). Also you
should not assume that this method will actually present the dialog, because
they are optional according to html spec. How about this?:

"Requests presentation of a JavaScript dialog. Clients must always call
|callback| even if they choose not to present the dialog".

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:32: const GURL& origin_url,
Should this be just origin? There is no actual way to determine the whole URL.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:41: // Displays a dialog prompting for a
username and password. If a dialog is
Sorry, I just realized that DialogPresenter should not be used for HTTP Auth.
Clients are not obligated to show the dialog, they free to use pre-saved
credentials (even though Chrome does not support that) so it's not necessarily
about dialog presentation. We need go back to design doc on this, sorry for not
noticing this earlier.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate.h:43: // WebDependencyFactory along
with the analogous method in content//.
NIT: We don't know if WebDependencyFactory will exist, please use more generic
comment, like "Find better place for this method."

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate_bridge.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate_bridge.h:30: -
(web::DialogPresenter*)dialogPresenterForWebState:(web::WebState*)webState;
Could you please leave the line-break

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:856: // Helper to respond to
|webView:runJavaScript...| delegate methods.
Please document whether or not |completionHandler| can be nil. I think if you
disallow nil, then the code will be simpler.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3784: completion:(void (^)(BOOL,
NSString*))completionHandler {
I think if you always pass |completionHandler| the code will be simpler. WDYT?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3787: callback = [completionHandler
copy];
Why do you need a separate |callback| variable? Can you just always use
completionHandler?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_stub.mm (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_stub.mm:7: #import
"ios/web/public/web_state/context_menu_params.h"
Could you please land this change separately :)

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:276: DialogClosedCallback;
This looks exactly like a callback from DialogPresenter. Is there a way to share
the code?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:277: // Notifies the delegate that a
Javascript message needs handling.
s/Javascript message needs handling/JavaScript dialog needs to be presented ?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:278: void RunJavaScriptDialog(const GURL&
origin_URL,
s/origin_URL/origin_url

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:287: AuthDialogCallback;
Same question. Is there a way to share this code?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:447: }
I would expect a crash if GetDialogPresenter returns nil. Should this code be
like this:

DialogPresenter* presenter = delegate_ ? delegate_->GetDialogPresenter(this) :
nullptr;
if (!presenter) {
  callback.Run(false, nil);
  return;
}

presenter->RunJavaScriptDialog(this, origin_url, javascript_message_type,
message_text, default_prompt_text, callback);

[michaeldo, 2016-06-27 20:30:10]

https://codereview.chromium.org/2074733002/diff/80001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/BUILD.gn#newcod...
ios/web/BUILD.gn:110: "public/javascript_message_type.h",
On 2016/06/24 17:04:52, Eugene But wrote:
> s/message/dialog ?

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
File ios/web/public/dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:24: // responded with OK, |false| if the
prompt was cancelled. The |user_input|
On 2016/06/24 17:04:52, Eugene But wrote:
> s/prompt/prompt or confirm ?

Replaced with "dialog".

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:28: // Displays a Javascript dialog.  If a
dialog is already being presented, the
On 2016/06/24 17:04:52, Eugene But wrote:
> Please do no assume here how presentation is implemented (queued). Also you
> should not assume that this method will actually present the dialog, because
> they are optional according to html spec. How about this?:
> 
> "Requests presentation of a JavaScript dialog. Clients must always call
> |callback| even if they choose not to present the dialog".

yes, this is much better since this is just the public header not the
implementation.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:32: const GURL& origin_url,
On 2016/06/24 17:04:52, Eugene But wrote:
> Should this be just origin? There is no actual way to determine the whole URL.

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:41: // Displays a dialog prompting for a
username and password. If a dialog is
On 2016/06/24 17:04:52, Eugene But wrote:
> Sorry, I just realized that DialogPresenter should not be used for HTTP Auth.
> Clients are not obligated to show the dialog, they free to use pre-saved
> credentials (even though Chrome does not support that) so it's not necessarily
> about dialog presentation. We need go back to design doc on this, sorry for
not
> noticing this earlier.

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate.h:43: // WebDependencyFactory along
with the analogous method in content//.
On 2016/06/24 17:04:52, Eugene But wrote:
> NIT: We don't know if WebDependencyFactory will exist, please use more generic
> comment, like "Find better place for this method."

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate_bridge.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate_bridge.h:30: -
(web::DialogPresenter*)dialogPresenterForWebState:(web::WebState*)webState;
On 2016/06/24 17:04:52, Eugene But wrote:
> Could you please leave the line-break

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:856: // Helper to respond to
|webView:runJavaScript...| delegate methods.
On 2016/06/24 17:04:52, Eugene But wrote:
> Please document whether or not |completionHandler| can be nil. I think if you
> disallow nil, then the code will be simpler.

you're right, made it non-nil.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3784: completion:(void (^)(BOOL,
NSString*))completionHandler {
On 2016/06/24 17:04:52, Eugene But wrote:
> I think if you always pass |completionHandler| the code will be simpler. WDYT?

Agreed, made the completion handler required per your above recommendation.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3787: callback = [completionHandler
copy];
On 2016/06/24 17:04:52, Eugene But wrote:
> Why do you need a separate |callback| variable? Can you just always use
> completionHandler? 

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_stub.mm (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_stub.mm:7: #import
"ios/web/public/web_state/context_menu_params.h"
On 2016/06/24 17:04:52, Eugene But wrote:
> Could you please land this change separately :)

will do :)

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:276: DialogClosedCallback;
On 2016/06/24 17:04:53, Eugene But wrote:
> This looks exactly like a callback from DialogPresenter. Is there a way to
share
> the code?

I went with the duplication based on other callbacks, but I like the shared
definition better too. I moved it into javascript_dialog_callback.h in
/ios/web/public

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:277: // Notifies the delegate that a
Javascript message needs handling.
On 2016/06/24 17:04:52, Eugene But wrote:
> s/Javascript message needs handling/JavaScript dialog needs to be presented ?

Done.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:278: void RunJavaScriptDialog(const GURL&
origin_URL,
On 2016/06/24 17:04:53, Eugene But wrote:
> s/origin_URL/origin_url

replaced with "origin" based on your previous comment.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:287: AuthDialogCallback;
On 2016/06/24 17:04:52, Eugene But wrote:
> Same question. Is there a way to share this code?

This code is no longer part of this CL.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:447: }
On 2016/06/24 17:04:53, Eugene But wrote:
> I would expect a crash if GetDialogPresenter returns nil. Should this code be
> like this:
> 
> DialogPresenter* presenter = delegate_ ? delegate_->GetDialogPresenter(this) :
> nullptr;
> if (!presenter) {
>   callback.Run(false, nil);
>   return;
> }
> 
> presenter->RunJavaScriptDialog(this, origin_url, javascript_message_type,
> message_text, default_prompt_text, callback);

Done.

[Eugene But (OOO till 7-30), 2016-06-27 21:09:37]

Can you add unit tests for this new code?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
File ios/web/public/dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:32: const GURL& origin_url,
On 2016/06/27 20:30:09, michaeldo wrote:
> On 2016/06/24 17:04:52, Eugene But wrote:
> > Should this be just origin? There is no actual way to determine the whole
URL.
> 
> Done.

Sorry for misleading you. You are actually passing the whole URL. Old name was
good.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:144: 'public/javascript_dialog_callback.h',
NIT: I think per style guide these should be java_script instead of javascript.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
File ios/web/public/javascript_dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
ios/web/public/javascript_dialog_presenter.h:24: JavaScriptDialogType
javascript_dialog_type,
NIT: s/java_script_dialog_type/javascript_dialog_type

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
ios/web/public/javascript_dialog_presenter.h:28: // Cancels the display of any
currently displayed dialog and any queued
Please do not assume that clients queue these dialogs. How about this:

"Informs clients that all requested dialogs associated with |web_state| should
be dismissed."

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
File ios/web/public/javascript_dialog_type.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
ios/web/public/javascript_dialog_type.h:13: JAVASCRIPT_DIALOG_TYPE_ALERT,
NIT: Could you please initialize this with non-zero value. 0 is default in
Objective-C and it may be passed by mistake.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/web_sta...
File ios/web/public/web_state/web_state_delegate_bridge.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/web_sta...
ios/web/public/web_state/web_state_delegate_bridge.h:28: // Returns a pointer to
a service to manage dialogs. May return nil in which
NIT: s/nil/null nil is a specific value for Objective-C objections, while |null|
is idiomatic thing with mean NULL, nullptr, 0 and other stuff.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:58: #include
"ios/web/public/javascript_dialog_presenter.h"
s/include/import

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:3781: completionHandler =
[completionHandler copy];
Why do you need this? |completionHandler| should be retained by the block and
copied in chrome layer.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:4871: if (completionHandler) {
Optional NIT: We never had crashes here, so I believe we can remove nil checks
for completion handler (we not doing them everywhere anyways).

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:272: // Notifies the delegate that a
Javascript dialog needs to be presented.
NIT: s/Javascript/JavaScript

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:274: JavaScriptDialogType
javascript_dialog_type,
NIT: s/javascript_dialog_type/java_script_dialog_type

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:279: // Cancels the display of any currently
displayed dialog and any queued
Please do not make assumptions that dialogs are queued.

[michaeldo, 2016-06-27 22:25:10]

https://codereview.chromium.org/2074733002/diff/120001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:144: 'public/javascript_dialog_callback.h',
On 2016/06/27 21:09:37, Eugene But wrote:
> NIT: I think per style guide these should be java_script instead of
javascript.

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
File ios/web/public/javascript_dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
ios/web/public/javascript_dialog_presenter.h:24: JavaScriptDialogType
javascript_dialog_type,
On 2016/06/27 21:09:37, Eugene But wrote:
> NIT: s/java_script_dialog_type/javascript_dialog_type

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
ios/web/public/javascript_dialog_presenter.h:28: // Cancels the display of any
currently displayed dialog and any queued
On 2016/06/27 21:09:37, Eugene But wrote:
> Please do not assume that clients queue these dialogs. How about this:
> 
> "Informs clients that all requested dialogs associated with |web_state| should
> be dismissed."

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
File ios/web/public/javascript_dialog_type.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/javascr...
ios/web/public/javascript_dialog_type.h:13: JAVASCRIPT_DIALOG_TYPE_ALERT,
On 2016/06/27 21:09:37, Eugene But wrote:
> NIT: Could you please initialize this with non-zero value. 0 is default in
> Objective-C and it may be passed by mistake.

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/web_sta...
File ios/web/public/web_state/web_state_delegate_bridge.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/public/web_sta...
ios/web/public/web_state/web_state_delegate_bridge.h:28: // Returns a pointer to
a service to manage dialogs. May return nil in which
On 2016/06/27 21:09:37, Eugene But wrote:
> NIT: s/nil/null nil is a specific value for Objective-C objections, while
|null|
> is idiomatic thing with mean NULL, nullptr, 0 and other stuff.

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:58: #include
"ios/web/public/javascript_dialog_presenter.h"
On 2016/06/27 21:09:37, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:3781: completionHandler =
[completionHandler copy];
On 2016/06/27 21:09:37, Eugene But wrote:
> Why do you need this? |completionHandler| should be retained by the block and
> copied in chrome layer.

You are correct, I don't need this copy.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:4871: if (completionHandler) {
On 2016/06/27 21:09:37, Eugene But wrote:
> Optional NIT: We never had crashes here, so I believe we can remove nil checks
> for completion handler (we not doing them everywhere anyways).

The if check was here, I simply moved it from the else if below to here. I do
believe the if check is required. (Ref: Note at bottom of Block Syntax section:
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra...)

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:272: // Notifies the delegate that a
Javascript dialog needs to be presented.
On 2016/06/27 21:09:37, Eugene But wrote:
> NIT: s/Javascript/JavaScript

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:274: JavaScriptDialogType
javascript_dialog_type,
On 2016/06/27 21:09:37, Eugene But wrote:
> NIT: s/javascript_dialog_type/java_script_dialog_type

Done.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:279: // Cancels the display of any currently
displayed dialog and any queued
On 2016/06/27 21:09:37, Eugene But wrote:
> Please do not make assumptions that dialogs are queued.

Done.

[Eugene But (OOO till 7-30), 2016-06-27 22:33:56]

Per my previous comment, is it possible to add unit tests?

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
File ios/web/public/dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:32: const GURL& origin_url,
On 2016/06/27 21:09:37, Eugene But wrote:
> On 2016/06/27 20:30:09, michaeldo wrote:
> > On 2016/06/24 17:04:52, Eugene But wrote:
> > > Should this be just origin? There is no actual way to determine the whole
> URL.
> > 
> > Done.
> 
> Sorry for misleading you. You are actually passing the whole URL. Old name was
> good.
I guess you missed this comment.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:4871: if (completionHandler) {
On 2016/06/27 22:25:10, michaeldo wrote:
> On 2016/06/27 21:09:37, Eugene But wrote:
> > Optional NIT: We never had crashes here, so I believe we can remove nil
checks
> > for completion handler (we not doing them everywhere anyways).
> 
> The if check was here, I simply moved it from the else if below to here. I do
> believe the if check is required. (Ref: Note at bottom of Block Syntax
section:
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra...)
Right, so if block is nil then we'll crash. But according to
NS_ASSUME_NONNULL_BEGIN in WKNavigationDelegate.h this block will never be nil.

[michaeldo, 2016-06-28 17:44:47]

I've updated the existing unittests to cover the new functionality. Please let
me know if you'd like to test them more in depth. I do have tests for the
JavaScriptDialogPresenter downstream with the implementation.

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
File ios/web/public/dialog_presenter.h (right):

https://codereview.chromium.org/2074733002/diff/80001/ios/web/public/dialog_p...
ios/web/public/dialog_presenter.h:32: const GURL& origin_url,
On 2016/06/27 22:33:56, Eugene But wrote:
> On 2016/06/27 21:09:37, Eugene But wrote:
> > On 2016/06/27 20:30:09, michaeldo wrote:
> > > On 2016/06/24 17:04:52, Eugene But wrote:
> > > > Should this be just origin? There is no actual way to determine the
whole
> > URL.
> > > 
> > > Done.
> > 
> > Sorry for misleading you. You are actually passing the whole URL. Old name
was
> > good.
> I guess you missed this comment. 

Sorry, yes I believe I missed it, fixed now.

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/120001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:4871: if (completionHandler) {
On 2016/06/27 22:33:56, Eugene But wrote:
> On 2016/06/27 22:25:10, michaeldo wrote:
> > On 2016/06/27 21:09:37, Eugene But wrote:
> > > Optional NIT: We never had crashes here, so I believe we can remove nil
> checks
> > > for completion handler (we not doing them everywhere anyways).
> > 
> > The if check was here, I simply moved it from the else if below to here. I
do
> > believe the if check is required. (Ref: Note at bottom of Block Syntax
> section:
> >
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra...)
> Right, so if block is nil then we'll crash. But according to
> NS_ASSUME_NONNULL_BEGIN in WKNavigationDelegate.h this block will never be
nil.

Yes, you are right! I'll remove the check.

[Eugene But (OOO till 7-30), 2016-06-28 18:14:49]

LGTM thanks for tests!

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:3781:
net::GURLWithNSURL(frame.request.URL), type, message, defaultText,
Could you please test that this works correctly for iframes (and gives iframe
URL)

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
File ios/web/web_state/web_state_delegate_bridge.mm (right):

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
ios/web/web_state/web_state_delegate_bridge.mm:35: respondsToSelector:@selector(
Optional NIT: Could you please move SEL to a separate variable to improve
formatting.

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl_unittest.mm (right):

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl_unittest.mm:156: return nullptr;
How about adding SetJavaScriptDialogPresenter and making sure that
web_state_->RunJavaScriptDialog actually calls appropriate method on
JavaScriptDialogPresenter? We can do better than just verifying that delegate
method was called.

[michaeldo, 2016-06-28 21:58:27]

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:3781:
net::GURLWithNSURL(frame.request.URL), type, message, defaultText,
On 2016/06/28 18:14:48, Eugene But wrote:
> Could you please test that this works correctly for iframes (and gives iframe
> URL)

Yes, I will test that it works correctly.

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
File ios/web/web_state/web_state_delegate_bridge.mm (right):

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
ios/web/web_state/web_state_delegate_bridge.mm:35: respondsToSelector:@selector(
On 2016/06/28 18:14:48, Eugene But wrote:
> Optional NIT: Could you please move SEL to a separate variable to improve
> formatting.

Done.

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl_unittest.mm (right):

https://codereview.chromium.org/2074733002/diff/160001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl_unittest.mm:156: return nullptr;
On 2016/06/28 18:14:48, Eugene But wrote:
> How about adding SetJavaScriptDialogPresenter and making sure that
> web_state_->RunJavaScriptDialog actually calls appropriate method on
> JavaScriptDialogPresenter? We can do better than just verifying that delegate
> method was called.

I like that idea. Done.

[michaeldo, 2016-06-28 22:03:18]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2016-06-28 22:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2074733002/#ps180001
(title: "Add JavaScriptDialogPresenter implementation.")

[commit-bot: I haz the power, 2016-06-28 22:06:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 22:25:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 22:26:00]

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...)

[michaeldo, 2016-06-28 22:30:38]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2016-06-28 22:30:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2074733002/#ps200001
(title: "Replace BOOL with bool in unittest.")

[commit-bot: I haz the power, 2016-06-28 22:34:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 00:39:39]

Description was changed from

==========
Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

Proposal Link:
https://docs.google.com/document/d/1d3PxHcDGyZlrgEBa58oh-eFFOWUYLOv8kIiuftFjl...

BUG=620739
==========

to

==========
Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

Proposal Link:
https://docs.google.com/document/d/1d3PxHcDGyZlrgEBa58oh-eFFOWUYLOv8kIiuftFjl...

BUG=620739
==========

[commit-bot: I haz the power, 2016-06-29 00:39:40]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-29 00:42:02]

Description was changed from

==========
Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

Proposal Link:
https://docs.google.com/document/d/1d3PxHcDGyZlrgEBa58oh-eFFOWUYLOv8kIiuftFjl...

BUG=620739
==========

to

==========
Add public API for handling Javascript alerts and prompt.

DialogPresenter provides an abstract interface for handling JavaScript
alerts and the HTTP Authentication prompt. A subclass can be provided
through the WebStateDelegate.

Proposal Link:
https://docs.google.com/document/d/1d3PxHcDGyZlrgEBa58oh-eFFOWUYLOv8kIiuftFjl...

BUG=620739

Committed: https://crrev.com/e9e6159b0f99159b11f7d74d32bfeb81d14cbb1f
Cr-Commit-Position: refs/heads/master@{#402617}
==========

[commit-bot: I haz the power, 2016-06-29 00:42:03]

Patchset 11 (id:??) landed as
https://crrev.com/e9e6159b0f99159b11f7d74d32bfeb81d14cbb1f
Cr-Commit-Position: refs/heads/master@{#402617}