Require WebLocalFrame to be created with a non-null client

Require WebLocalFrame to be created with a non-null client

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none

Committed: https://crrev.com/9f8e2f611e44753f7d8de5a2a6716bd030bc7283
Cr-Commit-Position: refs/heads/master@{#421832}

[dcheng, 2016-09-24 02:47:04]

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

[commit-bot: I haz the power, 2016-09-24 02:47: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, 2016-09-24 03:25:29]

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

[commit-bot: I haz the power, 2016-09-24 03:25:32]

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

[dcheng, 2016-09-29 01:28:43]

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

[commit-bot: I haz the power, 2016-09-29 01:29:09]

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, 2016-09-29 01:33:24]

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

[commit-bot: I haz the power, 2016-09-29 01:33:25]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dcheng, 2016-09-29 01:46:19]

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

[commit-bot: I haz the power, 2016-09-29 01:46:55]

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, 2016-09-29 02:20:57]

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

[commit-bot: I haz the power, 2016-09-29 02:20:58]

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

[dcheng, 2016-09-29 03:25:32]

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

[commit-bot: I haz the power, 2016-09-29 03:25:41]

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

[dcheng, 2016-09-29 03:26:07]

Description was changed from

==========
Disallow null WebFrameClients

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

to

==========
Disallow null WebFrameClients

BUG=none
==========

[dcheng, 2016-09-29 03:26:07]

Description was changed from

==========
Disallow null WebFrameClients

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

to

==========
Disallow null WebFrameClients

BUG=none
==========

[dcheng, 2016-09-29 03:26:07]

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

[dcheng, 2016-09-29 03:26:07]

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

[dcheng, 2016-09-29 03:42:11]

Description was changed from

==========
Disallow null WebFrameClients

BUG=none
==========

to

==========
Disallow null WebFrameClients

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none
==========

[dcheng, 2016-09-29 04:06:35]

Description was changed from

==========
Disallow null WebFrameClients

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none
==========

to

==========
Require WebLocalFrame to be created with a non-null client

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none
==========

[dcheng, 2016-09-29 04:06:35]

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

[dcheng, 2016-09-29 04:13:28]

WDYT? I don't think we'll be able to get rid of all the WebFrameClient null
checks, but I'm hoping this will greatly reduce them in the long-term.

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

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1134: DCHECK(client);
The main goal of this patch is to remove these strange null checks: at this
point, the frame should never be detached (since we're in the middle of
initializing it).

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:114: virtual ~WebFrameClient() {}
I'm not sure how I feel about making this public, since it forces all derived
classes to have a public dtor.

At the same time, having two copies of MockWebFrameClient lying around the code
base also seemed weird.

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:166: virtual WebLocalFrame*
createChildFrame(WebLocalFrame* parent, WebTreeScopeType, const WebString& name,
const WebString& uniqueName, WebSandboxFlags sandboxFlags, const
WebFrameOwnerProperties&) { return nullptr; }
Changed the return type since all child frames start as local frames.

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:179: virtual void
frameDetached(WebLocalFrame*, DetachType);
I gave this a default implementation to avoid a bunch of boilerplate.

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:185: virtual void
willCommitProvisionalLoad(WebLocalFrame*) {}
I'm not sure how willClose() ended up meaning this, but the original name didn't
reflect its purpose at all, so I renamed it.

[dglazkov, 2016-09-29 04:27:06]

lgtm

[esprehn, 2016-09-29 04:28:42]

https://codereview.chromium.org/2369613003/diff/80001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2369613003/diff/80001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:530: blink::WebFrameClient
frame_client;
This scares me, the WebLocalFrame(Impl) below is garbage collected, so it can
live longer than this function if someone passes a reference to it, but the
client on the stack dies immediately after this function returns. What makes us
think this is safe? Should there be a static default WebFrameClient?

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

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1133: WebFrameClient* client
= webFrame->client();
we should make client() return a reference

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:40: // WebFrameClient may not be
null.
we should probably pass a reference then

[dcheng, 2016-09-29 04:42:38]

https://codereview.chromium.org/2369613003/diff/80001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2369613003/diff/80001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:530: blink::WebFrameClient
frame_client;
On 2016/09/29 04:28:42, esprehn wrote:
> This scares me, the WebLocalFrame(Impl) below is garbage collected, so it can
> live longer than this function if someone passes a reference to it, but the
> client on the stack dies immediately after this function returns. What makes
us
> think this is safe? Should there be a static default WebFrameClient?

When a frame is detached, it nulls out the client pointer, so there won't be a
dangling pointer.

We may want to consider using the static default WebFrameClient in the future,
but that's a non-trivial change.

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

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1133: WebFrameClient* client
= webFrame->client();
On 2016/09/29 04:28:42, esprehn wrote:
> we should make client() return a reference

Unfortunately, that's not easy, because it can legitimately be null when the
frame is detached.

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2369613003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:40: // WebFrameClient may not be
null.
On 2016/09/29 04:28:42, esprehn wrote:
> we should probably pass a reference then

I'm not opposed to changing this, but it's inconsistent with how all the
public/web interfaces are currently defined. If we want to change this, it might
be better to do a separate CL.

[commit-bot: I haz the power, 2016-09-29 04:53:52]

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

[commit-bot: I haz the power, 2016-09-29 04:53:53]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-09-29 05:09:15]

lgtm, okay seems legit.

[dcheng, 2016-09-29 05:59:07]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-09-29 05:59:16]

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, 2016-09-29 06:04:34]

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

[commit-bot: I haz the power, 2016-09-29 06:04:35]

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

[dcheng, 2016-09-29 15:14:43]

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

[dcheng, 2016-09-29 15:14:44]

+jochen, mind reviewing components, content/test, and extensions/renderer as an
OWNER?

[jochen (gone - plz use gerrit), 2016-09-29 15:17:15]

lgtm

[dcheng, 2016-09-29 15:26:31]

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

[dcheng, 2016-09-29 15:26:32]

+liberato for //media owners

[liberato (no reviews please), 2016-09-29 15:39:43]

lgtm.

[dcheng, 2016-09-29 15:50:27]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-09-29 15:50:38]

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, 2016-09-29 15:55:11]

Description was changed from

==========
Require WebLocalFrame to be created with a non-null client

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none
==========

to

==========
Require WebLocalFrame to be created with a non-null client

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none
==========

[commit-bot: I haz the power, 2016-09-29 15:55:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-29 15:59:27]

Description was changed from

==========
Require WebLocalFrame to be created with a non-null client

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none
==========

to

==========
Require WebLocalFrame to be created with a non-null client

This simplifies the invariants around WebLocalFrames in Blink:
the client is only ever null once the frame is detached. In
theory, this should allow the removal of a bunch of random
WebFrameClient null checks.

BUG=none

Committed: https://crrev.com/9f8e2f611e44753f7d8de5a2a6716bd030bc7283
Cr-Commit-Position: refs/heads/master@{#421832}
==========

[commit-bot: I haz the power, 2016-09-29 15:59:34]

Patchset 5 (id:??) landed as
https://crrev.com/9f8e2f611e44753f7d8de5a2a6716bd030bc7283
Cr-Commit-Position: refs/heads/master@{#421832}

[dcheng, 2016-10-01 02:10:11]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2389493002/ by dcheng@chromium.org.

The reason for reverting is: Also just testing..