Detach old frame on WebFrame::swap.

Detach old frame on WebFrame::swap.

This is a prerequisite for https://codereview.chromium.org/1041473002/.

BUG=464764
Committed: https://crrev.com/7984f01c8755d6dc229f58e65c5cc7037a499c5f
Cr-Commit-Position: refs/heads/master@{#332619}

[lfg, 2015-05-28 18:29:30]

Patchset #3 (id:40001) has been deleted

[lfg, 2015-05-28 20:57:33]

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

[lfg, 2015-05-28 20:57:33]

Daniel, please take a look before I send this to content/ owners. This is a
prerequisite for https://codereview.chromium.org/1041473002/ .

[dcheng, 2015-05-28 21:56:00]

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

https://codereview.chromium.org/1149793002/diff/60001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:597: virtual void
frameDetached(blink::WebFrame* frame, bool for_replacement);
Can we try to come up with an enum instead of a bool parameter here? I don't
have great ideas for names, but I'll throw an idea out there:

enum class DetachReason {
  Remove,
  Swap,
};

I'm sure someone can come up with a better idea. The wider site-isolation team
might have some ideas too =)

https://codereview.chromium.org/1149793002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:695:
render_frame_proxy_->frameDetached(false);
I don't understand the comment or this code. Why do we need to do this?

[dcheng, 2015-05-28 21:57:01]

Also, don't forget to update mandoline.

(It only builds with GN in a non-component build. Yay...)

[lfg, 2015-05-29 18:34:45]

PTAL, thanks!

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

https://codereview.chromium.org/1149793002/diff/60001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:597: virtual void
frameDetached(blink::WebFrame* frame, bool for_replacement);
On 2015/05/28 21:55:59, dcheng wrote:
> Can we try to come up with an enum instead of a bool parameter here? I don't
> have great ideas for names, but I'll throw an idea out there:
> 
> enum class DetachReason {
>   Remove,
>   Swap,
> };
> 
> I'm sure someone can come up with a better idea. The wider site-isolation team
> might have some ideas too =)

Done.

https://codereview.chromium.org/1149793002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:695:
render_frame_proxy_->frameDetached(false);
On 2015/05/28 21:56:00, dcheng wrote:
> I don't understand the comment or this code. Why do we need to do this?

This code is needed to destroy the RenderFrameProxy for a swapped out
RenderFrameImpl. Should be removed soon when nasko@ is able to remove the
swapped out RF/RV.

[lfg, 2015-05-29 18:38:12]

Patchset #7 (id:140001) has been deleted

[lfg, 2015-06-01 20:31:12]

Hi Daniel, can you take a look at this patch now?

Thanks!

[dcheng, 2015-06-01 21:15:55]

https://codereview.chromium.org/1149793002/diff/160001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/160001/components/html_viewer...
components/html_viewer/html_document.cc:276: frameDetached(frame,
blink::WebDetachReason::Remove);
Once this is defined inside WebFrameClient, we should be able to just say:
DetachReason::Remove.

https://codereview.chromium.org/1149793002/diff/160001/components/html_viewer...
components/html_viewer/html_document.cc:281: DCHECK(reason ==
blink::WebDetachReason::Remove);
DCHECK_EQ

https://codereview.chromium.org/1149793002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1149793002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.h:58: enum class WebDetachReason;
You shouldn't need to forward declare this, since you should get this from the
WebFrameClient inheritance.

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
components/html_viewer/html_document.cc:279: frameDetached(frame,
blink::WebFrameClient::DetachType::Remove);
Ditto. DetachType::Remove should work. Similar comment for the
blink::WebFrameClient/blink::WebRemoteFrameClient qualifiers elsewhere in this
patch.

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
components/html_viewer/html_document.cc:284: DCHECK(type ==
blink::DetachType::Remove);
DCHECK_EQ(DetatchType::Remove, type) here and elsewhere.

I guess this is to assert that we don't support OOPI in mandoline yet? I think
we've been planning to make sure it works there though?

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
components/html_viewer/html_document.h:103: blink::WebFrameClient::DetachType);
You shouldn't need any qualifiers for DetachType here.

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:128: #include
"third_party/WebKit/public/web/WebFrameClient.h"
You shouldn't need this include, since it should already be in the header.

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:20: #include
"third_party/WebKit/public/web/WebFrameClient.h"
This include shouldn't be needed.

