Refactored GLContext to own GPUTiming which spawn GPUTimingClients.

Refactored GLContext to own GPUTiming which spawn GPUTimingClients.

GPUTiming is our abstraction layer for various GL timing extensions. It
makes more sense for this abstraction layer to be owned by GLContext
instead. Clients that want to do GPU Timing calls (GPUTracer, eventual
support of WebGL tracer calls).

One of the main reasons for this differentiation is to support disjoint
queries across multiple virtual contexts. The actual support of this
will be completed in another CL.

BUG=451211
TEST=trybots

Committed: https://crrev.com/5b1c02ff50ebb8af95b50b4ea1658eef2bb89546
Cr-Commit-Position: refs/heads/master@{#318174}

[David Yen, 2015-02-24 19:46:47]

dyen@chromium.org changed reviewers:
+ dcastagna@chromium.org, kbr@chromium.org, vmiura@chromium.org

[David Yen, 2015-02-24 19:46:48]

This is the initial CL which replaces the last one which I had closed:
https://codereview.chromium.org/940633004/

All this CL does is refactor GPUTiming to GPUTimingClient which are created from
a parent GPUTiming class. GLContexts own a GPUTiming object to create these.

In cases where there are no GLContexts (unit tests), I also made the
GPUTimingClient constructor to be able to handle initialization to an invalid
timer, which is the same as if the GLContext did not support any timer
extensions.

PTAL.

[David Yen, 2015-02-24 19:48:30]

dyen@chromium.org changed reviewers:
+ piman@chromium.org

[David Yen, 2015-02-24 19:48:30]

piman@chromium.org: Please review changes in gpu/perftests/*

Sorry for the confusion yesterday, please review this one instead.

[vmiura, 2015-02-24 20:51:07]

https://codereview.chromium.org/937263006/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/937263006/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gpu_tracer.cc:94: gpu_timer_.reset(new
GPUTimer(gpu_timing_client));
It would be nicer to make GpuTimingClient the factory for GpuTimers, so making
GpuTimer() constructor private, and use gpu_timing_client->CreateTimer() instead
of constructing the timer here and passing the client to it.

[David Yen, 2015-02-24 22:13:01]

https://codereview.chromium.org/937263006/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/937263006/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gpu_tracer.cc:94: gpu_timer_.reset(new
GPUTimer(gpu_timing_client));
On 2015/02/24 20:51:07, vmiura wrote:
> It would be nicer to make GpuTimingClient the factory for GpuTimers, so making
> GpuTimer() constructor private, and use gpu_timing_client->CreateTimer()
instead
> of constructing the timer here and passing the client to it.

Done.

[vmiura, 2015-02-24 23:57:59]

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2479: gpu_state_tracer_ =
GPUStateTracer::Create(&state_);
nit: I think GPUStateTracer is not related to GPUTracer.  Revert this move?

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:145: gpu_timing_client_ =
context->GetGPUTiming()->CreateGPUTimingClient();
context->CreateGPUTimingClient() directly?

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.h:21: class GPUTiming;
nit: GPUTiming no longer used in this file.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.h#newco...
ui/gl/gl_context.h:100: gpu::GPUTiming* GetGPUTiming();
Do we need to return GPUTiming?  Can we have just have direct
CreateGPUTimingClient()?

Also, this should be virtual. GLContextVirtual needs to pass through to the real
underlying shared_context_.

[vmiura, 2015-02-25 00:10:38]

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.h#newco...
ui/gl/gl_context.h:100: gpu::GPUTiming* GetGPUTiming();
On 2015/02/24 23:57:59, vmiura wrote:
> Do we need to return GPUTiming?  Can we have just have direct
> CreateGPUTimingClient()?
> 
> Also, this should be virtual. GLContextVirtual needs to pass through to the
real
> underlying shared_context_.

Oh, probably make CreateGPUTimingClient() pure virtual here, and add
implementations to GlContextReal and GlContextVirtual.

[Ken Russell (switch to Gerrit), 2015-02-25 00:55:17]

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:71: class GL_EXPORT GPUTimingClient
I thought the design was going to be more of this form:

 - The GPUTiming class is hidden inside the GLContext
 - GLContext is the factory for GPUTimer instances
 - GPUTimingClient is pure virtual
 - Outside code registers one or more GPUTimingClients with the GLContext, and
these clients receive important notifications about the status of timers (like
the fact that a disjoint operation occurred)

Is this code an intermediate step toward that goal? Am I misunderstanding the
goal?

[piman, 2015-02-25 01:00:53]

Some nits

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.cc#newc...
ui/gl/gl_context.cc:118: gpu_timing_ = make_scoped_ptr(new
gpu::GPUTiming(this));
nit, shorter: gpu_timing_.reset(new gpu::GPUTiming(this));

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:16: namespace gpu {
nit: probably should be namespace gfx for consistency.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a description /
consider a more descriptive name?

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer instance we're
creating.
Should you just take a reference then?

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:65: friend class GPUTimingClient;
nit: friend declaration should be first in the private section.

[David Yen, 2015-02-25 01:47:36]

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2479: gpu_state_tracer_ =
GPUStateTracer::Create(&state_);
On 2015/02/24 23:57:58, vmiura wrote:
> nit: I think GPUStateTracer is not related to GPUTracer.  Revert this move?

Done.

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:145: gpu_timing_client_ =
context->GetGPUTiming()->CreateGPUTimingClient();
On 2015/02/24 23:57:58, vmiura wrote:
> context->CreateGPUTimingClient() directly?

Done.

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/937263006/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.h:21: class GPUTiming;
On 2015/02/24 23:57:58, vmiura wrote:
> nit: GPUTiming no longer used in this file.

Done.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.cc#newc...
ui/gl/gl_context.cc:118: gpu_timing_ = make_scoped_ptr(new
gpu::GPUTiming(this));
On 2015/02/25 01:00:52, piman (Very slow to review) wrote:
> nit, shorter: gpu_timing_.reset(new gpu::GPUTiming(this));

Done.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gl_context.h#newco...
ui/gl/gl_context.h:100: gpu::GPUTiming* GetGPUTiming();
On 2015/02/25 00:10:38, vmiura wrote:
> On 2015/02/24 23:57:59, vmiura wrote:
> > Do we need to return GPUTiming?  Can we have just have direct
> > CreateGPUTimingClient()?
> > 
> > Also, this should be virtual. GLContextVirtual needs to pass through to the
> real
> > underlying shared_context_.
> 
> Oh, probably make CreateGPUTimingClient() pure virtual here, and add
> implementations to GlContextReal and GlContextVirtual.

Done.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:16: namespace gpu {
On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> nit: probably should be namespace gfx for consistency.

Done.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a description
/
> consider a more descriptive name?

Let me discuss this with dcastagna since he was the original author.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer instance we're
creating.
On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> Should you just take a reference then?

Possibly, although in practice I don't think that is really an issue. I'll let
dcastagna comment on this since he wrote this code and the comment.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:65: friend class GPUTimingClient;
On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> nit: friend declaration should be first in the private section.

Done.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:71: class GL_EXPORT GPUTimingClient
On 2015/02/25 00:55:17, Ken Russell wrote:
> I thought the design was going to be more of this form:
> 
>  - The GPUTiming class is hidden inside the GLContext
>  - GLContext is the factory for GPUTimer instances
>  - GPUTimingClient is pure virtual
>  - Outside code registers one or more GPUTimingClients with the GLContext, and
> these clients receive important notifications about the status of timers (like
> the fact that a disjoint operation occurred)
> 
> Is this code an intermediate step toward that goal? Am I misunderstanding the
> goal?

Almost, as you said GPUTiming should be hidden inside of GLContextReal,
GLContext is the factory for GLTimingClients (this is done in the latest patch).

GPUTimingClient does not actually have to be subclassed so it doesn't need to be
virtualized.

[Daniele Castagna, 2015-02-25 17:42:00]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-02-25 17:42:02]

It'd be nice to have a few tests for the timers now that they live under gl.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
On 2015/02/25 at 01:47:35, David Yen wrote:
> On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a
description /
> > consider a more descriptive name?
> 
> Let me discuss this with dcastagna since he was the original author.

Do we still need to keep this class? Now that the timer offset logic is in
GPUTimingClient, all this class is doing is to figure out which extension to use
and store it in timer_type_.
Can we just make Initialize a static method that returns the TimerType and store
it inside the context instead of gpu_timer_?

I see that the context already has CreateGPUTimingClient, so the interface of
context wouldn't change.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer instance we're
creating.
On 2015/02/25 at 01:47:35, David Yen wrote:
> On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > Should you just take a reference then?
> 
> Possibly, although in practice I don't think that is really an issue. I'll let
dcastagna comment on this since he wrote this code and the comment.

I think a reference would be nicer but according to the style guide all the
reference arguments must be labeled as const
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
(someone please correct me if I missed something). Right now GPUTimer is using
gpu_timing_client in non const way.

[Daniele Castagna, 2015-02-25 17:42:06]

The CQ bit was unchecked by dcastagna@chromium.org

[David Yen, 2015-02-25 17:48:53]

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
On 2015/02/25 17:42:02, Daniele Castagna wrote:
> On 2015/02/25 at 01:47:35, David Yen wrote:
> > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > > nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a
> description /
> > > consider a more descriptive name?
> > 
> > Let me discuss this with dcastagna since he was the original author.
> 
> Do we still need to keep this class? Now that the timer offset logic is in
> GPUTimingClient, all this class is doing is to figure out which extension to
use
> and store it in timer_type_.
> Can we just make Initialize a static method that returns the TimerType and
store
> it inside the context instead of gpu_timer_?
> 
> I see that the context already has CreateGPUTimingClient, so the interface of
> context wouldn't change.

You are correct that currently it does almost nothing. This class will
eventually handle the disjoint context states as the previous CL did. Currently
it is only a refactor as a step in that direction.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer instance we're
creating.
On 2015/02/25 17:42:02, Daniele Castagna wrote:
> On 2015/02/25 at 01:47:35, David Yen wrote:
> > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > > Should you just take a reference then?
> > 
> > Possibly, although in practice I don't think that is really an issue. I'll
let
> dcastagna comment on this since he wrote this code and the comment.
> 
> I think a reference would be nicer but according to the style guide all the
> reference arguments must be labeled as const
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> (someone please correct me if I missed something). Right now GPUTimer is using
> gpu_timing_client in non const way.

I interpreted piman's comment as making this a scoped refcount of
GPUTimingClient, so it takes a reference count.

[piman, 2015-02-25 19:19:10]

On Wed, Feb 25, 2015 at 9:48 AM, <dyen@chromium.org> wrote:

>
> https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
> File ui/gl/gpu_timing.h (right):
>
> https://codereview.chromium.org/937263006/diff/60001/ui/
> gl/gpu_timing.h#newcode20
> ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
> On 2015/02/25 17:42:02, Daniele Castagna wrote:
>
>> On 2015/02/25 at 01:47:35, David Yen wrote:
>> > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
>> > > nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a
>> description /
>> > > consider a more descriptive name?
>> >
>> > Let me discuss this with dcastagna since he was the original author.
>>
>
>  Do we still need to keep this class? Now that the timer offset logic
>>
> is in
>
>> GPUTimingClient, all this class is doing is to figure out which
>>
> extension to use
>
>> and store it in timer_type_.
>> Can we just make Initialize a static method that returns the TimerType
>>
> and store
>
>> it inside the context instead of gpu_timer_?
>>
>
>  I see that the context already has CreateGPUTimingClient, so the
>>
> interface of
>
>> context wouldn't change.
>>
>
> You are correct that currently it does almost nothing. This class will
> eventually handle the disjoint context states as the previous CL did.
> Currently it is only a refactor as a step in that direction.
>
> https://codereview.chromium.org/937263006/diff/60001/ui/
> gl/gpu_timing.h#newcode57
> ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer
> instance we're creating.
> On 2015/02/25 17:42:02, Daniele Castagna wrote:
>
>> On 2015/02/25 at 01:47:35, David Yen wrote:
>> > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
>> > > Should you just take a reference then?
>> >
>> > Possibly, although in practice I don't think that is really an
>>
> issue. I'll let
>
>> dcastagna comment on this since he wrote this code and the comment.
>>
>
>  I think a reference would be nicer but according to the style guide
>>
> all the
>
>> reference arguments must be labeled as const
>>
>
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_
> Arguments
>
>> (someone please correct me if I missed something). Right now GPUTimer
>>
> is using
>
>> gpu_timing_client in non const way.
>>
>
> I interpreted piman's comment as making this a scoped refcount of
> GPUTimingClient, so it takes a reference count.
>

Right.

>
> https://codereview.chromium.org/937263006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Daniele Castagna, 2015-02-25 19:46:24]

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
On 2015/02/25 at 17:48:53, David Yen wrote:
> On 2015/02/25 17:42:02, Daniele Castagna wrote:
> > On 2015/02/25 at 01:47:35, David Yen wrote:
> > > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > > > nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a
> > description /
> > > > consider a more descriptive name?
> > > 
> > > Let me discuss this with dcastagna since he was the original author.
> > 
> > Do we still need to keep this class? Now that the timer offset logic is in
> > GPUTimingClient, all this class is doing is to figure out which extension to
use
> > and store it in timer_type_.
> > Can we just make Initialize a static method that returns the TimerType and
store
> > it inside the context instead of gpu_timer_?
> > 
> > I see that the context already has CreateGPUTimingClient, so the interface
of
> > context wouldn't change.
> 
> You are correct that currently it does almost nothing. This class will
eventually handle the disjoint context states as the previous CL did. Currently
it is only a refactor as a step in that direction.

It could be nice to have a comment explaining what's its intended purpose now? I
have no problem changing name, I just can't think of anything better...

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer instance we're
creating.
On 2015/02/25 at 17:48:53, David Yen wrote:
> On 2015/02/25 17:42:02, Daniele Castagna wrote:
> > On 2015/02/25 at 01:47:35, David Yen wrote:
> > > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > > > Should you just take a reference then?
> > > 
> > > Possibly, although in practice I don't think that is really an issue. I'll
let
> > dcastagna comment on this since he wrote this code and the comment.
> > 
> > I think a reference would be nicer but according to the style guide all the
> > reference arguments must be labeled as const
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> > (someone please correct me if I missed something). Right now GPUTimer is
using
> > gpu_timing_client in non const way.
> 
> I interpreted piman's comment as making this a scoped refcount of
GPUTimingClient, so it takes a reference count.

GPUTimingClient already extends base::RefCounted, so it makes sense. In the
previous code GPUTiming was not refcounted.

[David Yen, 2015-02-25 23:02:31]

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h
File ui/gl/gpu_timing.h (right):

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:20: class GL_EXPORT GPUTiming {
On 2015/02/25 19:46:24, Daniele Castagna wrote:
> On 2015/02/25 at 17:48:53, David Yen wrote:
> > On 2015/02/25 17:42:02, Daniele Castagna wrote:
> > > On 2015/02/25 at 01:47:35, David Yen wrote:
> > > > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > > > > nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a
> > > description /
> > > > > consider a more descriptive name?
> > > > 
> > > > Let me discuss this with dcastagna since he was the original author.
> > > 
> > > Do we still need to keep this class? Now that the timer offset logic is in
> > > GPUTimingClient, all this class is doing is to figure out which extension
to
> use
> > > and store it in timer_type_.
> > > Can we just make Initialize a static method that returns the TimerType and
> store
> > > it inside the context instead of gpu_timer_?
> > > 
> > > I see that the context already has CreateGPUTimingClient, so the interface
> of
> > > context wouldn't change.
> > 
> > You are correct that currently it does almost nothing. This class will
> eventually handle the disjoint context states as the previous CL did.
Currently
> it is only a refactor as a step in that direction.
> 
> It could be nice to have a comment explaining what's its intended purpose now?
I
> have no problem changing name, I just can't think of anything better...

I can't think of anything better either. I have added a comment at the top of
the file that describes all of the classes. Hopefully that will make things
clearer for the purposes of all 3 classes.

https://codereview.chromium.org/937263006/diff/60001/ui/gl/gpu_timing.h#newco...
ui/gl/gpu_timing.h:57: // gpu_timing_client must outlive GPUTimer instance we're
creating.
On 2015/02/25 19:46:24, Daniele Castagna wrote:
> On 2015/02/25 at 17:48:53, David Yen wrote:
> > On 2015/02/25 17:42:02, Daniele Castagna wrote:
> > > On 2015/02/25 at 01:47:35, David Yen wrote:
> > > > On 2015/02/25 01:00:53, piman (Very slow to review) wrote:
> > > > > Should you just take a reference then?
> > > > 
> > > > Possibly, although in practice I don't think that is really an issue.
I'll
> let
> > > dcastagna comment on this since he wrote this code and the comment.
> > > 
> > > I think a reference would be nicer but according to the style guide all
the
> > > reference arguments must be labeled as const
> > >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> > > (someone please correct me if I missed something). Right now GPUTimer is
> using
> > > gpu_timing_client in non const way.
> > 
> > I interpreted piman's comment as making this a scoped refcount of
> GPUTimingClient, so it takes a reference count.
> 
> GPUTimingClient already extends base::RefCounted, so it makes sense. In the
> previous code GPUTiming was not refcounted.

Done.

[piman, 2015-02-25 23:19:11]

LGTM when others are happy.

[vmiura, 2015-02-26 00:16:21]

LGTM

[Daniele Castagna, 2015-02-26 01:35:39]

LGTM.

[David Yen, 2015-02-26 01:36:10]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-02-26 01:36:56]

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

[commit-bot: I haz the power, 2015-02-26 01:54:16]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-26 01:55:23]

Patchset 8 (id:??) landed as
https://crrev.com/5b1c02ff50ebb8af95b50b4ea1658eef2bb89546
Cr-Commit-Position: refs/heads/master@{#318174}