Removed WebFrameClient.cpp from Source/web

Removed WebFrameClient.cpp from Source/web

Removed WebFrameClient.cpp from Source/web by removing the default
implementation of FrameDetached(). Most subclasses override with their
own version  FrameDetached() anyway, so this just involves adding it to
a few classes that were relying on the default.

BUG=361765
Review-Url: https://codereview.chromium.org/2874133002
Cr-Commit-Position: refs/heads/master@{#471703}
Committed: https://chromium.googlesource.com/chromium/src/+/6ce1292c9e18c511370b424765e2fe3be9b21213

[sashab, 2017-05-11 03:37:55]

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

[sashab, 2017-05-11 03:38:41]

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

[sashab, 2017-05-11 03:38:41]

daniel -- I implemented this patch by making FrameDetached() pure virtual, and
fixing the compile errors. So this is the minimal number of subclasses that need
updating. Lmkwyt :)

[commit-bot: I haz the power, 2017-05-11 03:38:53]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-05-11 03:46:29]

LGTM

[sashab, 2017-05-11 03:47:11]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-11 03:48:07]

CQ is trying da patch.

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

[sashab, 2017-05-11 03:49:54]

sashab@chromium.org changed reviewers:
+ dmazzoni@chromium.org, jochen@chromium.org

[sashab, 2017-05-11 03:49:55]

kinuko ptal at content/, jochen@ ptal at components/

[commit-bot: I haz the power, 2017-05-11 03:59:18]

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

[commit-bot: I haz the power, 2017-05-11 03:59:19]

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

[kinuko.yasuda, 2017-05-11 04:03:00]

lgtm, cool

[kinuko.yasuda, 2017-05-11 04:07:16]

kinuko.yasuda@gmail.com changed reviewers:
+ kinuko.yasuda@gmail.com

[kinuko.yasuda, 2017-05-11 04:07:17]

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:327:
frame->Parent()->RemoveChild(frame);
I think we can remove this (and possibly below) and just have DCHECK, but that
can be done separately.

[kinuko, 2017-05-11 04:09:09]

lgtm from right address

[jochen (gone - plz use gerrit), 2017-05-11 07:44:53]

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

[jochen (gone - plz use gerrit), 2017-05-11 07:44:53]

hum, I'm a bit worried of having every new WebFrameClient to duplicate this
logic.

Can we make method return e.g. a bool that indicates whether it did some work or
not? The default could the return false (so we can delete the cpp file), but the
caller of FrameDetached has to do the actual work if the method returned false?

[dcheng, 2017-05-11 21:17:11]

On 2017/05/11 07:44:53, jochen wrote:
> hum, I'm a bit worried of having every new WebFrameClient to duplicate this
> logic.
> 
> Can we make method return e.g. a bool that indicates whether it did some work
or
> not? The default could the return false (so we can delete the cpp file), but
the
> caller of FrameDetached has to do the actual work if the method returned
false?

I guess the problem is that as currently designed, the Close() methods are
exposed for the embedder to manage lifetimes, so it'd be a bit odd to call them
from Blink.

The other issue to consider is: we purposely chose to move frame tree management
into the embedder when we were refactoring for OOPIF. Unfortunately, I don't
remember the reason for this anymore, but it does feel like it'd be a bit weird
for AppendChild() to be in the embedder and RemoveChild() to be in Blink.

My suggestion for resolving this is to have WebLocalFrame::CreateChild(...)
instead of exposing AppendChild() as a public API. Then we can at least move
RemoveChild() into the Blink while preserving symmetry. WDYT?

https://codereview.chromium.org/2874133002/diff/1/components/plugins/renderer...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/2874133002/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.cc:355: if (type ==
DetachType::kRemove && frame->Parent())
Actually I think this one doesn't need the RemoveChild() logic, since it doesn't
have a non-default CreateChildFrame.

[dmazzoni, 2017-05-12 18:56:00]

rs lgtm for test_runner

[sashab, 2017-05-15 02:06:24]

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

[commit-bot: I haz the power, 2017-05-15 02:06:31]

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-15 02:16:01]

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

[commit-bot: I haz the power, 2017-05-15 02:16:02]

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

[sashab, 2017-05-15 02:18:02]

Thanks dcheng; see review comments.

Re your suggestion:
> My suggestion for resolving this is to have WebLocalFrame::CreateChild(...)
instead of exposing AppendChild() as a public API. Then we can at least move
RemoveChild() into the Blink while preserving symmetry. WDYT?

Is this something I can do in a follow-up patch? What would that look like?

https://codereview.chromium.org/2874133002/diff/1/components/plugins/renderer...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/2874133002/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.cc:355: if (type ==
DetachType::kRemove && frame->Parent())
On 2017/05/11 at 21:17:11, dcheng (in AEST) wrote:
> Actually I think this one doesn't need the RemoveChild() logic, since it
doesn't have a non-default CreateChildFrame.

