Moved |openedByDOM| to WebState's CreateParams and public interface.

Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984, 620045, 620045
Review-Url: https://codereview.chromium.org/2755823002
Cr-Commit-Position: refs/heads/master@{#458274}
Committed: https://chromium.googlesource.com/chromium/src/+/a8ee9decef09022c19cab0238a80487e96f408d4

[kkhorimoto, 2017-03-16 01:28:29]

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

[kkhorimoto, 2017-03-16 01:37:12]

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

[commit-bot: I haz the power, 2017-03-16 01:37:37]

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-03-16 02:08:23]

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

[commit-bot: I haz the power, 2017-03-16 02:08:23]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-03-16 15:31:20]

I looked up at content API more carefully and it looks like they have API we
need.

CCing Sylvain who also wanted to make a similar change.

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_window.h (left):

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_window.h:12: #import
"ios/web/web_state/web_state_impl.h"
Could you please cleanup ios/chrome/browser/sessions/DEPS and reference
crbug.com/620045 in you CL description

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_window_unittest.mm (right):

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_window_unittest.mm:60:
std::unique_ptr<web::WebState> webState1 = CreateWebState(false);
nit: false /*opened_by_dom*/

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_window_unittest.mm:78:
std::unique_ptr<web::WebState> webState1 = CreateWebState(true);
ditto

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/test/fak...
File ios/web/public/test/fakes/test_web_state_delegate.mm (right):

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/test/fak...
ios/web/public/test/fakes/test_web_state_delegate.mm:8: #import
"ios/web/web_state/web_state_impl.h"
Is this include still needed?

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state.h (right):

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/web_stat...
ios/web/public/web_state/web_state.h:64: bool opened_by_dom;
Content has this boolean, which actually better represents the intent of this
API:
bool created_with_opener;

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/web_stat...
ios/web/public/web_state/web_state.h:239: virtual bool IsOpenedByDOM() const =
0;
Is this the same as content's HasOpener() ?

[michaeldo, 2017-03-16 18:18:52]

Description was changed from

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984
==========

to

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984
==========

[kkhorimoto, 2017-03-17 01:23:17]

Description was changed from

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984
==========

to

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984, 620045
==========

[kkhorimoto, 2017-03-17 21:10:27]

Description was changed from

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984, 620045
==========

to

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984, 620045, 620045
==========

[kkhorimoto, 2017-03-17 21:52:03]

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

[commit-bot: I haz the power, 2017-03-17 21:53:00]

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

[kkhorimoto, 2017-03-17 21:55:38]

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

[commit-bot: I haz the power, 2017-03-17 21:55:58]

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-03-17 22:06:19]

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

[commit-bot: I haz the power, 2017-03-17 22:06:20]

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

[kkhorimoto, 2017-03-17 23:04:26]

test fix & removed include

[kkhorimoto, 2017-03-17 23:04:28]

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

[kkhorimoto, 2017-03-17 23:04:49]

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_window.h (left):

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_window.h:12: #import
"ios/web/web_state/web_state_impl.h"
On 2017/03/16 15:31:19, Eugene But wrote:
> Could you please cleanup ios/chrome/browser/sessions/DEPS and reference
> crbug.com/620045 in you CL description

Done.

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_window_unittest.mm (right):

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_window_unittest.mm:60:
std::unique_ptr<web::WebState> webState1 = CreateWebState(false);
On 2017/03/16 15:31:19, Eugene But wrote:
> nit: false /*opened_by_dom*/

Done.

https://codereview.chromium.org/2755823002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_window_unittest.mm:78:
std::unique_ptr<web::WebState> webState1 = CreateWebState(true);
On 2017/03/16 15:31:19, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/test/fak...
File ios/web/public/test/fakes/test_web_state_delegate.mm (right):

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/test/fak...
ios/web/public/test/fakes/test_web_state_delegate.mm:8: #import
"ios/web/web_state/web_state_impl.h"
On 2017/03/16 15:31:19, Eugene But wrote:
> Is this include still needed?

Done.

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state.h (right):

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/web_stat...
ios/web/public/web_state/web_state.h:64: bool opened_by_dom;
On 2017/03/16 15:31:19, Eugene But wrote:
> Content has this boolean, which actually better represents the intent of this
> API:
> bool created_with_opener;

Done.

https://codereview.chromium.org/2755823002/diff/40001/ios/web/public/web_stat...
ios/web/public/web_state/web_state.h:239: virtual bool IsOpenedByDOM() const =
0;
On 2017/03/16 15:31:19, Eugene But wrote:
> Is this the same as content's HasOpener() ?

Yes, it looks like it.  I've updated the naming.

[commit-bot: I haz the power, 2017-03-17 23:05:14]

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-03-17 23:25:19]

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

[commit-bot: I haz the power, 2017-03-17 23:25:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eugene But (OOO till 7-30), 2017-03-18 00:33:30]

