Refactor callbacks for opening a new window.

Refactored callbacks for opening a new window.

This CL replaces 4 callbacks:
 webPageOrderedOpen:referrer:windowName:inBackground:
 webPageOrderedOpen
 webController:shouldBlockPopupWithURL:sourceURL:
 webController:didBlockPopup:

with a single callback:
 webController:createWebControllerForURL:openerURL:initiatedByUser:
 
This makes ios code more similar to other platforms where windows
are open via single WebContentsDelegate::AddNewContents callback 
and all popup blocking logic lives in chrome layer.

This is a precursory CL to move window opening callback to
WebStateDelegate.


BUG=622072, 674991
Review-Url: https://codereview.chromium.org/2692803004
Cr-Commit-Position: refs/heads/master@{#451778}
Committed: https://chromium.googlesource.com/chromium/src/+/91cfb3ad9a9b9502aed2f9bc76c5d3cc96105f22

[Eugene But (OOO till 7-30), 2017-02-13 22:57:48]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 22:59:13]

Dry run: 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-13 23:08:36]

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

[commit-bot: I haz the power, 2017-02-13 23:08:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

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

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 23:28:04]

Dry run: 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-13 23:59:14]

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

[commit-bot: I haz the power, 2017-02-13 23:59:15]

Dry run: 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...)

[Eugene But (OOO till 7-30), 2017-02-14 03:54:59]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 03:55:34]

Dry run: 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-14 04:09:45]

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

[commit-bot: I haz the power, 2017-02-14 04:09:45]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-02-14 17:55:18]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 17:55:57]

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

[Eugene But (OOO till 7-30), 2017-02-14 18:05:56]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 18:06:27]

Dry run: 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-14 18:24:20]

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

[commit-bot: I haz the power, 2017-02-14 18:24:21]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-02-14 18:49:03]

Description was changed from

==========
Refactor callbacks for opening a new window.

BUG=622072,674991
==========

to

==========
Refactored callbacks for opening a new window.

This CL replaces 4 callbacks:
 webPageOrderedOpen:referrer:windowName:inBackground:
 webPageOrderedOpen
 webController:shouldBlockPopupWithURL:sourceURL:
 webController:didBlockPopup:

with a single callback:
 webController:createWebControllerForURL:openerURL:initiatedByUser:
 
This makes ios code more similar to other platforms where windows
are open via single WebContentsDelegate::AddNewContents callback 
and all popup blocking logic lives in chrome layer.

This is a precursory CL to move window opening callback to
WebStateDelegate.


BUG=622072,674991
==========

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

eugenebut@chromium.org changed reviewers:
+ marq@chromium.org, rohitrao@chromium.org

[marq (ping after 24h), 2017-02-15 10:34:10]

https://codereview.chromium.org/2692803004/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2692803004/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4310: base::WeakNSObject<Tab>
weakTab(tab);
Since the block now only cares about [tab url], instead of doing weak/strong on
the tab, can we just grab [tab url] outside of the block and directly use that
in the block, and remove |tab| from the block altogether?

Likewise, instead of weakSelf, can we grab weakTabModel([self tabModel) and
directly use that in the block?

https://codereview.chromium.org/2692803004/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/ui/crw_web_delegate.h (right):

https://codereview.chromium.org/2692803004/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/ui/crw_web_delegate.h:92:
createWebControllerForURL:(const GURL&)URL
Naming nit: Delegate methods that return something don't usually include a verb
like 'create' -- compare to tableView:cellForRowAtIndexPath:. I think
webController:webControllerForURL:openerURL:initiatedByUser: is fine, and it
doesn't imply that the delegate necessarily has to "create" a new webController
(although they very likely will).

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

https://codereview.chromium.org/2692803004/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4366: NSString* referer = [self
refererFromNavigationAction:action];
We should be consistent in our spelling of 'referrer'; either we should match
the (misspelled) HTTP header name everywhere ('referer'), or we should use
"referrer" everywhere.

[Eugene But (OOO till 7-30), 2017-02-15 16:46:59]