[lfg, 2015-06-01 21:34:19]

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
components/html_viewer/html_document.cc:279: frameDetached(frame,
blink::WebFrameClient::DetachType::Remove);
On 2015/06/01 21:15:54, dcheng wrote:
> Ditto. DetachType::Remove should work. Similar comment for the
> blink::WebFrameClient/blink::WebRemoteFrameClient qualifiers elsewhere in this
> patch.

Done.

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
components/html_viewer/html_document.cc:284: DCHECK(type ==
blink::DetachType::Remove);
On 2015/06/01 21:15:54, dcheng wrote:
> DCHECK_EQ(DetatchType::Remove, type) here and elsewhere.
> 
> I guess this is to assert that we don't support OOPI in mandoline yet? I think
> we've been planning to make sure it works there though?

Yes, this assert is here because we don't support OOPIF in mandoline. There are
plans to support it, but since it's a non-trivial amount of work I've added the
assert. When we start adding the OOPIF support, it'll be easy to remove the
assert.

We can't use DCHECK_EQ, because base::logging doesn't know how to print the enum
type.

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/200001/components/html_viewer...
components/html_viewer/html_document.h:103: blink::WebFrameClient::DetachType);
On 2015/06/01 21:15:54, dcheng wrote:
> You shouldn't need any qualifiers for DetachType here.

Done.

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:128: #include
"third_party/WebKit/public/web/WebFrameClient.h"
On 2015/06/01 21:15:54, dcheng wrote:
> You shouldn't need this include, since it should already be in the header.

Done.

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1149793002/diff/200001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:20: #include
"third_party/WebKit/public/web/WebFrameClient.h"
On 2015/06/01 21:15:54, dcheng wrote:
> This include shouldn't be needed.

Done.

[dcheng, 2015-06-01 21:41:11]

lgtm with comments addressed

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
components/html_viewer/html_document.cc:1: 
Typo?

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
components/html_viewer/html_document.h:92: virtual void
frameDetached(blink::WebFrame*, DetachType);
Nit: name these parameters to follow Chrome style.

[lfg, 2015-06-01 21:46:04]

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
components/html_viewer/html_document.cc:1: 
On 2015/06/01 21:41:11, dcheng wrote:
> Typo?

Oops.

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/240001/components/html_viewer...
components/html_viewer/html_document.h:92: virtual void
frameDetached(blink::WebFrame*, DetachType);
On 2015/06/01 21:41:11, dcheng wrote:
> Nit: name these parameters to follow Chrome style.

Done.

[lfg, 2015-06-01 21:46:47]

lfg@chromium.org changed reviewers:
+ creis@chromium.org

[lfg, 2015-06-01 21:46:49]

+creis@

Charlie, can you take a look?

[Charlie Reis, 2015-06-01 23:41:45]

Hmm, there's a few things I don't understand here.

Can you run try jobs for this?  I don't see how it can compile.

I also don't see any tests, or calls with DetachType::Swap.  Is there a way to
test this code?  Is that not possible until more of it lands?

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
components/html_viewer/html_document.cc:262: DCHECK(type == DetachType::Remove);
DCHECK_EQ?

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
components/html_viewer/html_document.h:92: virtual void
frameDetached(blink::WebFrame* frame, DetachType type);
This doesn't look right to me for two reasons.

1) dcheng, why is it right to omit the qualifiers?  All the other Blink types
are prefixed with blink::, here and elsewhere.  I'm not 100% sure on our policy
here, but it seems hard for readers to know where this is coming from
(especially with another DetachType defined in browser.h).

2) There's no DetachType defined anywhere I can find.  The corresponding Blink
CL introduces WebDetachReason in WebFrameClient, but I thought this CL was a
prerequisite for that one?

https://codereview.chromium.org/1149793002/diff/260001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2138: if (type == DetachType::Remove)
What other values can it be, and why would this method make sense with them?

I think some comments here would help a lot.  It looks like Swap is the other
value, so maybe it makes sense to mention why we don't send this IPC or call
removeChild when we're doing a swap.

[lfg, 2015-06-02 00:00:22]

On 2015/06/01 23:41:45, Charlie Reis wrote:
> Hmm, there's a few things I don't understand here.
> 
> Can you run try jobs for this?  I don't see how it can compile.
> 
> I also don't see any tests, or calls with DetachType::Swap.  Is there a way to
> test this code?  Is that not possible until more of it lands?

Just a quick reply, I have a blink CL to add the enum here:
https://codereview.chromium.org/1145973006/ (it's in the CQ). The idea is to
land that first, then this one, then the next blink CL.


I'll address the other comments tomorrow.

