Add window open and close delegate methods to CWVUIDelegate.

Add window open and close delegate methods to CWVUIDelegate.

BUG=697061
Review-Url: https://codereview.chromium.org/2757603003
Cr-Commit-Position: refs/heads/master@{#457559}
Committed: https://chromium.googlesource.com/chromium/src/+/93b10af49dc4f680debc7fa2274bd07a55c6492f

[michaeldo, 2017-03-16 15:07:05]

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

[michaeldo, 2017-03-16 15:07:19]

Description was changed from

==========
Add window open and close delegate methods to CWVUIDelegate	.

BUG=697061
==========

to

==========
Add window open and close delegate methods to CWVUIDelegate.

BUG=697061
==========

[Eugene But (OOO till 7-30), 2017-03-16 17:09:03]

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_n...
File ios/web_view/internal/cwv_navigation_action_internal.h (right):

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_n...
ios/web_view/internal/cwv_navigation_action_internal.h:14: - (nonnull
instancetype)initWithRequest:(nonnull NSURLRequest*)hyperlink
s/hyperlink/request

This request may be initiated by window.open, which is not a hyperlink.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:54: //@property(nonatomic, readonly)
web::WebState estimatedProgress;
Please remove this

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:251:
forNavigationAction:navigationAction];
This will create a non-closable window. WebState can only be closed if it was
open by DOM. Calling NavigationManagerImpl::InitializeSession(YES) would solve
your problem, but Kurt is removing opened_by_dom flag:
https://codereview.chromium.org/2755823002/

I think it's ok to land your CL with TODO and fix this problem separately after
cl/2755823002 lands.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:252: return webView->_webState.get();
This will crash if |webView| is nil.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:257: if (![_UIDelegate
respondsToSelector:selector]) {
nit: How about this?:

if ([_UIDelegate respondsToSelector:@selector(webViewDidClose:)]) {
  [_UIDelegate webViewDidClose:self];
}

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
File ios/web_view/public/cwv_navigation_action.h (right):

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
ios/web_view/public/cwv_navigation_action.h:9: 
Do you want to use nullability annotation?

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
ios/web_view/public/cwv_navigation_action.h:14: @property(copy, readonly)
NSURLRequest* request;
nonatomic

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
ios/web_view/public/cwv_navigation_action.h:16: @property(readonly) BOOL
userInitiated;
@property(nonatomic, readonly, getter=isUserInitiated) BOOL userInitiated;

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

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:22: // window.open JavaScript call).
Add to the comments: "Page will not open a window if this method returns nil or
not implemented. This method can not return |webView|."

[michaeldo, 2017-03-16 18:24:28]

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_n...
File ios/web_view/internal/cwv_navigation_action_internal.h (right):

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_n...
ios/web_view/internal/cwv_navigation_action_internal.h:14: - (nonnull
instancetype)initWithRequest:(nonnull NSURLRequest*)hyperlink
On 2017/03/16 17:09:02, Eugene But wrote:
> s/hyperlink/request
> 
> This request may be initiated by window.open, which is not a hyperlink.

Sorry, must've been a copy/paste error. Fixed.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:54: //@property(nonatomic, readonly)
web::WebState estimatedProgress;
On 2017/03/16 17:09:02, Eugene But wrote:
> Please remove this

Done.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:251:
forNavigationAction:navigationAction];
On 2017/03/16 17:09:02, Eugene But wrote:
> This will create a non-closable window. WebState can only be closed if it was
> open by DOM. Calling NavigationManagerImpl::InitializeSession(YES) would solve
> your problem, but Kurt is removing opened_by_dom flag:
> https://codereview.chromium.org/2755823002/
> 
> I think it's ok to land your CL with TODO and fix this problem separately
after
> cl/2755823002 lands.

Thank you for the details, I have created a bug and will followup once that CL
lands.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:252: return webView->_webState.get();
On 2017/03/16 17:09:02, Eugene But wrote:
> This will crash if |webView| is nil.

I've added a check that a webview was in fact returned.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:257: if (![_UIDelegate
respondsToSelector:selector]) {
On 2017/03/16 17:09:02, Eugene But wrote:
> nit: How about this?:
> 
> if ([_UIDelegate respondsToSelector:@selector(webViewDidClose:)]) {
>   [_UIDelegate webViewDidClose:self];
> }

Done.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
File ios/web_view/public/cwv_navigation_action.h (right):

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
ios/web_view/public/cwv_navigation_action.h:9: 
On 2017/03/16 17:09:02, Eugene But wrote:
> Do you want to use nullability annotation?

yes, thank you for reminder.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
ios/web_view/public/cwv_navigation_action.h:14: @property(copy, readonly)
NSURLRequest* request;
On 2017/03/16 17:09:03, Eugene But wrote:
> nonatomic

Done.

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_nav...
ios/web_view/public/cwv_navigation_action.h:16: @property(readonly) BOOL
userInitiated;
On 2017/03/16 17:09:02, Eugene But wrote:
> @property(nonatomic, readonly, getter=isUserInitiated) BOOL userInitiated;

Done.

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

https://codereview.chromium.org/2757603003/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:22: // window.open JavaScript call).
On 2017/03/16 17:09:03, Eugene But wrote:
> Add to the comments: "Page will not open a window if this method returns nil
or
> not implemented. This method can not return |webView|."

Done.

[Eugene But (OOO till 7-30), 2017-03-16 18:40:01]

FYI: there is no new patch :)

[michaeldo, 2017-03-16 18:43:59]

On 2017/03/16 18:40:01, Eugene But wrote:
> FYI: there is no new patch :)

I was working on uploading it still :) PTAL now.

[Eugene But (OOO till 7-30), 2017-03-16 18:48:56]

lgtm

https://codereview.chromium.org/2757603003/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2757603003/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:238: return nil;
s/nil/nullptr

https://codereview.chromium.org/2757603003/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:250: if (webView) {
Optional nit:

Do you want to swap conditions for consistency?:
if (!webView) {
  return nullptr;
}

return webView->_webState.get();

[michaeldo, 2017-03-16 20:25:39]

https://codereview.chromium.org/2757603003/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2757603003/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:238: return nil;
On 2017/03/16 18:48:56, Eugene But wrote:
> s/nil/nullptr

Done.

https://codereview.chromium.org/2757603003/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_web_view.mm:250: if (webView) {
On 2017/03/16 18:48:55, Eugene But wrote:
> Optional nit:
> 
> Do you want to swap conditions for consistency?:
> if (!webView) {
>   return nullptr;
> }
> 
> return webView->_webState.get();

Done.

[michaeldo, 2017-03-16 20:25:44]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-03-16 20:25:44]

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

[commit-bot: I haz the power, 2017-03-16 20:26:10]

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-03-16 21:28:08]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489695944162410,
"parent_rev": "37974aacf99394a06029f545511eb5d0022bec03", "commit_rev":
"93b10af49dc4f680debc7fa2274bd07a55c6492f"}

[commit-bot: I haz the power, 2017-03-16 21:28:57]

Description was changed from

==========
Add window open and close delegate methods to CWVUIDelegate.

BUG=697061
==========

to

==========
Add window open and close delegate methods to CWVUIDelegate.

BUG=697061

Review-Url: https://codereview.chromium.org/2757603003
Cr-Commit-Position: refs/heads/master@{#457559}
Committed:
https://chromium.googlesource.com/chromium/src/+/93b10af49dc4f680debc7fa2274b...
==========

[commit-bot: I haz the power, 2017-03-16 21:28:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/93b10af49dc4f680debc7fa2274b...