Implement WebFrameClient in RenderFrame and proxy all calls to RenderView (for now).

Implement WebFrameClient in RenderFrame and proxy all calls to RenderView (for now).

This is continuation of the original CL (https://codereview.chromium.org/16223006/), which didn't account for Blink tests using WebTestProxy.

BUG=245126
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207825

[jochen (gone - plz use gerrit), 2013-06-19 16:09:28]

lgtm (assuming it passes layout tests)

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:891:
//webview()->initializeMainFrame(this);
nit. just delete this

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.h:690: virtual WebKit::WebString
acceptLanguages() OVERRIDE;
nit. while you're removing the OVERRIDEs, why not this one as well?

[nasko, 2013-06-19 17:15:03]

Including creis@ as FYI.

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:891:
//webview()->initializeMainFrame(this);
On 2013/06/19 16:09:28, jochen wrote:
> nit. just delete this

Done.

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.h:690: virtual WebKit::WebString
acceptLanguages() OVERRIDE;
On 2013/06/19 16:09:28, jochen wrote:
> nit. while you're removing the OVERRIDEs, why not this one as well?

I'm removing only the ones that are part of WebFrameClient, as I will be
removing the inheritance, once the Blink changes make it in. acceptLanguages is
part of WebViewClient and it is still being overridden, so it should stay.

nit: It will be great to enforce the override keyword the way we do it for the
rest of Chromium.

[jochen (gone - plz use gerrit), 2013-06-19 18:55:24]

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/17114006/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.h:690: virtual WebKit::WebString
acceptLanguages() OVERRIDE;
On 2013/06/19 17:15:03, nasko wrote:
> On 2013/06/19 16:09:28, jochen wrote:
> > nit. while you're removing the OVERRIDEs, why not this one as well?
> 
> I'm removing only the ones that are part of WebFrameClient, as I will be
> removing the inheritance, once the Blink changes make it in. acceptLanguages
is
> part of WebViewClient and it is still being overridden, so it should stay.
> 
> nit: It will be great to enforce the override keyword the way we do it for the
> rest of Chromium.

We don't use OVERRIDE on blink API implementations, because it makes changing
the API unnecessarily difficult

[commit-bot: I haz the power, 2013-06-20 18:32:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/17114006/21001

[commit-bot: I haz the power, 2013-06-20 19:10:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jam, 2013-06-20 19:25:39]

https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
content/renderer/render_frame_impl.h:29: RenderFrameImpl*
(*create_render_frame_impl)(RenderViewImpl*, int32));
nit: why isn't this a callback?

[nasko, 2013-06-20 20:25:42]

https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
content/renderer/render_frame_impl.h:29: RenderFrameImpl*
(*create_render_frame_impl)(RenderViewImpl*, int32));
On 2013/06/20 19:25:39, jam wrote:
> nit: why isn't this a callback?

In general, I've tried to keep the same pattern that is used with RenderViewImpl
for consistency.

[jam, 2013-06-20 21:40:09]

https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
content/renderer/render_frame_impl.h:29: RenderFrameImpl*
(*create_render_frame_impl)(RenderViewImpl*, int32));
On 2013/06/20 20:25:42, nasko wrote:
> On 2013/06/20 19:25:39, jam wrote:
> > nit: why isn't this a callback?
> 
> In general, I've tried to keep the same pattern that is used with
RenderViewImpl
> for consistency.

ah. just saw that. is that RVImpl one going away after this one?
it would just be nice to use callbacks as that's the standard way now in chrome
code. but i dont feel strongly about it

[nasko, 2013-06-20 21:43:45]

On 2013/06/20 21:40:09, jam wrote:
>
https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
> File content/renderer/render_frame_impl.h (right):
> 
>
https://codereview.chromium.org/17114006/diff/21001/content/renderer/render_f...
> content/renderer/render_frame_impl.h:29: RenderFrameImpl*
> (*create_render_frame_impl)(RenderViewImpl*, int32));
> On 2013/06/20 20:25:42, nasko wrote:
> > On 2013/06/20 19:25:39, jam wrote:
> > > nit: why isn't this a callback?
> > 
> > In general, I've tried to keep the same pattern that is used with
> RenderViewImpl
> > for consistency.
> 
> ah. just saw that. is that RVImpl one going away after this one?
> it would just be nice to use callbacks as that's the standard way now in
chrome
> code. but i dont feel strongly about it

No, the RVImpl one won't be going away. We can do a follow up CL at a later
point to clean those two up and convert them to using Callback.

[commit-bot: I haz the power, 2013-06-20 21:46:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/17114006/21001

[commit-bot: I haz the power, 2013-06-20 22:02:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 13:46:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/17114006/21001

[commit-bot: I haz the power, 2013-06-21 15:23:42]

Change committed as 207825