lgtm!

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/session_window_unittest.mm (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/session_window_unittest.mm:61:
std::unique_ptr<web::WebState> webState1 =
nit: s/webState1/web_state_1 since you touching this

Same comment for 3 other variables in this file

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/browser_view_controller.mm:2420: DCHECK(webState &&
webState->HasOpener() ||
nit: Consider dropping |webState && | it's still going to crash on the next
check.

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/crw_ses...
File ios/web/public/crw_session_storage.mm (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/crw_ses...
ios/web/public/crw_session_storage.mm:19: NSString* const kHasOpenerKey =
@"openedByDOM";
Optional nit: do you want to deprecate this key in a separate CL?

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/web_sta...
File ios/web/public/web_state/web_state.h (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/web_sta...
ios/web/public/web_state/web_state.h:63: // Whether the WebState is created as
the result of a window.open
Could you please update the comment:

Whether this WebState was created via a call to window.open or by clicking a
link with a blank target. Used when determining whether the WebState is allowed
to be closed via window.close(). May be true even with a null |opener| (e.g.,
for blocked popups).

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/web_sta...
ios/web/public/web_state/web_state.h:239: // clicking a link with a blank
targer.
How about this comment?:
Whether this WebState was created with an opener. See
CreateParams::created_with_opener comment for details.

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

https://codereview.chromium.org/2755823002/diff/100001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:4234: if (self.webState &&
self.webState->HasOpener())
|self.webState && self.webState->HasOpener()| is quite common code in web
controller. Do you want to add |- (BOOL)hasOpener| helper method?

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

https://codereview.chromium.org/2755823002/diff/100001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:368: // Whether this WebState is associated
with a session opened via window.open.
How about this comment?:

Whether this WebState was created with an opener. See
CreateParams::created_with_opener comment for details.

[Eugene But (OOO till 7-30), 2017-03-18 00:34:34]

Almost forgot. Could you please add a test that calls WebState::Create and
checks HasOpener().

[kkhorimoto, 2017-03-20 22:39:20]

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/session_window_unittest.mm (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/session_window_unittest.mm:61:
std::unique_ptr<web::WebState> webState1 =
On 2017/03/18 00:33:29, Eugene But wrote:
> nit: s/webState1/web_state_1 since you touching this
> 
> Same comment for 3 other variables in this file

Done.

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/browser_view_controller.mm:2420: DCHECK(webState &&
webState->HasOpener() ||
On 2017/03/18 00:33:30, Eugene But wrote:
> nit: Consider dropping |webState && | it's still going to crash on the next
> check.

Done.

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/crw_ses...
File ios/web/public/crw_session_storage.mm (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/crw_ses...
ios/web/public/crw_session_storage.mm:19: NSString* const kHasOpenerKey =
@"openedByDOM";
On 2017/03/18 00:33:30, Eugene But wrote:
> Optional nit: do you want to deprecate this key in a separate CL?

Acknowledged.

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/web_sta...
File ios/web/public/web_state/web_state.h (right):

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/web_sta...
ios/web/public/web_state/web_state.h:63: // Whether the WebState is created as
the result of a window.open
On 2017/03/18 00:33:30, Eugene But wrote:
> Could you please update the comment:
> 
> Whether this WebState was created via a call to window.open or by clicking a
> link with a blank target. Used when determining whether the WebState is
allowed
> to be closed via window.close(). May be true even with a null |opener| (e.g.,
> for blocked popups).

Done, although I left out the last sentence since our CreateParams don't include
|opener|.

https://codereview.chromium.org/2755823002/diff/100001/ios/web/public/web_sta...
ios/web/public/web_state/web_state.h:239: // clicking a link with a blank
targer.
On 2017/03/18 00:33:30, Eugene But wrote:
> How about this comment?:
> Whether this WebState was created with an opener. See
> CreateParams::created_with_opener comment for details.

Done.

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

https://codereview.chromium.org/2755823002/diff/100001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:4234: if (self.webState &&
self.webState->HasOpener())
On 2017/03/18 00:33:30, Eugene But wrote:
> |self.webState && self.webState->HasOpener()| is quite common code in web
> controller. Do you want to add |- (BOOL)hasOpener| helper method?

Done.

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

https://codereview.chromium.org/2755823002/diff/100001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:368: // Whether this WebState is associated
with a session opened via window.open.
On 2017/03/18 00:33:30, Eugene But wrote:
> How about this comment?:
> 
> Whether this WebState was created with an opener. See
> CreateParams::created_with_opener comment for details.

Done.

[kkhorimoto, 2017-03-20 22:39:25]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-20 22:39:25]

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/2755823002/#ps120001
(title: "addressed comments, added test")

[commit-bot: I haz the power, 2017-03-20 22:40:30]

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-20 23:05:00]

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

[commit-bot: I haz the power, 2017-03-20 23:05:01]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kkhorimoto, 2017-03-21 00:38:03]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-21 00:38:03]

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/2755823002/#ps140001
(title: "fix DEPS")

[commit-bot: I haz the power, 2017-03-21 00:38:48]

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-21 01:01:22]

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

[commit-bot: I haz the power, 2017-03-21 01:01:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kkhorimoto, 2017-03-21 01:15:08]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-21 01:15:09]

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/2755823002/#ps160001
(title: "fix DEPS (forealz)")

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

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-21 01:25:28]

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

[commit-bot: I haz the power, 2017-03-21 01:25:29]

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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kkhorimoto, 2017-03-21 01:37:09]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-21 01:37:09]

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/2755823002/#ps180001
(title: ".")

[commit-bot: I haz the power, 2017-03-21 01:37: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-03-21 01:54:03]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490060229342220,
"parent_rev": "57842131a06bc01168d581f14fb496bb63be216a", "commit_rev":
"a8ee9decef09022c19cab0238a80487e96f408d4"}

[commit-bot: I haz the power, 2017-03-21 01:54:44]

Description was changed from

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984, 620045, 620045
==========

to

==========
Moved |openedByDOM| to WebState's CreateParams and public interface.

WebContents::CreateParams also includes opened by DOM information.  Our
interface diverges slightly because web// requires that information to
be exposed, while content// handles that logic internally via render
view hosts.

BUG=454984, 620045, 620045

Review-Url: https://codereview.chromium.org/2755823002
Cr-Commit-Position: refs/heads/master@{#458274}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8ee9decef09022c19cab0238a80...
==========

[commit-bot: I haz the power, 2017-03-21 01:54:45]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a8ee9decef09022c19cab0238a80...