Add enum that specifies the frame detachment reason.

Add enum that specifies the frame detachment reason.

A frame can be detached when it's either removed or swapped with another
frame through WebFrame::swap(). This enum will be used to specify the
detachment reason when detaching a frame.

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

BUG=464764
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196265

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

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

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

PTAL, thanks!

[dcheng, 2015-05-29 22:01:21]

https://codereview.chromium.org/1145973006/diff/1/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1145973006/diff/1/public/web/WebFrame.h#newco...
public/web/WebFrame.h:93: enum class WebDetachReason { Remove, Swap };
I would put this in WebFrameClient, since that's where we'll use it. I'd
probably define it inside WebFrameClient as well; that way, you can skip the
"Web" prefix.

[dcheng, 2015-05-29 22:03:24]

Never mind, I see why you defined it in WebFrame. Let me think about it.

[dcheng, 2015-06-01 18:05:53]

On 2015/05/29 at 22:03:24, dcheng wrote:
> Never mind, I see why you defined it in WebFrame. Let me think about it.

I think the best alternative (for now) is to just duplicate the enum in
WebFrameClient/WebRemoteFrameClient, above the frameDetached() method. Feel free
to add a TODO(dcheng) to combine it somehow.

[lfg, 2015-06-01 18:16:40]

On 2015/06/01 18:05:53, dcheng wrote:
> On 2015/05/29 at 22:03:24, dcheng wrote:
> > Never mind, I see why you defined it in WebFrame. Let me think about it.
> 
> I think the best alternative (for now) is to just duplicate the enum in
> WebFrameClient/WebRemoteFrameClient, above the frameDetached() method. Feel
free
> to add a TODO(dcheng) to combine it somehow.

That won't work, if we do it then anything that includes both of them won't
compile. Doing hacks like defining different types or adding ifdef's to avoid
the double definition seems way too hacky.

There's another reason we don't want to put it in the *Client, WebFrame.h needs
to include the enum (for the detach function), so we would be creating a
dependency cycle: WebFrame.h -> WebFrameClient.h -> WebFrame.h.

[dcheng, 2015-06-01 18:18:27]

On 2015/06/01 at 18:16:40, lfg wrote:
> On 2015/06/01 18:05:53, dcheng wrote:
> > On 2015/05/29 at 22:03:24, dcheng wrote:
> > > Never mind, I see why you defined it in WebFrame. Let me think about it.
> > 
> > I think the best alternative (for now) is to just duplicate the enum in
> > WebFrameClient/WebRemoteFrameClient, above the frameDetached() method. Feel
free
> > to add a TODO(dcheng) to combine it somehow.
> 
> That won't work, if we do it then anything that includes both of them won't
compile. Doing hacks like defining different types or adding ifdef's to avoid
the double definition seems way too hacky.

Not if it's defined inside the class definition.

> 
> There's another reason we don't want to put it in the *Client, WebFrame.h
needs to include the enum (for the detach function), so we would be creating a
dependency cycle: WebFrame.h -> WebFrameClient.h -> WebFrame.h.

Why does DetachPolicy/DetachReason need to be a parameter to WebFrame::detach()?

[lfg, 2015-06-01 18:23:15]

> > That won't work, if we do it then anything that includes both of them won't
> compile. Doing hacks like defining different types or adding ifdef's to avoid
> the double definition seems way too hacky.
> 
> Not if it's defined inside the class definition.

This falls under defining different types. We would need to have static casts to
convert between WebFrameClient::WebDetachReason into
WebRemoteFrameClient::WebDetachReason, which is pretty hacky.

> > There's another reason we don't want to put it in the *Client, WebFrame.h
> needs to include the enum (for the detach function), so we would be creating a
> dependency cycle: WebFrame.h -> WebFrameClient.h -> WebFrame.h.
> 
> Why does DetachPolicy/DetachReason need to be a parameter to
WebFrame::detach()?

It needs to be there to pass the argument between the content layer and the
blink core layer. See https://codereview.chromium.org/1041473002/.

