Refactored GLContext to own GPUTiming which spawn GPUTimingClients.

ui/gl/gpu_timing.h

 // Copyright (c) 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef GPU_COMMAND_BUFFER_SERVICE_GPU_TIMING_H_
 #define GPU_COMMAND_BUFFER_SERVICE_GPU_TIMING_H_
 
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
-#include "gpu/gpu_export.h"
+#include "ui/gl/gl_export.h"
 
 namespace gfx {
 class GLContext;
 }
 
 namespace gpu {

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

nit: probably should be namespace gfx for consistency.

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

Done.

-class GPUTiming;
+
+class GPUTimingClient;
+
+class GL_EXPORT GPUTiming {

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

nit: it's not clear what is GPUTiming vs GPUTimer. Could you add a description /
consider a more descriptive name?

[David Yen, 2015/02/25 01:47:35]

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

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

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.

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

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.

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

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...

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

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.

+ public:
+  enum TimerType {
+    kTimerTypeInvalid = -1,
+
+    kTimerTypeARB,      // ARB_timer_query
+    kTimerTypeDisjoint  // EXT_disjoint_timer_query
+  };
+
+  explicit GPUTiming(gfx::GLContext* context = nullptr);
+  virtual ~GPUTiming();
+
+  void Initialize(gfx::GLContext* context);
+
+  TimerType GetTimerType() const { return timer_type_; }
+
+  scoped_refptr<GPUTimingClient> CreateGPUTimingClient();
+
+ private:
+  TimerType timer_type_ = kTimerTypeInvalid;
+  DISALLOW_COPY_AND_ASSIGN(GPUTiming);
+};
 
 // Class to compute the amount of time it takes to fully
 // complete a set of GL commands
-class GPU_EXPORT GPUTimer {
+class GL_EXPORT GPUTimer {
  public:
-  // gpu_timing must outlive GPUTimer instance we're creating.
-  explicit GPUTimer(GPUTiming* gpu_timing);
   ~GPUTimer();
 
   void Start();
   void End();
   bool IsAvailable();
 
   void GetStartEndTimestamps(int64* start, int64* end);
   int64 GetDeltaElapsed();
 
  private:
+  // gpu_timing_client must outlive GPUTimer instance we're creating.

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

Should you just take a reference then?

[David Yen, 2015/02/25 01:47:35]

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.

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

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.

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

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

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

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]

Done.

+  explicit GPUTimer(GPUTimingClient* gpu_timing_client);
+
   unsigned int queries_[2];
   int64 offset_ = 0;
   bool end_requested_ = false;
-  GPUTiming* gpu_timing_;
+  GPUTimingClient* gpu_timing_client_;
 
+  friend class GPUTimingClient;

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

nit: friend declaration should be first in the private section.

[David Yen, 2015/02/25 01:47:35]

Done.

   DISALLOW_COPY_AND_ASSIGN(GPUTimer);
 };
 
-// GPUTiming contains all the gl timing logic that is not specific
+// GPUTimingClient contains all the gl timing logic that is not specific
 // to a single GPUTimer.
-class GPU_EXPORT GPUTiming {
+class GL_EXPORT GPUTimingClient

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

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?

[David Yen, 2015/02/25 01:47:35]

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.

+    : public base::RefCounted<GPUTimingClient> {
  public:
-  enum TimerType {
-    kTimerTypeInvalid = -1,
+  explicit GPUTimingClient(GPUTiming* gpu_timing = nullptr);
 
-    kTimerTypeARB,      // ARB_timer_query
-    kTimerTypeDisjoint  // EXT_disjoint_timer_query
-  };
-
-  GPUTiming();
-  virtual ~GPUTiming();
-
-  bool Initialize(gfx::GLContext* context);
+  scoped_ptr<GPUTimer> CreateGPUTimer();
   bool IsAvailable();
+  const char* GetTimerTypeName() const;
 
   // CheckAndResetTimerErrors has to be called before reading timestamps
   // from GPUTimers instances and after making sure all the timers
   // were available.
   // If the returned value is false, all the previous timers should be
   // discarded.
   bool CheckAndResetTimerErrors();
 
-  const char* GetTimerTypeName() const;
-
   // Returns the offset between the current gpu time and the cpu time.
   int64 CalculateTimerOffset();
   void InvalidateTimerOffset();
 
   void SetCpuTimeForTesting(const base::Callback<int64(void)>& cpu_time);
-  void SetOffsetForTesting(int64 offset, bool cache_it);
-  void SetTimerTypeForTesting(TimerType type);
 
- private:
-  TimerType timer_type_ = kTimerTypeInvalid;
+ protected:
+  virtual ~GPUTimingClient();
+
+  GPUTiming* gpu_timing_;
+  GPUTiming::TimerType timer_type_ = GPUTiming::kTimerTypeInvalid;
   int64 offset_ = 0;  // offset cache when timer_type_ == kTimerTypeARB
   bool offset_valid_ = false;
   base::Callback<int64(void)> cpu_time_for_testing_;
+
+  friend class base::RefCounted<GPUTimingClient>;
   friend class GPUTimer;
-  DISALLOW_COPY_AND_ASSIGN(GPUTiming);
+  friend class GPUTiming;
+  DISALLOW_COPY_AND_ASSIGN(GPUTimingClient);
 };
+
 }  // namespace gpu
 
 #endif  // GPU_COMMAND_BUFFER_SERVICE_GPU_TIMING_H_