Removed, thanks.

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:327:
frame->Parent()->RemoveChild(frame);
On 2017/05/11 at 04:07:17, kinuko.yasuda wrote:
> I think we can remove this (and possibly below) and just have DCHECK, but that
can be done separately.

Added TODOs for both :) Will do in follow-up patch.

[dcheng, 2017-05-15 03:58:47]

On 2017/05/15 02:18:02, sashab wrote:
> Thanks dcheng; see review comments.
> 
> Re your suggestion:
> > My suggestion for resolving this is to have WebLocalFrame::CreateChild(...)
> instead of exposing AppendChild() as a public API. Then we can at least move
> RemoveChild() into the Blink while preserving symmetry. WDYT?
> 
> Is this something I can do in a follow-up patch? What would that look like?

I'll defer to jochen@ on this one; he does have a good point about the
duplication.

I actually thought about this a bit more, and we could probably clean up the
widget logic too (but that should be a followup): basically when discarding a
local root, Blink can tell the embedder to free the widget associated with the
local root (similar to the FrameDetached callback). What's not clear to me is if
the embedder really needs logic in the equivalent of WidgetDetached(): if not,
maybe we should pass ownership of the widget to the local root directly...

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:327:
frame->Parent()->RemoveChild(frame);
On 2017/05/15 02:18:02, sashab wrote:
> On 2017/05/11 at 04:07:17, kinuko.yasuda wrote:
> > I think we can remove this (and possibly below) and just have DCHECK, but
that
> can be done separately.
> 
> Added TODOs for both :) Will do in follow-up patch.

Given that we fixed up the other places (in WebViewPlugin), let's just go ahead
and do it here too?

[jochen (gone - plz use gerrit), 2017-05-15 05:50:59]

a follow-up (assuming you're signing up for it) sgtm

lgtm once the bots agree :)

[sashab, 2017-05-15 06:22:12]

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

[commit-bot: I haz the power, 2017-05-15 06:22:21]

Dry run: CQ is trying da patch.

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

[sashab, 2017-05-15 06:23:48]

Thanks dcheng & yochen - added a TODO to add a CreateChild() method in
WebLocalFrame.

Put dcheng's name next to it for now ;)

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2874133002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:327:
frame->Parent()->RemoveChild(frame);
On 2017/05/15 at 03:58:47, dcheng (in AEST) wrote:
> On 2017/05/15 02:18:02, sashab wrote:
> > On 2017/05/11 at 04:07:17, kinuko.yasuda wrote:
> > > I think we can remove this (and possibly below) and just have DCHECK, but
that
> > can be done separately.
> > 
> > Added TODOs for both :) Will do in follow-up patch.
> 
> Given that we fixed up the other places (in WebViewPlugin), let's just go
ahead and do it here too?

Done :)

[kinuko, 2017-05-15 06:27:17]

https://codereview.chromium.org/2874133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2874133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:178: if (type ==
DetachType::kRemove && frame->Parent())
Can we also just DCHECK here as we do in WebEmbeddedWorkerImpl cases?

[sashab, 2017-05-15 06:43:22]

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

[commit-bot: I haz the power, 2017-05-15 06:43:44]

Dry run: CQ is trying da patch.

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

[sashab, 2017-05-15 07:07:45]

Done :) Thanks kinuko

[sashab, 2017-05-15 07:08:50]

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

[sashab, 2017-05-15 07:08:54]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-15 07:08:54]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dmazzoni@chromium.org, dcheng@chromium.org, kinuko.yasuda@gmail.com,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2874133002/#ps80001
(title: "Added TODO back")

[commit-bot: I haz the power, 2017-05-15 07:09:09]

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-15 08:53:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494832134009650,
"parent_rev": "2c1c51b6f4179133c0b5dbb03cc5d69837c5088a", "commit_rev":
"6ce1292c9e18c511370b424765e2fe3be9b21213"}

[commit-bot: I haz the power, 2017-05-15 08:53:40]

Description was changed from

==========
Removed WebFrameClient.cpp from Source/web

Removed WebFrameClient.cpp from Source/web by removing the default
implementation of FrameDetached(). Most subclasses override with their
own version  FrameDetached() anyway, so this just involves adding it to
a few classes that were relying on the default.

BUG=361765
==========

to

==========
Removed WebFrameClient.cpp from Source/web

Removed WebFrameClient.cpp from Source/web by removing the default
implementation of FrameDetached(). Most subclasses override with their
own version  FrameDetached() anyway, so this just involves adding it to
a few classes that were relying on the default.

BUG=361765

Review-Url: https://codereview.chromium.org/2874133002
Cr-Commit-Position: refs/heads/master@{#471703}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ce1292c9e18c511370b424765e2...
==========

[commit-bot: I haz the power, 2017-05-15 08:53:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6ce1292c9e18c511370b424765e2...