Remove a bunch of dead code around WindowFeatures

Remove a bunch of dead code around WindowFeatures

Store the WindowFeatures on Page instead of ChromeClientImpl, to reduce
core<->web layer transfers.

Move the logic that makes the final call on a new window's
NavigationPolicy to CreateWindow.cpp.

BUG=

Review-Url: https://codereview.chromium.org/2905283003
Cr-Commit-Position: refs/heads/master@{#476830}
Committed: https://chromium.googlesource.com/chromium/src/+/bfc4789dcaff98323551d6bfcbb1e4db3c820251

[Nate Chapin, 2017-05-26 20:40:34]

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

[commit-bot: I haz the power, 2017-05-26 20:40:59]

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-05-26 21:23:09]

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

[commit-bot: I haz the power, 2017-05-26 21:23:10]

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

[Nate Chapin, 2017-05-30 21:15:51]

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

[commit-bot: I haz the power, 2017-05-30 21:16:28]

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-05-30 21:20:17]

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

[commit-bot: I haz the power, 2017-05-30 21:20:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Nate Chapin, 2017-05-30 21:27:44]

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

[commit-bot: I haz the power, 2017-05-30 21:28:28]

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-05-30 21:42:50]

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

[commit-bot: I haz the power, 2017-05-30 21:42:51]

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

[Nate Chapin, 2017-05-30 23:53:55]

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

[commit-bot: I haz the power, 2017-05-30 23:54:40]

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-05-31 03:09:04]

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

[commit-bot: I haz the power, 2017-05-31 03:09:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2017-05-31 16:53:38]

Description was changed from

==========
Remove a bunch of dead code around WindowFeatures

BUG=
==========

to

==========
Remove a bunch of dead code around WindowFeatures

Store the WindowFeatures on Page instead of ChromeClientImpl, to reduce
core<->web layer transfers.

Move the logic that makes the final call on a new window's
NavigationPolicy to CreateWindow.cpp.

BUG=
==========

[Nate Chapin, 2017-05-31 16:55:11]

japhet@chromium.org changed reviewers:
+ kinuko@chromium.org

[Nate Chapin, 2017-05-31 16:55:14]

