Plumb Guest flag to RenderViewImpl

Plumb Guest flag to RenderViewImpl

This wil be used by a subsequent patch to create a WebGraphicsContext3DGuest
instead of a WebGraphicsContext3DCommandBufferImpl in the guest RenderView.

BUG=118659
TEST=manually


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127653

[Fady Samuel, 2012-03-16 20:33:44]

Hi John,

This is a plumbing only patch. The WebGraphicsContext3DGuest that will use this
plumbing will be a fairly large CL that will probably be iterated over several
times, but I think we agreed that this plumbing needs to happen regardless of
the details of the graphics context initialization.

Thanks,

Fady

[jam, 2012-03-18 19:02:33]

why does this stuff need to go into the public content API?

[Fady Samuel, 2012-03-19 15:28:29]

On 2012/03/18 19:02:33, John Abd-El-Malek wrote:
> why does this stuff need to go into the public content API?

You're right, it doesn't, removed! Thanks!

[Fady Samuel, 2012-03-19 15:28:58]

Hi John,

I removed the public content API change. Could you please take another look?
Thanks,

Fady

[jam, 2012-03-19 16:11:30]

http://codereview.chromium.org/9721001/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/9721001/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.h:214: virtual void
SetHostsGuest(bool hosts_guest) OVERRIDE;
I think this should be called "SetGuest" to be consistent with the RenderView
side. also HostsGuest is confusing, because it might sound like the opposite of
what it is

http://codereview.chromium.org/9721001/diff/6001/content/public/browser/rende...
File content/public/browser/render_view_host.h (right):

http://codereview.chromium.org/9721001/diff/6001/content/public/browser/rende...
content/public/browser/render_view_host.h:233: virtual void SetHostsGuest(bool
hosts_guest) = 0;
what about this one?

again it's not clear to me how it'll be used. but since the code for this
feature should all be in content, you don't this method on the interface. other
code in content will have access to the RenderViewHostImpl

[Fady Samuel, 2012-03-19 17:00:58]

On 2012/03/19 16:11:30, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/9721001/diff/6001/content/browser/renderer_hos...
> File content/browser/renderer_host/render_view_host_impl.h (right):
> 
>
http://codereview.chromium.org/9721001/diff/6001/content/browser/renderer_hos...
> content/browser/renderer_host/render_view_host_impl.h:214: virtual void
> SetHostsGuest(bool hosts_guest) OVERRIDE;
> I think this should be called "SetGuest" to be consistent with the RenderView
> side. also HostsGuest is confusing, because it might sound like the opposite
of
> what it is
> 
>
http://codereview.chromium.org/9721001/diff/6001/content/public/browser/rende...
> File content/public/browser/render_view_host.h (right):
> 
>
http://codereview.chromium.org/9721001/diff/6001/content/public/browser/rende...
> content/public/browser/render_view_host.h:233: virtual void SetHostsGuest(bool
> hosts_guest) = 0;
> what about this one?
> 
> again it's not clear to me how it'll be used. but since the code for this
> feature should all be in content, you don't this method on the interface.
other
> code in content will have access to the RenderViewHostImpl

I've been accessing things via the interfaces, and so I might need to change the
code a bit in a couple of places, but I believe this is unnecessary.

[Fady Samuel, 2012-03-19 17:15:05]

Hopefully I've addressed your concerns over public content API changes. I've
renamed SetHostsGuest to SetGuest.

The purpose of this plumbing is to delay the initialization of a
WebGraphicsContext3DGuest, and to use that Guest immediately, instead of
creating a WebGraphicsContext3DCommandBufferImpl.

To do the above, we need to be able to distinguish a guest from a host on the
construct of a guest. This flag allows us to do so.

[jam, 2012-03-19 19:58:43]

lgtm

[commit-bot: I haz the power, 2012-03-19 20:02:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/9721001/9001

[commit-bot: I haz the power, 2012-03-19 20:25:03]

Try job failure for 9721001-9001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-03-19 20:56:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/9721001/9004

[commit-bot: I haz the power, 2012-03-20 05:17:55]

Try job failure for 9721001-9004 (retry) on mac_rel for steps "ui_tests,
browser_tests, unit_tests, content_unittests".
It's a second try, previously, steps "ui_tests, browser_tests, unit_tests,
content_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-20 05:53:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/9721001/9004

[commit-bot: I haz the power, 2012-03-20 08:26:38]

Change committed as 127653

[Lei Zhang, 2012-03-20 19:30:57]

http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
content/renderer/render_view_impl.h:590: bool IsGuest() const;
Err, why is this under the "content::RenderView" section without being defined
in content/public/renderer/render_view.h ? Notice all the other methods have
OVERRIDEs.

[Fady Samuel, 2012-03-20 21:53:05]

On 2012/03/20 19:30:57, Lei Zhang wrote:
>
http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
> File content/renderer/render_view_impl.h (right):
> 
>
http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
> content/renderer/render_view_impl.h:590: bool IsGuest() const;
> Err, why is this under the "content::RenderView" section without being defined
> in content/public/renderer/render_view.h ? Notice all the other methods have
> OVERRIDEs.

Sorry, I'm not sure I understand what you're asking...did you want me to move it
down in the class? This flag doesn't need to be exposed in the public content
API.

[Lei Zhang, 2012-03-20 22:02:08]

On 2012/03/20 21:53:05, Fady Samuel wrote:
> On 2012/03/20 19:30:57, Lei Zhang wrote:
> >
>
http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
> > File content/renderer/render_view_impl.h (right):
> > 
> >
>
http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
> > content/renderer/render_view_impl.h:590: bool IsGuest() const;
> > Err, why is this under the "content::RenderView" section without being
defined
> > in content/public/renderer/render_view.h ? Notice all the other methods have
> > OVERRIDEs.
> 
> Sorry, I'm not sure I understand what you're asking...did you want me to move
it
> down in the class? This flag doesn't need to be exposed in the public content
> API.

The organization of the public methods in the header file is not random. The
section under:

// content::RenderView implementation

is for methods that implement/overrides virtual function defined in the public
header. IsGuest() doesn't belong there. I'd put it near line 228. Same goes for
SetGuest() in content/browser/renderer_host/render_view_host_impl.h.

[Fady Samuel, 2012-03-20 22:03:50]

On 2012/03/20 22:02:08, Lei Zhang wrote:
> On 2012/03/20 21:53:05, Fady Samuel wrote:
> > On 2012/03/20 19:30:57, Lei Zhang wrote:
> > >
> >
>
http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
> > > File content/renderer/render_view_impl.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9721001/diff/9004/content/renderer/render_view...
> > > content/renderer/render_view_impl.h:590: bool IsGuest() const;
> > > Err, why is this under the "content::RenderView" section without being
> defined
> > > in content/public/renderer/render_view.h ? Notice all the other methods
have
> > > OVERRIDEs.
> > 
> > Sorry, I'm not sure I understand what you're asking...did you want me to
move
> it
> > down in the class? This flag doesn't need to be exposed in the public
content
> > API.
> 
> The organization of the public methods in the header file is not random. The
> section under:
> 
> // content::RenderView implementation
> 
> is for methods that implement/overrides virtual function defined in the public
> header. IsGuest() doesn't belong there. I'd put it near line 228. Same goes
for
> SetGuest() in content/browser/renderer_host/render_view_host_impl.h.

Doing that now. Thanks. Sorry about that. I didn't notice the comment when
making the change.