[dcheng, 2015-06-02 00:21:51]

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
components/html_viewer/html_document.h:92: virtual void
frameDetached(blink::WebFrame* frame, DetachType type);
On 2015/06/01 23:41:46, Charlie Reis wrote:
> This doesn't look right to me for two reasons.
> 
> 1) dcheng, why is it right to omit the qualifiers?  All the other Blink types
> are prefixed with blink::, here and elsewhere.  I'm not 100% sure on our
policy
> here, but it seems hard for readers to know where this is coming from
> (especially with another DetachType defined in browser.h).

There is no special rule for Blink types. In general, Chrome code only qualify
names as much as needed: for example, developers are encouraged to write
RenderFrameHost inside namespace content { } rather than
content::RenderFrameHost. There are similar examples for subclasses of
interfaces following this principle as well:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
is one: the enum DangerousDownloadAction is defined inside the
content::DownloadTestObserver, but the constructor calls it without any
qualifiers.

Similarly, the qualifiers here are redundant, so we omit them to be consistent.
DetachType is defined inside WebFrameClient, and inside WebFrameClient and its
subclasses, the meaning of DetachType is not ambiguous. Outside of
WebFrameClient, full qualification of the identifier will be required,
preventing potential ambiguity.

> 
> 2) There's no DetachType defined anywhere I can find.  The corresponding Blink
> CL introduces WebDetachReason in WebFrameClient, but I thought this CL was a
> prerequisite for that one?

It's the other way around.

[lfg, 2015-06-02 16:00:01]

Now that the blink patch landed, I've started the trybots.

Please, take another look.

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
components/html_viewer/html_document.cc:262: DCHECK(type == DetachType::Remove);
On 2015/06/01 23:41:46, Charlie Reis wrote:
> DCHECK_EQ?

Can't because base::logging doesn't know how to print the enum types.

https://codereview.chromium.org/1149793002/diff/260001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2138: if (type == DetachType::Remove)
On 2015/06/01 23:41:46, Charlie Reis wrote:
> What other values can it be, and why would this method make sense with them?
> 
> I think some comments here would help a lot.  It looks like Swap is the other
> value, so maybe it makes sense to mention why we don't send this IPC or call
> removeChild when we're doing a swap.

I've added some comments, please take a look.

[Charlie Reis, 2015-06-02 17:05:28]

Thanks, that makes a lot more sense.  Just one more thing from the last round:

On 2015/06/01 23:41:45, Charlie Reis wrote:
> I also don't see any tests, or calls with DetachType::Swap.  Is there a way to
> test this code?  Is that not possible until more of it lands?

I'm still wondering about the larger plan here.  Will the DetachType::Swap cases
be added later?  And does that mean there's basically no behavior change here so
existing tests should cover it?  If so, then I'm good with this.

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
components/html_viewer/html_document.cc:262: DCHECK(type == DetachType::Remove);
On 2015/06/02 16:00:01, lfg wrote:
> On 2015/06/01 23:41:46, Charlie Reis wrote:
> > DCHECK_EQ?
> 
> Can't because base::logging doesn't know how to print the enum types.

Acknowledged.

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1149793002/diff/260001/components/html_viewer...
components/html_viewer/html_document.h:92: virtual void
frameDetached(blink::WebFrame* frame, DetachType type);
On 2015/06/02 00:21:51, dcheng wrote:
> On 2015/06/01 23:41:46, Charlie Reis wrote:
> > This doesn't look right to me for two reasons.
> > 
> > 1) dcheng, why is it right to omit the qualifiers?  All the other Blink
types
> > are prefixed with blink::, here and elsewhere.  I'm not 100% sure on our
> policy
> > here, but it seems hard for readers to know where this is coming from
> > (especially with another DetachType defined in browser.h).
> 
> There is no special rule for Blink types. In general, Chrome code only qualify
> names as much as needed: for example, developers are encouraged to write
> RenderFrameHost inside namespace content { } rather than
> content::RenderFrameHost. There are similar examples for subclasses of
> interfaces following this principle as well:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
> is one: the enum DangerousDownloadAction is defined inside the
> content::DownloadTestObserver, but the constructor calls it without any
> qualifiers.
> 
> Similarly, the qualifiers here are redundant, so we omit them to be
consistent.
> DetachType is defined inside WebFrameClient, and inside WebFrameClient and its
> subclasses, the meaning of DetachType is not ambiguous. Outside of
> WebFrameClient, full qualification of the identifier will be required,
> preventing potential ambiguity.

