Implement WebFrameClient in RenderFrameImpl and proxy all calls to RenderView.

Implement WebFrameClient in RenderFrameImpl and proxy all calls to RenderView.

This is the first step in moving all WebFrameClient functionality into RenderViewImpl. Once this change is in, I will start moving methods few at a time, until all the code has migrated over.

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

[nasko, 2013-06-12 17:13:05]

Charlie,
Can you review this CL for me? It just adds WebFrameClient to RenderFrame and
proxies all the calls to RenderView. It is a first step in moving actual
implementation to RenderFrame.
Thanks,
Nasko

[Charlie Reis, 2013-06-12 20:32:29]

Cool, good start.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.cc:8: #include
"content/common/appcache/appcache_dispatcher.h"
Many of these (including this one) don't look like they're needed.  They're only
used in the render_view_impl.cc implementation.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.cc:22: using
appcache::WebApplicationCacheHostImpl;
Doesn't look like you need this, or some of the ones below.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:9: #include "base/compiler_specific.h"
Why is this needed?

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:10: #include
"content/common/navigation_gesture.h"
Doesn't look like you need this.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:11: #include
"content/public/common/referrer.h"
Doesn't look like you need this.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:22: public WebKit::WebFrameClient {
Should this have NON_EXPORTED_BASE as in render_view_impl.h?

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:23: 
nit: Remove blank line here.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:82: virtual WebKit::WebURLError
cannotHandleRequestError(
Hmm, why is there a canHandleRequest in render_view_impl.h?  It doesn't appear
to be in WebFrameClient, so I suppose you don't need it here?

Same question for assignIdentifierToRequest.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:131: 
nit: Remove blank line.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:164: virtual void
didFirstVisuallyNonEmptyLayout(WebKit::WebFrame* frame) OVERRIDE;
Thanks for ordering these the same as in WebFrameClient.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:885:
webview()->initializeMainFrame(main_render_frame_.get());
Does RenderViewImpl still need to derive from WebFrameClient?  I know all its
WebFrameClient methods will stick around for a while, but that will let us start
removing them one-by-one.

[nasko, 2013-06-12 21:36:00]

Some of the includes crept up from the bigger CL I carved this out of. All fixed
now. PTAL.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.cc:8: #include
"content/common/appcache/appcache_dispatcher.h"
On 2013/06/12 20:32:29, creis wrote:
> Many of these (including this one) don't look like they're needed.  They're
only
> used in the render_view_impl.cc implementation.

Done.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.cc:22: using
appcache::WebApplicationCacheHostImpl;
On 2013/06/12 20:32:29, creis wrote:
> Doesn't look like you need this, or some of the ones below.

Done.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:9: #include "base/compiler_specific.h"
On 2013/06/12 20:32:29, creis wrote:
> Why is this needed?

Needed for OVERRIDE, but it seems some other include brings it in. Removed.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:10: #include
"content/common/navigation_gesture.h"
On 2013/06/12 20:32:29, creis wrote:
> Doesn't look like you need this.

Done.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:11: #include
"content/public/common/referrer.h"
On 2013/06/12 20:32:29, creis wrote:
> Doesn't look like you need this.

Done.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:22: public WebKit::WebFrameClient {
On 2013/06/12 20:32:29, creis wrote:
> Should this have NON_EXPORTED_BASE as in render_view_impl.h?

Added in the following diff.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:23: 
On 2013/06/12 20:32:29, creis wrote:
> nit: Remove blank line here.

Done.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:82: virtual WebKit::WebURLError
cannotHandleRequestError(
On 2013/06/12 20:32:29, creis wrote:
> Hmm, why is there a canHandleRequest in render_view_impl.h?  It doesn't appear
> to be in WebFrameClient, so I suppose you don't need it here?
> 
> Same question for assignIdentifierToRequest.

They are just in the same area of the header file, they are not part of the
interface. Hence, I haven't included them here.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:131: 
On 2013/06/12 20:32:29, creis wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:164: virtual void
didFirstVisuallyNonEmptyLayout(WebKit::WebFrame* frame) OVERRIDE;
On 2013/06/12 20:32:29, creis wrote:
> Thanks for ordering these the same as in WebFrameClient.

You're welcome! It made my life easier too.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:885:
webview()->initializeMainFrame(main_render_frame_.get());
On 2013/06/12 20:32:29, creis wrote:
> Does RenderViewImpl still need to derive from WebFrameClient?  I know all its
> WebFrameClient methods will stick around for a while, but that will let us
start
> removing them one-by-one.

I didn't want to touch the header file for RenderViewImpl for this change. If I
remove the derivation, I need to remove a bunch of OVERRIDE keywords. I agree it
shouldn't derive and if you feel the code churn is worth now, I'll do it.

[Charlie Reis, 2013-06-12 22:16:10]

Great.  Looks good, pending the change to render_view_impl.h.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:82: virtual WebKit::WebURLError
cannotHandleRequestError(
On 2013/06/12 21:36:00, nasko wrote:
> On 2013/06/12 20:32:29, creis wrote:
> > Hmm, why is there a canHandleRequest in render_view_impl.h?  It doesn't
appear
> > to be in WebFrameClient, so I suppose you don't need it here?
> > 
> > Same question for assignIdentifierToRequest.
> 
> They are just in the same area of the header file, they are not part of the
> interface. Hence, I haven't included them here.

Ok, sounds good.  We can move/delete them in render_view_impl.h later. 
(Possibly dead code.)

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:885:
webview()->initializeMainFrame(main_render_frame_.get());
On 2013/06/12 21:36:00, nasko wrote:
> On 2013/06/12 20:32:29, creis wrote:
> > Does RenderViewImpl still need to derive from WebFrameClient?  I know all
its
> > WebFrameClient methods will stick around for a while, but that will let us
> start
> > removing them one-by-one.
> 
> I didn't want to touch the header file for RenderViewImpl for this change. If
I
> remove the derivation, I need to remove a bunch of OVERRIDE keywords. I agree
it
> shouldn't derive and if you feel the code churn is worth now, I'll do it.

I think we might as well.  It's conceptually tied to what you're doing here,
makes it so we keep only one WebFrameClient implementation, and sets you up to
start moving methods.

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

https://codereview.chromium.org/16223006/diff/24001/content/renderer/render_f...
content/renderer/render_frame_impl.h:19: : public RenderFrame,
nit: 2 more spaces

[Charlie Reis, 2013-06-12 23:06:34]

Sounds like changing RenderViewImpl to not derive from WebFrameClient is a
larger change in itself.  Let's move forward with this.  LGTM once the spacing
nit is fixed.

[nasko, 2013-06-12 23:13:28]

Fixed spacing and another instance where we pass "this" for a WebFrameClient.

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/16223006/diff/6001/content/renderer/render_fr...
content/renderer/render_frame_impl.h:82: virtual WebKit::WebURLError
cannotHandleRequestError(
On 2013/06/12 22:16:10, creis wrote:
> On 2013/06/12 21:36:00, nasko wrote:
> > On 2013/06/12 20:32:29, creis wrote:
> > > Hmm, why is there a canHandleRequest in render_view_impl.h?  It doesn't
> appear
> > > to be in WebFrameClient, so I suppose you don't need it here?
> > > 
> > > Same question for assignIdentifierToRequest.
> > 
> > They are just in the same area of the header file, they are not part of the
> > interface. Hence, I haven't included them here.
> 
> Ok, sounds good.  We can move/delete them in render_view_impl.h later. 
> (Possibly dead code.)

Done.

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

https://codereview.chromium.org/16223006/diff/24001/content/renderer/render_f...
content/renderer/render_frame_impl.h:19: : public RenderFrame,
On 2013/06/12 22:16:10, creis wrote:
> nit: 2 more spaces

Done.

[commit-bot: I haz the power, 2013-06-12 23:14:02]

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

[commit-bot: I haz the power, 2013-06-13 02:35:36]

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

[commit-bot: I haz the power, 2013-06-13 06:37:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
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-14 04:53:31]

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

[commit-bot: I haz the power, 2013-06-14 07:07:02]

Change committed as 206346