Thanks. PTAL

https://codereview.chromium.org/2692803004/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2692803004/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4310: base::WeakNSObject<Tab>
weakTab(tab);
On 2017/02/15 10:34:10, marq wrote:
> Since the block now only cares about [tab url], instead of doing weak/strong
on
> the tab, can we just grab [tab url] outside of the block and directly use that
> in the block, and remove |tab| from the block altogether?
> 
> Likewise, instead of weakSelf, can we grab weakTabModel([self tabModel) and
> directly use that in the block?
This block actually uses |strongTab| on line 4328, so I can't use just URL.

Also I would still use weakSelf even if block needs only tabModel to avoid
assumptions about tabModel lifetime and because it's a common pattern to use
weakSelf. If for some reason |tabModel| outlives BVC, then this code will still
work as expected, but using weakTabModel may lead to unexpected behavior. 

WDYT?

https://codereview.chromium.org/2692803004/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/ui/crw_web_delegate.h (right):

https://codereview.chromium.org/2692803004/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/ui/crw_web_delegate.h:92:
createWebControllerForURL:(const GURL&)URL
On 2017/02/15 10:34:10, marq wrote:
> Naming nit: Delegate methods that return something don't usually include a
verb
> like 'create' -- compare to tableView:cellForRowAtIndexPath:. I think
> webController:webControllerForURL:openerURL:initiatedByUser: is fine, and it
> doesn't imply that the delegate necessarily has to "create" a new
webController
> (although they very likely will).
Modeled from
|webView:createWebViewWithConfiguration:forNavigationAction:windowFeatures:|
which I think appropriately has "create" word, because there is a concrete side
effect for this action (creating a new UI window). In any case, this method will
be removed in subsequent CL in favor or WebStateDelegate method, so I would
prefer to leave it as it is.

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

https://codereview.chromium.org/2692803004/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4366: NSString* referer = [self
refererFromNavigationAction:action];
On 2017/02/15 10:34:10, marq wrote:
> We should be consistent in our spelling of 'referrer'; either we should match
> the (misspelled) HTTP header name everywhere ('referer'), or we should use
> "referrer" everywhere. 

Done.

[Eugene But (OOO till 7-30), 2017-02-16 02:32:04]

Rohit?

[marq (ping after 24h), 2017-02-16 12:49:44]

lgtm