Ah, it's because all of these are subclasses of WebFrameClient.  I'm ok with
that.  (It seemed random to prefix some Blink types but not others, but now I
see the reasoning.)

> > 
> > 2) There's no DetachType defined anywhere I can find.  The corresponding
Blink
> > CL introduces WebDetachReason in WebFrameClient, but I thought this CL was a
> > prerequisite for that one?
> 
> It's the other way around.

Oh, there was another CL.  The one I mentioned (linked from the description) is
meant to land after this.

https://codereview.chromium.org/1149793002/diff/260001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1149793002/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2138: if (type == DetachType::Remove)
On 2015/06/02 16:00:01, lfg wrote:
> On 2015/06/01 23:41:46, Charlie Reis wrote:
> > What other values can it be, and why would this method make sense with them?
> > 
> > I think some comments here would help a lot.  It looks like Swap is the
other
> > value, so maybe it makes sense to mention why we don't send this IPC or call
> > removeChild when we're doing a swap.
> 
> I've added some comments, please take a look.

Much better, thanks.

[lfg, 2015-06-02 17:17:03]

On 2015/06/02 17:05:28, Charlie Reis wrote:
> Thanks, that makes a lot more sense.  Just one more thing from the last round:
> 
> On 2015/06/01 23:41:45, Charlie Reis wrote:
> > I also don't see any tests, or calls with DetachType::Swap.  Is there a way
to
> > test this code?  Is that not possible until more of it lands?
> 
> I'm still wondering about the larger plan here.  Will the DetachType::Swap
cases
> be added later?  And does that mean there's basically no behavior change here
so
> existing tests should cover it?  If so, then I'm good with this.

There's no behavior change, so any test that exercises the local to remote
navigation should cover DetachType::Swap, in particular, NavigateRemoteFrame
will do it.

[Charlie Reis, 2015-06-02 17:40:47]

On 2015/06/02 17:17:03, lfg wrote:
> On 2015/06/02 17:05:28, Charlie Reis wrote:
> > Thanks, that makes a lot more sense.  Just one more thing from the last
round:
> > 
> > On 2015/06/01 23:41:45, Charlie Reis wrote:
> > > I also don't see any tests, or calls with DetachType::Swap.  Is there a
way
> to
> > > test this code?  Is that not possible until more of it lands?
> > 
> > I'm still wondering about the larger plan here.  Will the DetachType::Swap
> cases
> > be added later?  And does that mean there's basically no behavior change
here
> so
> > existing tests should cover it?  If so, then I'm good with this.
> 
> There's no behavior change, so any test that exercises the local to remote
> navigation should cover DetachType::Swap, in particular, NavigateRemoteFrame
> will do it.

Oh, so the DetachType::Swap call will be within the Blink CL that lands after
this.  Ok.

LGTM!

[lfg, 2015-06-02 17:46:53]

lfg@chromium.org changed reviewers:
+ sky@chromium.org, vitalybuka@chromium.org

[lfg, 2015-06-02 17:46:54]

+sky:

Please, take a look at components/html_viewer.

+vitalybuka:

Please, take a look at components/printing.

[sky, 2015-06-02 23:01:41]

LGTM

https://codereview.chromium.org/1149793002/diff/280001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1149793002/diff/280001/components/html_viewer...
components/html_viewer/html_document.cc:262: DCHECK(type == DetachType::Remove);
nit: DCHECK_EQ

[lfg, 2015-06-02 23:04:02]

On 2015/06/02 23:01:41, sky wrote:
> LGTM
> 
>
https://codereview.chromium.org/1149793002/diff/280001/components/html_viewer...
> File components/html_viewer/html_document.cc (right):
> 
>
https://codereview.chromium.org/1149793002/diff/280001/components/html_viewer...
> components/html_viewer/html_document.cc:262: DCHECK(type ==
DetachType::Remove);
> nit: DCHECK_EQ

Can't because base::logging doesn't know how to print the enum type.

[Vitaly Buka (NO REVIEWS), 2015-06-03 05:59:48]

lgtm

[lfg, 2015-06-03 15:50:00]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-06-03 15:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1149793002/#ps280001
(title: "adding comments")

[commit-bot: I haz the power, 2015-06-03 15:50:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149793002/280001

[commit-bot: I haz the power, 2015-06-03 15:58:41]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-06-03 15:59:34]

Patchset 13 (id:??) landed as
https://crrev.com/7984f01c8755d6dc229f58e65c5cc7037a499c5f
Cr-Commit-Position: refs/heads/master@{#332619}