Nuked WebFrameImplBase.

Nuked WebFrameImplBase to make Frame access more efficient.

This "hidden" base class was forcing two virtual calls to reach a Frame from a WebFrame. It is now one virtual call away.

BUG=713798
Review-Url: https://codereview.chromium.org/2837593002
Cr-Commit-Position: refs/heads/master@{#467706}
Committed: https://chromium.googlesource.com/chromium/src/+/6a56addf7e3a7f7a70558980a216b2d7081251ba

[mustaq, 2017-04-21 14:06:58]

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

[commit-bot: I haz the power, 2017-04-21 14:07:21]

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-04-21 15:57:38]

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

[commit-bot: I haz the power, 2017-04-21 15:57:39]

Dry run: This issue passed the CQ dry run.

[mustaq, 2017-04-21 17:28:53]

mustaq@chromium.org changed reviewers:
+ dcheng@chromium.org, dtapuska@chromium.org

[mustaq, 2017-04-21 17:28:53]

ptal. This seems working. In terms of visibility outside Blink,
WebFrame::GetFrame() now behaves exactly the same way as
WebFrame::ToBaseImpl()->GetFrame().

https://codereview.chromium.org/2837593002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2837593002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrame.h:114: virtual Frame* GetFrame() const =
0;
I think it makes sense to resolve this TODO in this CL. Wdyt?

[dtapuska, 2017-04-21 17:42:14]

On 2017/04/21 17:28:53, mustaq wrote:
> ptal. This seems working. In terms of visibility outside Blink,
> WebFrame::GetFrame() now behaves exactly the same way as
> WebFrame::ToBaseImpl()->GetFrame().
> 
>
https://codereview.chromium.org/2837593002/diff/1/third_party/WebKit/public/w...
> File third_party/WebKit/public/web/WebFrame.h (right):
> 
>
https://codereview.chromium.org/2837593002/diff/1/third_party/WebKit/public/w...
> third_party/WebKit/public/web/WebFrame.h:114: virtual Frame* GetFrame() const
=
> 0;
> I think it makes sense to resolve this TODO in this CL. Wdyt?

Sure it will compile but I'm deferring the review to dcheng@ as I've raised
concerns about exposing a virtual on a public interface that doesn't have access
to the class impl.

[dcheng, 2017-04-22 01:15:26]

On 2017/04/21 17:42:14, dtapuska wrote:
> On 2017/04/21 17:28:53, mustaq wrote:
> > ptal. This seems working. In terms of visibility outside Blink,
> > WebFrame::GetFrame() now behaves exactly the same way as
> > WebFrame::ToBaseImpl()->GetFrame().
> > 
> >
>
https://codereview.chromium.org/2837593002/diff/1/third_party/WebKit/public/w...
> > File third_party/WebKit/public/web/WebFrame.h (right):
> > 
> >
>
https://codereview.chromium.org/2837593002/diff/1/third_party/WebKit/public/w...
> > third_party/WebKit/public/web/WebFrame.h:114: virtual Frame* GetFrame()
const
> =
> > 0;
> > I think it makes sense to resolve this TODO in this CL. Wdyt?
> 
> Sure it will compile but I'm deferring the review to dcheng@ as I've raised
> concerns about exposing a virtual on a public interface that doesn't have
access
> to the class impl.

I'd rather not expose internal Blink functionality in the public API. As these
methods are virtual, it's impossible to hide them. The original idea behind this
was to use WebFrameImplBase to hide these details from the public API as much as
possible.

[mustaq, 2017-04-25 18:43:38]

ptal, moved to non-virtual static helper functions.

As of now, everything seems fine except that the webkit_unit_tests build can't
see the WebFrame.cpp implementations. Will look into it if the general approach
seems okay.

[dcheng, 2017-04-26 11:45:37]

Overall looks pretty good. Just a few small suggestions and one question.