https://codereview.chromium.org/2692803004/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2692803004/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4310: base::WeakNSObject<Tab>
weakTab(tab);
On 2017/02/15 16:46:59, Eugene But wrote:
> On 2017/02/15 10:34:10, marq wrote:
> > Since the block now only cares about [tab url], instead of doing weak/strong
> on
> > the tab, can we just grab [tab url] outside of the block and directly use
that
> > in the block, and remove |tab| from the block altogether?
> > 
> > Likewise, instead of weakSelf, can we grab weakTabModel([self tabModel) and
> > directly use that in the block?
> This block actually uses |strongTab| on line 4328, so I can't use just URL.
> 
> Also I would still use weakSelf even if block needs only tabModel to avoid
> assumptions about tabModel lifetime and because it's a common pattern to use
> weakSelf. If for some reason |tabModel| outlives BVC, then this code will
still
> work as expected, but using weakTabModel may lead to unexpected behavior. 
> 
> WDYT?

Yeah, this is fine for now -- at least, this isn't the place to clean up any of
this.

https://codereview.chromium.org/2692803004/diff/80001/ios/web/public/web_stat...
File ios/web/public/web_state/ui/crw_web_delegate.h (right):

https://codereview.chromium.org/2692803004/diff/80001/ios/web/public/web_stat...
ios/web/public/web_state/ui/crw_web_delegate.h:92:
createWebControllerForURL:(const GURL&)URL
On 2017/02/15 16:46:59, Eugene But wrote:
> On 2017/02/15 10:34:10, marq wrote:
> > Naming nit: Delegate methods that return something don't usually include a
> verb
> > like 'create' -- compare to tableView:cellForRowAtIndexPath:. I think
> > webController:webControllerForURL:openerURL:initiatedByUser: is fine, and it
> > doesn't imply that the delegate necessarily has to "create" a new
> webController
> > (although they very likely will).
> Modeled from
> |webView:createWebViewWithConfiguration:forNavigationAction:windowFeatures:|
> which I think appropriately has "create" word, because there is a concrete
side
> effect for this action (creating a new UI window). In any case, this method
will
> be removed in subsequent CL in favor or WebStateDelegate method, so I would
> prefer to leave it as it is.

Given that, I'm OK with leaving it as is.

[Eugene But (OOO till 7-30), 2017-02-16 16:34:48]

Thanks Mark. Rohit, do you have any comments?

[rohitrao (ping after 24h), 2017-02-17 19:53:16]

lgtm

https://codereview.chromium.org/2692803004/diff/100001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2692803004/diff/100001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1474: -
(BOOL)shouldBlockPopupForPageWithURL:(const GURL&)URL {
Both of these methods should eventually move out of Tab and into
BlockedPopupTabHelper.

[Eugene But (OOO till 7-30), 2017-02-17 20:48:55]

https://codereview.chromium.org/2692803004/diff/100001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2692803004/diff/100001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1474: -
(BOOL)shouldBlockPopupForPageWithURL:(const GURL&)URL {
On 2017/02/17 19:53:16, rohitrao wrote:
> Both of these methods should eventually move out of Tab and into
> BlockedPopupTabHelper.
Yeah, I think both are doable

[Eugene But (OOO till 7-30), 2017-02-17 20:49:01]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-02-17 20:49:54]

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-17 22:52:05]

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

[commit-bot: I haz the power, 2017-02-17 22:52:06]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Eugene But (OOO till 7-30), 2017-02-17 23:39:35]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:40:42]

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-18 01:44:19]

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

[commit-bot: I haz the power, 2017-02-18 01:44:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Eugene But (OOO till 7-30), 2017-02-21 16:17:20]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-02-21 16:17:59]

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-21 16:40:38]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487693840457700,
"parent_rev": "eb0661c8cacbbd0b86f6b7e5a7f7b080f11ad300", "commit_rev":
"91cfb3ad9a9b9502aed2f9bc76c5d3cc96105f22"}

[commit-bot: I haz the power, 2017-02-21 16:41:24]

Description was changed from

==========
Refactored callbacks for opening a new window.

This CL replaces 4 callbacks:
 webPageOrderedOpen:referrer:windowName:inBackground:
 webPageOrderedOpen
 webController:shouldBlockPopupWithURL:sourceURL:
 webController:didBlockPopup:

with a single callback:
 webController:createWebControllerForURL:openerURL:initiatedByUser:
 
This makes ios code more similar to other platforms where windows
are open via single WebContentsDelegate::AddNewContents callback 
and all popup blocking logic lives in chrome layer.

This is a precursory CL to move window opening callback to
WebStateDelegate.


BUG=622072,674991
==========

to

==========
Refactored callbacks for opening a new window.

This CL replaces 4 callbacks:
 webPageOrderedOpen:referrer:windowName:inBackground:
 webPageOrderedOpen
 webController:shouldBlockPopupWithURL:sourceURL:
 webController:didBlockPopup:

with a single callback:
 webController:createWebControllerForURL:openerURL:initiatedByUser:
 
This makes ios code more similar to other platforms where windows
are open via single WebContentsDelegate::AddNewContents callback 
and all popup blocking logic lives in chrome layer.

This is a precursory CL to move window opening callback to
WebStateDelegate.


BUG=622072,674991

Review-Url: https://codereview.chromium.org/2692803004
Cr-Commit-Position: refs/heads/master@{#451778}
Committed:
https://chromium.googlesource.com/chromium/src/+/91cfb3ad9a9b9502aed2f9bc76c5...
==========

[commit-bot: I haz the power, 2017-02-21 16:41:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/91cfb3ad9a9b9502aed2f9bc76c5...