Create base class for common functionality of Web{Local,Remote}Frame

Create base class for common functionality of Web{Local,Remote}Frame

This replaces the manual dispatch that emulates virtual calls today, and
future patches will move more details of Oilpan tracing, etc. into this
shared implementation base.

BUG=none

Committed: https://crrev.com/f0408c3aa2c878571a05e341363c5a0319959733
Cr-Commit-Position: refs/heads/master@{#361300}

[dcheng, 2015-11-23 09:00:16]

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

[dcheng, 2015-11-23 09:00:17]

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:115: return frame ?
frame->toImplBase()->frame() : nullptr;
Most locations that convert a WebFrame to a Frame don't need to care about null.
This and RemoteFrameClientImpl are the only locations that need to handle this
detail.

One alternative I considered was making a sentinel placeholder for logically
null WebFrames. This would avoid the need for the null check, but at the cost of
overloading:
  explicit operator bool() so the sentinel value tests to false
  operator==(std::nullptr_t, WebFrame*)
  operator==(WebFrame*, std::nullptr_t) so the sentinel value is considered
equivalent to nullptr

In the end, this seemed too clever and easy to accidentally break in some way,
so I ended up just going with the simple way.

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:159: if
(!m_webFrame->toImplBase()->frame()->ownerLayoutObject())
Note that these changed, since this should never be null.

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:309: ALWAYS_INLINE bool
WebFrame::isFrameAlive(const WebFrame* frame)
It's possible to take advantage of the shared impl base to move all tracing
details there. However, I have a followup patch to change how the frame tree is
stored which will substantially change this anyway (to fix some edge cases
around swap and simplify how the swap code looks). So I'm going to address the
Oilpan tracing in a followup patch.

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1684:
WebFrame::traceFrames(visitor, this);
I'm pretty sure I need to update this to trace the new base, but I wanted to get
this patch out for review (I think this and WebRemoteFrameImpl::trace are the
only fix required at the moment). I'll fix it in my next patchset.

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/1467123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:155: WebRemoteFrameImpl*
frame() const { return m_frame; }
I made this return the impl directly, to make it easier for tests.

[haraken, 2015-11-24 02:30:54]

LGTM

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:26: Frame*
toCoreFrame(WebFrame* frame)

toCoreFrameOrNull ?

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:276: if (Frame* frame =
toImplBase()->frame())

Not related to this patch, it seems confusing that some code worries about null
and other code don't...

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameImplBase.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameImplBase.cpp:16: #if ENABLE(OILPAN)

I guess ENABLE(OILPAN) won't be needed.

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameImplBase.h (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameImplBase.h:38: #if ENABLE(OILPAN)

I guess ENABLE(OILPAN) won't be needed.

[dcheng, 2015-11-24 06:55:07]

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:26: Frame*
toCoreFrame(WebFrame* frame)
On 2015/11/24 at 02:30:53, haraken wrote:
> toCoreFrameOrNull ?

My concern about that name is it sounds like it might not convert to core frame
for another reason (aka toLocalFrameIfNotNull), rather than the input might be
null. Would you be OK with a comment that explains why this is necessary?
Something like:

Convenience function to convert a WebFrame to Frame. This avoids a lot of
boilerplate for FrameClient members that traverse the frame tree, since things
like WebFrame::parent() may be null if a frame has no parent.

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:276: if (Frame* frame =
toImplBase()->frame())
On 2015/11/24 at 02:30:53, haraken wrote:
> Not related to this patch, it seems confusing that some code worries about
null and other code don't...

Yeah, this is similar to some of the null-check problems elsewhere in Blink.
Sometimes functions can never be called on a detached frame, but others can =(

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameImplBase.h (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameImplBase.h:38: #if ENABLE(OILPAN)
On 2015/11/24 at 02:30:53, haraken wrote:
> I guess ENABLE(OILPAN) won't be needed.

Hmm, is ENABLE(OILPAN) universally enabled? Right now, this still has to be a
WillBe since WebLocalFrameImpl/WebRemoteFrameImpl is a WillBe type. While it's
not strictly needed, it doesn't make sense to have the trace either if oilpan
isn't enabled. Or is this just to make the conversion simpler later?

[haraken, 2015-11-24 07:00:34]

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:26: Frame*
toCoreFrame(WebFrame* frame)
On 2015/11/24 06:55:06, dcheng wrote:
> On 2015/11/24 at 02:30:53, haraken wrote:
> > toCoreFrameOrNull ?
> 
> My concern about that name is it sounds like it might not convert to core
frame
> for another reason (aka toLocalFrameIfNotNull), rather than the input might be
> null. Would you be OK with a comment that explains why this is necessary?
> Something like:
> 
> Convenience function to convert a WebFrame to Frame. This avoids a lot of
> boilerplate for FrameClient members that traverse the frame tree, since things
> like WebFrame::parent() may be null if a frame has no parent.

Yes, let's add the comment.

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameImplBase.h (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameImplBase.h:38: #if ENABLE(OILPAN)
On 2015/11/24 06:55:07, dcheng wrote:
> On 2015/11/24 at 02:30:53, haraken wrote:
> > I guess ENABLE(OILPAN) won't be needed.
> 
> Hmm, is ENABLE(OILPAN) universally enabled? Right now, this still has to be a
> WillBe since WebLocalFrameImpl/WebRemoteFrameImpl is a WillBe type. While it's
> not strictly needed, it doesn't make sense to have the trace either if oilpan
> isn't enabled. Or is this just to make the conversion simpler later?

We normally don't add #if ENABLE(OILPAN) to a trace method of a WillBe class.
It's just never called.

[dcheng, 2015-11-24 07:50:24]

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:26: Frame*
toCoreFrame(WebFrame* frame)
On 2015/11/24 at 07:00:33, haraken wrote:
> On 2015/11/24 06:55:06, dcheng wrote:
> > On 2015/11/24 at 02:30:53, haraken wrote:
> > > toCoreFrameOrNull ?
> > 
> > My concern about that name is it sounds like it might not convert to core
frame
> > for another reason (aka toLocalFrameIfNotNull), rather than the input might
be
> > null. Would you be OK with a comment that explains why this is necessary?
> > Something like:
> > 
> > Convenience function to convert a WebFrame to Frame. This avoids a lot of
> > boilerplate for FrameClient members that traverse the frame tree, since
things
> > like WebFrame::parent() may be null if a frame has no parent.
> 
> Yes, let's add the comment.

Done (I also added a comment to try to dedupe the code at some point: onion soup
will probably help as well)

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameImplBase.cpp (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameImplBase.cpp:16: #if ENABLE(OILPAN)
On 2015/11/24 at 02:30:53, haraken wrote:
> I guess ENABLE(OILPAN) won't be needed.

Done.

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameImplBase.h (right):

https://codereview.chromium.org/1467123003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameImplBase.h:38: #if ENABLE(OILPAN)
On 2015/11/24 at 07:00:33, haraken wrote:
> On 2015/11/24 06:55:07, dcheng wrote:
> > On 2015/11/24 at 02:30:53, haraken wrote:
> > > I guess ENABLE(OILPAN) won't be needed.
> > 
> > Hmm, is ENABLE(OILPAN) universally enabled? Right now, this still has to be
a
> > WillBe since WebLocalFrameImpl/WebRemoteFrameImpl is a WillBe type. While
it's
> > not strictly needed, it doesn't make sense to have the trace either if
oilpan
> > isn't enabled. Or is this just to make the conversion simpler later?
> 
> We normally don't add #if ENABLE(OILPAN) to a trace method of a WillBe class.
It's just never called.

Makes sense. Done.

[dcheng, 2015-11-24 07:50:32]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-11-24 07:50:32]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1467123003/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2015-11-24 07:51:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467123003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467123003/80001

[commit-bot: I haz the power, 2015-11-24 09:20:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-24 09:21:28]

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