[dcheng, 2015-06-01 18:25:44]

On 2015/06/01 at 18:23:15, lfg wrote:
> > > That won't work, if we do it then anything that includes both of them
won't
> > compile. Doing hacks like defining different types or adding ifdef's to
avoid
> > the double definition seems way too hacky.
> > 
> > Not if it's defined inside the class definition.
> 
> This falls under defining different types. We would need to have static casts
to convert between WebFrameClient::WebDetachReason into
WebRemoteFrameClient::WebDetachReason, which is pretty hacky.
> 
> > > There's another reason we don't want to put it in the *Client, WebFrame.h
> > needs to include the enum (for the detach function), so we would be creating
a
> > dependency cycle: WebFrame.h -> WebFrameClient.h -> WebFrame.h.
> > 
> > Why does DetachPolicy/DetachReason need to be a parameter to
WebFrame::detach()?
> 
> It needs to be there to pass the argument between the content layer and the
blink core layer. See https://codereview.chromium.org/1041473002/.

I don't think we would ever call WebFrame::detach() with reason ==
DetachReason::Swap. Am I missing something?

[lfg, 2015-06-01 18:44:34]

On 2015/06/01 18:25:44, dcheng wrote:
> On 2015/06/01 at 18:23:15, lfg wrote:
> > > > That won't work, if we do it then anything that includes both of them
> won't
> > > compile. Doing hacks like defining different types or adding ifdef's to
> avoid
> > > the double definition seems way too hacky.
> > > 
> > > Not if it's defined inside the class definition.
> > 
> > This falls under defining different types. We would need to have static
casts
> to convert between WebFrameClient::WebDetachReason into
> WebRemoteFrameClient::WebDetachReason, which is pretty hacky.
> > 
> > > > There's another reason we don't want to put it in the *Client,
WebFrame.h
> > > needs to include the enum (for the detach function), so we would be
creating
> a
> > > dependency cycle: WebFrame.h -> WebFrameClient.h -> WebFrame.h.
> > > 
> > > Why does DetachPolicy/DetachReason need to be a parameter to
> WebFrame::detach()?
> > 
> > It needs to be there to pass the argument between the content layer and the
> blink core layer. See https://codereview.chromium.org/1041473002/.
> 
> I don't think we would ever call WebFrame::detach() with reason ==
> DetachReason::Swap. Am I missing something?

Yes, I guess that's true. The only place we call detach with reason==Swap is in
WebFrame::swap() directly which can already call the Frame version directly. The
first point is still the case though, we would need to convert between
WebFrameClient/WebRemoteFrameClient types (and it would affect both blink and
content).

[lfg, 2015-06-01 19:52:17]

Moved the enum to WebFrameClient/WebRemoteFrameClient, PTAL.

[dcheng, 2015-06-01 20:06:47]

https://codereview.chromium.org/1145973006/diff/20001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1145973006/diff/20001/public/web/WebFrameClie...
public/web/WebFrameClient.h:159: enum WebDetachReason { Remove, Swap };
enum class instead of enum.

Also, let's call this "DetachType" (nasko@ came up with this at lunch), with no
Web prefix.

[lfg, 2015-06-01 20:22:15]

https://codereview.chromium.org/1145973006/diff/20001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1145973006/diff/20001/public/web/WebFrameClie...
public/web/WebFrameClient.h:159: enum WebDetachReason { Remove, Swap };
On 2015/06/01 20:06:46, dcheng wrote:
> enum class instead of enum.
> 
> Also, let's call this "DetachType" (nasko@ came up with this at lunch), with
no
> Web prefix.

Done.

[dcheng, 2015-06-01 20:26:32]

lgtm

[lfg, 2015-06-01 20:28:44]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-06-01 20:29:32]

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

[commit-bot: I haz the power, 2015-06-01 23:35:55]

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

[commit-bot: I haz the power, 2015-06-01 23:35:56]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[lfg, 2015-06-01 23:53:40]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-06-01 23:55:03]

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

[commit-bot: I haz the power, 2015-06-02 01:08:22]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196265