kinuko, would you mind reviewing this large-ish cleanup?

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/exported/WebViewBase.h (left):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/exported/WebViewBase.h:221: virtual void
SetCurrentInputEventForTest(const WebInputEvent*) = 0;
This is only needed to test NavigationPolicy decisions. Now that
EffectiveNavigationPolicy() takes a WebInputEvent parameter, it's not needed.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/CreateWindow.cpp:176: policy =
EffectiveNavigationPolicy(
Bonus of calling this: here we only need to do it once, instead of separate
invocations in CreateWindow() and Show().

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/CreateWindow.cpp:196:
frame.View()->SetCanHaveScrollbars(features.scrollbars_visible);
This was a side effect of ChromeClientImpl::SetScrollbarsVisible

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.h (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.h:342: WindowFeatures window_features_;
Rather than storing the WindowFeatures in web/ and manually propagating bits
one-at-a-time via ChromeClient, just stash the WindowFeatures here.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/WindowFeatures.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.cpp:54: // NO. (There is no
public standard that applies to this method.)
I guess there is a spec for this now, kind of. The parsing rules are specced,
but the possible values are unspecced (except for noopener).

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/WindowFeatures.h (left):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:62: WindowFeatures(const
String& dialog_features_string,
Unused.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:77: bool
location_bar_visible;
This is *almost* unused on trunk: when deciding whether to treat a new window as
a popup or a new tab, we use a popup if either tool_bar_visible or
location_bar_visible are true. Instead, just use tool_bar_visible and set it to
true if either is present in the feature string.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:82: bool dialog;
resizable, fullscreen and dialog are all dead.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:86: Vector<String>
additional_features;
This is just used to detect background/persistent in RenderViewImpl. Detect them
during initial parse.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:94: static int
IntFeature(const DialogFeaturesMap&,
All this private stuff was just for the unused constructor.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (left):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:279: namespace {
This whole namespace {} block moved to CreateWindow.cpp.
WebViewBase::CurrentInputEvent() is the only web/ layer concept present here, so
expose that on ChromeClient. Unfortunate to have to add it, but it seems like
less of a layering problem than the old way :)

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:943: const WebInputEvent*
ChromeClientImpl::GetCurrentInputEvent() const {
Unfortunate to have to add this, but it seems like less of a layering problem
than the old way :)

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp (left):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp:67: class
GetNavigationPolicyTest : public testing::Test {
Moved to a new file (EffectiveNavigationPolicyTest.cpp) in Source/core/page/

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp:217:
TEST_F(GetNavigationPolicyTest, NoStatusbarIsNotPopup) {
I dropped these last 3 tests, because the signature of EffectiveNavigationPolicy
makes it explicit that only toolbars affect popup decision.

[kinuko, 2017-06-01 06:39:28]

Looks great, I have some comments/questions but lgtm

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/CreateWindow.cpp:55: void
UpdatePolicyForEvent(const WebInputEvent* input_event,
nit: put these in anon namespace except for EffectiveNavigationPolicy?

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/WindowFeatures.h (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:55: bool tool_bar_visible;
maybe rename this to tool_or_location_bar_visible, or add a short comment about
that, to unconfuse readers?

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:60: bool persistent;
By the way now that core/ can refer to public/web I feel we might be able to
just use WebWindowFeatures here (as a composition or just merge these two),
would it work?

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:944: return
WebViewBase::CurrentInputEvent();
WebViewBase.h is now in core/exported, would just calling this in Page.cpp
works? (Maybe not as it's still implemented in Source/web?)

[Nate Chapin, 2017-06-01 19:40:34]

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

[commit-bot: I haz the power, 2017-06-01 19:41:18]

Dry run: CQ is trying da patch.

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

[Nate Chapin, 2017-06-01 20:57:11]

Would you mind re-reviewing PS 5->6? The changes were substantive enough that
I'm not sure the LGTM still holds :)

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/CreateWindow.cpp:55: void
UpdatePolicyForEvent(const WebInputEvent* input_event,
On 2017/06/01 06:39:28, kinuko wrote:
> nit: put these in anon namespace except for EffectiveNavigationPolicy?

Done.

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/WindowFeatures.h (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:55: bool tool_bar_visible;
On 2017/06/01 06:39:28, kinuko wrote:
> maybe rename this to tool_or_location_bar_visible, or add a short comment
about
> that, to unconfuse readers?

Done (in WebWindowFeatures.h)

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/WindowFeatures.h:60: bool persistent;
On 2017/06/01 06:39:28, kinuko wrote:
> By the way now that core/ can refer to public/web I feel we might be able to
> just use WebWindowFeatures here (as a composition or just merge these two),
> would it work?

Moved the constructor logic to CreateWindow.cpp, and dropped
WindowFeatures.h/cpp

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:944: return
WebViewBase::CurrentInputEvent();
On 2017/06/01 06:39:28, kinuko wrote:
> WebViewBase.h is now in core/exported, would just calling this in Page.cpp
> works? (Maybe not as it's still implemented in Source/web?)

Linker error :(

[Nate Chapin, 2017-06-01 20:58:19]

On 2017/06/01 20:57:11, Nate Chapin wrote:
> Would you mind re-reviewing PS 5->6? The changes were substantive enough that
> I'm not sure the LGTM still holds :)

...by which I meant 6->7. Sorry.

> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/CreateWindow.cpp (right):
> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/CreateWindow.cpp:55: void
> UpdatePolicyForEvent(const WebInputEvent* input_event,
> On 2017/06/01 06:39:28, kinuko wrote:
> > nit: put these in anon namespace except for EffectiveNavigationPolicy?
> 
> Done.
> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/WindowFeatures.h (right):
> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/WindowFeatures.h:55: bool
tool_bar_visible;
> On 2017/06/01 06:39:28, kinuko wrote:
> > maybe rename this to tool_or_location_bar_visible, or add a short comment
> about
> > that, to unconfuse readers?
> 
> Done (in WebWindowFeatures.h)
> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/WindowFeatures.h:60: bool persistent;
> On 2017/06/01 06:39:28, kinuko wrote:
> > By the way now that core/ can refer to public/web I feel we might be able to
> > just use WebWindowFeatures here (as a composition or just merge these two),
> > would it work?
> 
> Moved the constructor logic to CreateWindow.cpp, and dropped
> WindowFeatures.h/cpp
> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2905283003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/ChromeClientImpl.cpp:944: return
> WebViewBase::CurrentInputEvent();
> On 2017/06/01 06:39:28, kinuko wrote:
> > WebViewBase.h is now in core/exported, would just calling this in Page.cpp
> > works? (Maybe not as it's still implemented in Source/web?)
> 
> Linker error :(

[commit-bot: I haz the power, 2017-06-01 21:20:25]

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

[commit-bot: I haz the power, 2017-06-01 21:20:27]

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

[kinuko, 2017-06-02 04:12:24]

lgtm!

[Nate Chapin, 2017-06-02 16:26:43]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-06-02 16:26: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-06-02 16:37:22]

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

[commit-bot: I haz the power, 2017-06-02 16:37:25]

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

[Nate Chapin, 2017-06-02 16:43:12]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2017-06-02 16:43:13]

dcheng, would you mind IPC-reviewing this? Deletes only :)

[dcheng, 2017-06-02 18:46:22]

ipc seems fine. one question about layering (since I'm not sure about the
current status of things), and one about how we layer extensions

https://codereview.chromium.org/2905283003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/Page.h (right):

https://codereview.chromium.org/2905283003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/Page.h:46: #include
"public/web/WebWindowFeatures.h"
I'm not actually sure what the rules are on #including web things from core now.
I guess we consider this OK?

(In the past, this would have to move to platform)

https://codereview.chromium.org/2905283003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebWindowFeatures.h (right):

https://codereview.chromium.org/2905283003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebWindowFeatures.h:54: bool background = false;
From a layering perspective, I think it might be better for this to remain a
vector of strings: this is an extension feature, and (in the past) we seem to
have tried to keep a reasonable separation between extensions and Blink.

[dcheng, 2017-06-02 20:07:21]

Btw I thought about this a bit more at lunch, and since we're already doing this
check for extensions in content (which are a decidedly non-content feature),
doing it in Blink instead seems equivalently terrible.

So LGTM!

[Nate Chapin, 2017-06-02 20:20:23]

https://codereview.chromium.org/2905283003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/Page.h (right):

https://codereview.chromium.org/2905283003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/Page.h:46: #include
"public/web/WebWindowFeatures.h"
On 2017/06/02 18:46:22, dcheng wrote:
> I'm not actually sure what the rules are on #including web things from core
now.
> I guess we consider this OK?
> 
> (In the past, this would have to move to platform)

kinuko suggested it in https://codereview.chromium.org/2905283003/#msg20 and
core/DEPS blesses it without commenting on restrictions, so I guess it's OK,
too?

I haven't been able to find any documentation of the decision.

[Nate Chapin, 2017-06-02 20:20:35]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-06-02 20:21:22]

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-06-02 23:01:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496434835574760,
"parent_rev": "814963bca83b93a2aa87b54d1d4cd3eb15785b4b", "commit_rev":
"bfc4789dcaff98323551d6bfcbb1e4db3c820251"}

[commit-bot: I haz the power, 2017-06-02 23:01:34]

Description was changed from

==========
Remove a bunch of dead code around WindowFeatures

Store the WindowFeatures on Page instead of ChromeClientImpl, to reduce
core<->web layer transfers.

Move the logic that makes the final call on a new window's
NavigationPolicy to CreateWindow.cpp.

BUG=
==========

to

==========
Remove a bunch of dead code around WindowFeatures

Store the WindowFeatures on Page instead of ChromeClientImpl, to reduce
core<->web layer transfers.

Move the logic that makes the final call on a new window's
NavigationPolicy to CreateWindow.cpp.

BUG=

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

[commit-bot: I haz the power, 2017-06-02 23:01:35]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bfc4789dcaff98323551d6bfcbb1...