Add JavaScript prompts support to CWVUIDelegate.

Add JavaScript prompts support to CWVUIDelegate.

BUG=696630
Review-Url: https://codereview.chromium.org/2715343002
Cr-Commit-Position: refs/heads/master@{#453536}
Committed: https://chromium.googlesource.com/chromium/src/+/627ab487f2c43ca1721935cc4c4579ef9eff421d

[michaeldo, 2017-02-27 18:19:14]

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

[michaeldo, 2017-02-27 18:19:15]

Note that I added a cancelDialogs on top of the initial API spec. PTAL and
ensure the addition of that method and it's signature is reasonable.

[Eugene But (OOO till 7-30), 2017-02-27 18:30:53]

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
File ios/web_view/internal/web_view_java_script_dialog_presenter.h (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:14: class
WebViewJavaScriptDialogPresenter : public web::JavaScriptDialogPresenter {
Please add comments to all non-trirvial interfaces.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:19: 
// web::JavaScriptDialogPresenter overrides:

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
File ios/web_view/internal/web_view_java_script_dialog_presenter.mm (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:26: if
([ui_delegate_ respondsToSelector:@selector
This is quite a long method. Do you want to move the body of each case into
separate methods?

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:34: completionHandler:(void
(^)(void))handler;
s/handler/completionHandler

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:52: -
(void)cancelDialogsForWebView:(CWVWebView*)webView;
Can we remove this method? It's quite unclear when and why it is called.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
File ios/web_view/shell/shell_view_controller.m (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:233: handler:^(UIAlertAction*
_Nonnull action) {
Is it possible to remove |action| to improve the formatting?

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:253: handler(true);
s/true/YES

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:259: handler(false);
s/false/NO

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:288: textInput =
weakAlert.textFields[0].text;
How about this?:

NSString* textInput = weakAlert.textFields.firstObject.text;
handler(textInput);

[michaeldo, 2017-02-27 21:35:52]

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
File ios/web_view/internal/web_view_java_script_dialog_presenter.h (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:14: class
WebViewJavaScriptDialogPresenter : public web::JavaScriptDialogPresenter {
On 2017/02/27 18:30:52, Eugene But wrote:
> Please add comments to all non-trirvial interfaces.

Done.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:19: 
On 2017/02/27 18:30:52, Eugene But wrote:
> // web::JavaScriptDialogPresenter overrides:

Done.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
File ios/web_view/internal/web_view_java_script_dialog_presenter.mm (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/internal/web_v...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:26: if
([ui_delegate_ respondsToSelector:@selector
On 2017/02/27 18:30:52, Eugene But wrote:
> This is quite a long method. Do you want to move the body of each case into
> separate methods?

Done.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:34: completionHandler:(void
(^)(void))handler;
On 2017/02/27 18:30:52, Eugene But wrote:
> s/handler/completionHandler

Done.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:52: -
(void)cancelDialogsForWebView:(CWVWebView*)webView;
On 2017/02/27 18:30:52, Eugene But wrote:
> Can we remove this method? It's quite unclear when and why it is called.

I have removed this method. It is only called when the webview is closed or
crashes. We can easily re-introduce it if needed in the future.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
File ios/web_view/shell/shell_view_controller.m (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:233: handler:^(UIAlertAction*
_Nonnull action) {
On 2017/02/27 18:30:53, Eugene But wrote:
> Is it possible to remove |action| to improve the formatting?

I get an error without the parameter name, but I can remove the nullability
annotations which cleans up the formatting a bit.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:253: handler(true);
On 2017/02/27 18:30:52, Eugene But wrote:
> s/true/YES

Done.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:259: handler(false);
On 2017/02/27 18:30:53, Eugene But wrote:
> s/false/NO

Done.

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:288: textInput =
weakAlert.textFields[0].text;
On 2017/02/27 18:30:53, Eugene But wrote:
> How about this?:
> 
> NSString* textInput = weakAlert.textFields.firstObject.text;
> handler(textInput);

Done.

[Eugene But (OOO till 7-30), 2017-02-27 22:01:05]

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:52: -
(void)cancelDialogsForWebView:(CWVWebView*)webView;
On 2017/02/27 21:35:51, michaeldo wrote:
> On 2017/02/27 18:30:52, Eugene But wrote:
> > Can we remove this method? It's quite unclear when and why it is called.
> 
> I have removed this method. It is only called when the webview is closed or
> crashes. We can easily re-introduce it if needed in the future.
I still see this method in the header. Please double check.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_java_script_dialog_presenter.h (right):

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:41: // Display
JavaScript alert.
nit: Please place methods before ivars.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:41: // Display
JavaScript alert.
s/Display/Displays

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:46: // Display
JavaScript confirm dialog.
s/Display/Displays

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:51: // Display
JavaScript text prompt.
s/Display/Displays

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_java_script_dialog_presenter.mm (right):

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:27: case
web::JAVASCRIPT_DIALOG_TYPE_ALERT: {
Do you still need braces?

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:40: default:
Please remove default. It is better to get a compiler warning/error if
JavaScriptDialogType is extended with one more entry.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:49: if
([ui_delegate_ respondsToSelector:@selector
Optional nit: do you want to swap if-else block and have an early return? Same
question for other methods.

[michaeldo, 2017-02-27 23:39:16]

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2715343002/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:52: -
(void)cancelDialogsForWebView:(CWVWebView*)webView;
On 2017/02/27 22:01:05, Eugene But wrote:
> On 2017/02/27 21:35:51, michaeldo wrote:
> > On 2017/02/27 18:30:52, Eugene But wrote:
> > > Can we remove this method? It's quite unclear when and why it is called.
> > 
> > I have removed this method. It is only called when the webview is closed or
> > crashes. We can easily re-introduce it if needed in the future.
> I still see this method in the header. Please double check.

Done.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_java_script_dialog_presenter.h (right):

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:41: // Display
JavaScript alert.
On 2017/02/27 22:01:05, Eugene But wrote:
> s/Display/Displays

Done.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:46: // Display
JavaScript confirm dialog.
On 2017/02/27 22:01:05, Eugene But wrote:
> s/Display/Displays

Done.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:51: // Display
JavaScript text prompt.
On 2017/02/27 22:01:05, Eugene But wrote:
> s/Display/Displays

Done.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_java_script_dialog_presenter.mm (right):

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:27: case
web::JAVASCRIPT_DIALOG_TYPE_ALERT: {
On 2017/02/27 22:01:05, Eugene But wrote:
> Do you still need braces? 

Nope. Removed.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:40: default:
On 2017/02/27 22:01:05, Eugene But wrote:
> Please remove default. It is better to get a compiler warning/error if
> JavaScriptDialogType is extended with one more entry.

Done.

https://codereview.chromium.org/2715343002/diff/40001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.mm:49: if
([ui_delegate_ respondsToSelector:@selector
On 2017/02/27 22:01:05, Eugene But wrote:
> Optional nit: do you want to swap if-else block and have an early return? Same
> question for other methods.

Done.

[Eugene But (OOO till 7-30), 2017-02-28 01:18:59]

lgtm!

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_java_script_dialog_presenter.h (right):

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:9: #include
"ios/web/public/java_script_dialog_presenter.h"
s/include/import

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_ui_delegate.h:35: // Instructs the delegate to show UI
in response to
nit:

// Instructs the delegate to show UI in response to window.confirm JavaScript
// call.

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_ui_delegate.h:42: // Instructs the delegate to show UI
in response to window.prompt
nit: 

// Instructs the delegate to show UI in response to window.prompt JavaScript
// call.

[michaeldo, 2017-02-28 06:10:38]

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/internal/w...
File ios/web_view/internal/web_view_java_script_dialog_presenter.h (right):

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/internal/w...
ios/web_view/internal/web_view_java_script_dialog_presenter.h:9: #include
"ios/web/public/java_script_dialog_presenter.h"
On 2017/02/28 01:18:58, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_ui_delegate.h:35: // Instructs the delegate to show UI
in response to
On 2017/02/28 01:18:58, Eugene But wrote:
> nit:
> 
> // Instructs the delegate to show UI in response to window.confirm JavaScript
> // call.

Done.

https://codereview.chromium.org/2715343002/diff/60001/ios/web_view/public/cwv...
ios/web_view/public/cwv_ui_delegate.h:42: // Instructs the delegate to show UI
in response to window.prompt
On 2017/02/28 01:18:58, Eugene But wrote:
> nit: 
> 
> // Instructs the delegate to show UI in response to window.prompt JavaScript
> // call.

Done.

[michaeldo, 2017-02-28 06:12:36]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-02-28 06:12:37]

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/2715343002/#ps80001
(title: "Respond to comments.")

[commit-bot: I haz the power, 2017-02-28 06:13:05]

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, 2017-02-28 07:20:32]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488262356713340,
"parent_rev": "41ca6f1ea85eaf0105769ba88dd51e0b1aab6f3d", "commit_rev":
"627ab487f2c43ca1721935cc4c4579ef9eff421d"}

[commit-bot: I haz the power, 2017-02-28 07:21:08]

Description was changed from

==========
Add JavaScript prompts support to CWVUIDelegate.

BUG=696630
==========

to

==========
Add JavaScript prompts support to CWVUIDelegate.

BUG=696630

Review-Url: https://codereview.chromium.org/2715343002
Cr-Commit-Position: refs/heads/master@{#453536}
Committed:
https://chromium.googlesource.com/chromium/src/+/627ab487f2c43ca1721935cc4c45...
==========

[commit-bot: I haz the power, 2017-02-28 07:21:09]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/627ab487f2c43ca1721935cc4c45...