Refactor RenderView creation to use ViewMsg_New_Params.

Refactor RenderView creation to use ViewMsg_New_Params.

Using a RenderViewImplParams struct to create RenderViewImpl objects was introduced in crrev.com/167341. It doesn't look necessary anymore, so I'm removing it as it adds an unnecessary step of converting one struct to another identicial one.

BUG=357747
TEST=Layout tests and content_browsertests should still work.

TBR=jochen@chromium.org

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

[nasko, 2014-11-24 19:02:04]

Patchset #1 (id:1) has been deleted

[nasko, 2014-11-24 19:02:27]

nasko@chromium.org changed reviewers:
+ creis@chromium.org

[nasko, 2014-11-24 19:02:28]

Hey Charlie,
Can you review this CL for me?
Thanks in advance!
Nasko

[Charlie Reis, 2014-11-24 23:28:21]

creis@chromium.org changed reviewers:
+ jochen@chromium.org

[Charlie Reis, 2014-11-24 23:28:21]

[+jochen for the TBR line]

LGTM, since this seems like an improvement and I don't see a better way.

https://codereview.chromium.org/751043002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/751043002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.h:169: const ViewMsg_New_Params& params);
I kind of wish we could do this without taking an IPC struct here, but I think
this is better than requiring a manual copy between two identical structs.

The closest I could come up with is FrameNavigateParams (in content/common),
which is used in FrameHostMsg_DidCommitProvisionalLoad_Params via
IPC_STRUCT_TRAITS_PARENT.  That doesn't solve this problem, though, since we
would still need to take in the IPC struct here.  (For comparison,
NavigationControllerImpl::RendererDidNavigate takes in
FrameHostMsg_DidCommitProvisionalLoad_Params, not FrameNavigateParams.)

https://codereview.chromium.org/751043002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.h:528: void Initialize(bool
is_renderer_created, const ViewMsg_New_Params& params);
nit: is_renderer_created isn't a great name, since it's easily misinterpreted. 
This code runs in the renderer process, so the renderer has necessarily been
created.

Maybe was_created_by_renderer?

I'd also list it second, since it's a supplement to params rather than something
that heavily affects the behavior of this method.

[nasko, 2014-11-24 23:47:55]

https://codereview.chromium.org/751043002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/751043002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.h:169: const ViewMsg_New_Params& params);
On 2014/11/24 23:28:21, Charlie Reis wrote:
> I kind of wish we could do this without taking an IPC struct here, but I think
> this is better than requiring a manual copy between two identical structs.
> 
> The closest I could come up with is FrameNavigateParams (in content/common),
> which is used in FrameHostMsg_DidCommitProvisionalLoad_Params via
> IPC_STRUCT_TRAITS_PARENT.  That doesn't solve this problem, though, since we
> would still need to take in the IPC struct here.  (For comparison,
> NavigationControllerImpl::RendererDidNavigate takes in
> FrameHostMsg_DidCommitProvisionalLoad_Params, not FrameNavigateParams.)

Thanks for looking into this! FYI, I've applied the same transform as for
Initialize to rename the boolean and add it as second param.

https://codereview.chromium.org/751043002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.h:528: void Initialize(bool
is_renderer_created, const ViewMsg_New_Params& params);
On 2014/11/24 23:28:21, Charlie Reis wrote:
> nit: is_renderer_created isn't a great name, since it's easily misinterpreted.

> This code runs in the renderer process, so the renderer has necessarily been
> created.
> 
> Maybe was_created_by_renderer?
> 
> I'd also list it second, since it's a supplement to params rather than
something
> that heavily affects the behavior of this method.

Done.

[Charlie Reis, 2014-11-24 23:53:24]

LGTM

[nasko, 2014-11-24 23:54:00]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-11-24 23:54:24]

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

[commit-bot: I haz the power, 2014-11-25 01:13:49]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2014-11-25 01:14:34]

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