Also, please update the CL description to briefly describe what this CL does and
why it's an improvement (fewer virtual calls).

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

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:327:
ToWebRemoteFrameImpl(frame)->InitializeCoreFrame(page, 0, g_null_atom);
I think this is missing an else before the NOTREACHED()

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

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:873:
CurrentDOMWindow(MainThreadIsolate()), WebFrame::CoreFrame(target),
Is it possible to omit WebFrame:: here?

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.h (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.h:107: // TODO(mustaq): regroup
these to WebFrame or WebRemoteFrame
I don't understand this TODO. What's your followup plan?

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TouchActionTest.cpp:303:
static_cast<LocalFrame*>(WebFrame::CoreFrame(web_view->MainFrame()));
Nit: ToLocalFrame() instead of static_cast while we're changing this anyway

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:427: static void
InitializeCoreFrame(WebFrame*, Page&);
Nit: WebFrame&, as the frame parameter should never be omitted.

Nit 2: group with TraceFrames() below, as it's conceptually somewhat similar.

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:428: static Frame* CoreFrame(const
WebFrame*);
Nit: ToCoreFrame

[mustaq, 2017-04-26 16:16:30]

Description was changed from

==========
Nuked WebFrameImplBase.

BUG=713798
==========

to

==========
Nuked WebFrameImplBase to make Frame access more efficient.

This "hidden" base class was forcing two virtual calls to reach a Frame from a
WebFrame. It is now one virtual call away.

BUG=713798
==========

[mustaq, 2017-04-26 18:12:49]

All done, ptal.

Still no clue why WebFrame.cpp is not visible to webkit_unit_tests. The build
rule third_party/WebKit/Source/web:web includes WebFrame.cpp, and
:webkit_unit_tests depends on :web. But still seeing a link  failure for
ToCoreFrame.

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

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:327:
ToWebRemoteFrameImpl(frame)->InitializeCoreFrame(page, 0, g_null_atom);
On 2017/04/26 11:45:33, dcheng (OOO through May 2) wrote:
> I think this is missing an else before the NOTREACHED()

Yikes! Done.

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

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:873:
CurrentDOMWindow(MainThreadIsolate()), WebFrame::CoreFrame(target),
On 2017/04/26 11:45:34, dcheng (OOO through May 2) wrote:
> Is it possible to omit WebFrame:: here?

Done.

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.h (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.h:107: // TODO(mustaq): regroup
these to WebFrame or WebRemoteFrame
On 2017/04/26 11:45:34, dcheng (OOO through May 2) wrote:
> I don't understand this TODO. What's your followup plan?

These were TODO for this CL. Done: moved them to non-overridden methods below.
Same in *Local*.h .

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TouchActionTest.cpp:303:
static_cast<LocalFrame*>(WebFrame::CoreFrame(web_view->MainFrame()));
On 2017/04/26 11:45:35, dcheng (OOO through May 2) wrote:
> Nit: ToLocalFrame() instead of static_cast while we're changing this anyway

Done.

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:427: static void
InitializeCoreFrame(WebFrame*, Page&);
On 2017/04/26 11:45:36, dcheng (OOO through May 2) wrote:
> Nit: WebFrame&, as the frame parameter should never be omitted.
> 
> Nit 2: group with TraceFrames() below, as it's conceptually somewhat similar.

Done #2.

#1: Ref params must be const ref according to the style guide, but a const is
not allowed here because it calls Web*FrameImpl::InitializeCoreFrame() which is
not a const function.

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:428: static Frame* CoreFrame(const
WebFrame*);
On 2017/04/26 11:45:37, dcheng (OOO through May 2) wrote:
> Nit: ToCoreFrame

Done.

But it seems |FromFrame| should now be called |FromCoreFrame| :(
Wdyt?

[mustaq, 2017-04-26 18:34:25]

ptal. Added the missing BLINK_EXPORT, thanks to Dave.

[mustaq, 2017-04-26 18:34:32]

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

[commit-bot: I haz the power, 2017-04-26 18:35:39]

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-04-26 20:28:59]

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

[commit-bot: I haz the power, 2017-04-26 20:29:00]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-27 02:22:37]

LGTM with comments addressed

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:427: static void
InitializeCoreFrame(WebFrame*, Page&);
On 2017/04/26 18:12:48, mustaq wrote:
> On 2017/04/26 11:45:36, dcheng (OOO through May 2) wrote:
> > Nit: WebFrame&, as the frame parameter should never be omitted.
> > 
> > Nit 2: group with TraceFrames() below, as it's conceptually somewhat
similar.
> 
> Done #2.
> 
> #1: Ref params must be const ref according to the style guide, but a const is
> not allowed here because it calls Web*FrameImpl::InitializeCoreFrame() which
is
> not a const function.

Mutable references are fine in Blink though: this is one of the style exceptions
that we carved out in Blink (hence Page& instead of Page*)

(And I think we should make ToCoreFrame take a ref as well, since all the
original callers must have passed a non-null WebFrame)

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:428: static Frame* CoreFrame(const
WebFrame*);
On 2017/04/26 18:12:48, mustaq wrote:
> On 2017/04/26 11:45:37, dcheng (OOO through May 2) wrote:
> > Nit: ToCoreFrame
> 
> Done.
> 
> But it seems |FromFrame| should now be called |FromCoreFrame| :(
> Wdyt?

Sure, we can do that in a separate change though.

[mustaq, 2017-04-27 14:21:21]

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:427: static void
InitializeCoreFrame(WebFrame*, Page&);
On 2017/04/27 02:22:36, dcheng (OOO through May 2) wrote:
> On 2017/04/26 18:12:48, mustaq wrote:
> > On 2017/04/26 11:45:36, dcheng (OOO through May 2) wrote:
> > > Nit: WebFrame&, as the frame parameter should never be omitted.
> > > 
> > > Nit 2: group with TraceFrames() below, as it's conceptually somewhat
> similar.
> > 
> > Done #2.
> > 
> > #1: Ref params must be const ref according to the style guide, but a const
is
> > not allowed here because it calls Web*FrameImpl::InitializeCoreFrame() which
> is
> > not a const function.
> 
> Mutable references are fine in Blink though: this is one of the style
exceptions
> that we carved out in Blink (hence Page& instead of Page*)
> 
> (And I think we should make ToCoreFrame take a ref as well, since all the
> original callers must have passed a non-null WebFrame)

Done.

https://codereview.chromium.org/2837593002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:428: static Frame* CoreFrame(const
WebFrame*);
On 2017/04/27 02:22:36, dcheng (OOO through May 2) wrote:
> On 2017/04/26 18:12:48, mustaq wrote:
> > On 2017/04/26 11:45:37, dcheng (OOO through May 2) wrote:
> > > Nit: ToCoreFrame
> > 
> > Done.
> > 
> > But it seems |FromFrame| should now be called |FromCoreFrame| :(
> > Wdyt?
> 
> Sure, we can do that in a separate change though.

Added a TODO.

[mustaq, 2017-04-27 14:43:35]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2017-04-27 14:43:36]

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/2837593002/#ps100001
(title: "Replaced non-null params with refs.")

[commit-bot: I haz the power, 2017-04-27 14:44:07]

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-04-27 16:34:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493304215910080,
"parent_rev": "b71e98cdf14f18cb967a73857826f6e8c568cea0", "commit_rev":
"6a56addf7e3a7f7a70558980a216b2d7081251ba"}

[commit-bot: I haz the power, 2017-04-27 16:34:26]

Description was changed from

==========
Nuked WebFrameImplBase to make Frame access more efficient.

This "hidden" base class was forcing two virtual calls to reach a Frame from a
WebFrame. It is now one virtual call away.

BUG=713798
==========

to

==========
Nuked WebFrameImplBase to make Frame access more efficient.

This "hidden" base class was forcing two virtual calls to reach a Frame from a
WebFrame. It is now one virtual call away.

BUG=713798

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

[commit-bot: I haz the power, 2017-04-27 16:34:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6a56addf7e3a7f7a70558980a216...