Change GpuDataManager to use Observer notifications rather than callbacks

Change GpuDataManager to use Observer notifications rather than callbacks.  This eliminates
use of legacy callbacks and generally simplifies the implementation. Also migrate one instance
of NewRunnableMethod to base::Bind().

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

[csilv, 2011-10-25 23:36:38]

+jhawkins, +zmo for review

[James Hawkins, 2011-10-25 23:36:51]

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:577: base::Unretained(this)));
Is base::Unretained() really needed here?  The conversion is from
NewRunnableMethod, which implies the ref-counting is desired.

[csilv, 2011-10-26 00:02:46]

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:577: base::Unretained(this)));
On 2011/10/25 23:36:52, James Hawkins wrote:
> Is base::Unretained() really needed here?  The conversion is from
> NewRunnableMethod, which implies the ref-counting is desired.

So, in the original implementation NewRunnableMethod is not ref counting due to
use of:

  DISABLE_RUNNABLE_METHOD_REFCOUNT(GpuDataManager);

So that line needs to be deleted and thus the above represents a correct
migration.  GpuDataManger is a singleton so stays around until app exit.  The
alternative would be to make GpuDataManger a ref-counted object and then get rid
of the Unretained usage.

[Zhenyao Mo, 2011-10-26 00:03:12]

LGTM after you address James's comment.

http://codereview.chromium.org/8390018/diff/6002/chrome/browser/ui/webui/trac...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/8390018/diff/6002/chrome/browser/ui/webui/trac...
chrome/browser/ui/webui/tracing_ui.cc:187: gpu_data_manager_->AddObserver(this);
Since we always call RemoveObserver() in destructor, we should move AddObserver
to constructor.

[csilv, 2011-10-26 00:16:03]

Just a followup... I'm observing that most instances of Singletons disable
ref-counting since due to the lifespan of the object.  So I think this is the
correct approach after along with the removal of
DISABLE_RUNNABLE_METHOD_REFCOUNT.

On 2011/10/26 00:02:46, csilv wrote:
>
http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
> File content/browser/gpu/gpu_data_manager.cc (right):
> 
>
http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
> content/browser/gpu/gpu_data_manager.cc:577: base::Unretained(this)));
> On 2011/10/25 23:36:52, James Hawkins wrote:
> > Is base::Unretained() really needed here?  The conversion is from
> > NewRunnableMethod, which implies the ref-counting is desired.
> 
> So, in the original implementation NewRunnableMethod is not ref counting due
to
> use of:
> 
>   DISABLE_RUNNABLE_METHOD_REFCOUNT(GpuDataManager);
> 
> So that line needs to be deleted and thus the above represents a correct
> migration.  GpuDataManger is a singleton so stays around until app exit.  The
> alternative would be to make GpuDataManger a ref-counted object and then get
rid
> of the Unretained usage.

[csilv, 2011-10-26 00:16:13]

http://codereview.chromium.org/8390018/diff/6002/chrome/browser/ui/webui/trac...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/8390018/diff/6002/chrome/browser/ui/webui/trac...
chrome/browser/ui/webui/tracing_ui.cc:187: gpu_data_manager_->AddObserver(this);
On 2011/10/26 00:03:12, Zhenyao Mo wrote:
> Since we always call RemoveObserver() in destructor, we should move
AddObserver
> to constructor.

This is actually a better approach to do it here.  This way, the callback will
only be invoked after the page is loaded.  If we move this to the constructor,
it may result in the handler trying to execute a javascript function before the
page is ready which yeilds a javascript error.

There's no harm in calling RemoveObserver if it was never added.

[zmo, 2011-10-26 00:18:45]

On 2011/10/26 00:16:13, csilv wrote:
>
http://codereview.chromium.org/8390018/diff/6002/chrome/browser/ui/webui/trac...
> File chrome/browser/ui/webui/tracing_ui.cc (right):
> 
>
http://codereview.chromium.org/8390018/diff/6002/chrome/browser/ui/webui/trac...
> chrome/browser/ui/webui/tracing_ui.cc:187:
gpu_data_manager_->AddObserver(this);
> On 2011/10/26 00:03:12, Zhenyao Mo wrote:
> > Since we always call RemoveObserver() in destructor, we should move
> AddObserver
> > to constructor.
> 
> This is actually a better approach to do it here.  This way, the callback will
> only be invoked after the page is loaded.  If we move this to the constructor,
> it may result in the handler trying to execute a javascript function before
the
> page is ready which yeilds a javascript error.
> 
> There's no harm in calling RemoveObserver if it was never added.

If it's no harm, then the current place looks good.

[James Hawkins, 2011-10-26 00:21:29]

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:577: base::Unretained(this)));
On 2011/10/26 00:02:47, csilv wrote:
> On 2011/10/25 23:36:52, James Hawkins wrote:
> > Is base::Unretained() really needed here?  The conversion is from
> > NewRunnableMethod, which implies the ref-counting is desired.
> 
> So, in the original implementation NewRunnableMethod is not ref counting due
to
> use of:
> 
>   DISABLE_RUNNABLE_METHOD_REFCOUNT(GpuDataManager);
> 
> So that line needs to be deleted and thus the above represents a correct
> migration.  GpuDataManger is a singleton so stays around until app exit.  The
> alternative would be to make GpuDataManger a ref-counted object and then get
rid
> of the Unretained usage.

Gotcha.

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.h (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:98: void AddObserver(Observer* observer);
|observer| must not be NULL?  Here and RemoveObserver().

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:100: // Unregisters |observer| from
receiving notifications.  This must be called
Single space between comments.

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:172: typedef
ObserverListThreadSafe<GpuDataManager::Observer>
Document var.

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:218: const
scoped_refptr<GpuDataManagerObserverList> observer_list_;
Why const?

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:225:
DISABLE_RUNNABLE_METHOD_REFCOUNT(GpuDataManager);
Assuming it's not safe to delete yet, add a TODO to remove.

[James Hawkins, 2011-10-26 01:32:16]

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.h (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:172: typedef
ObserverListThreadSafe<GpuDataManager::Observer>
On 2011/10/26 00:21:29, James Hawkins wrote:
> Document var.

Oops, nm.

[csilv, 2011-10-26 01:35:31]

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.h (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:98: void AddObserver(Observer* observer);
On 2011/10/26 00:21:29, James Hawkins wrote:
> |observer| must not be NULL?  Here and RemoveObserver().

Wouldn't observer != NULL be assumed?  ;-)  Done.

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:100: // Unregisters |observer| from
receiving notifications.  This must be called
On 2011/10/26 00:21:29, James Hawkins wrote:
> Single space between comments.

Done.

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:218: const
scoped_refptr<GpuDataManagerObserverList> observer_list_;
On 2011/10/26 00:21:29, James Hawkins wrote:
> Why const?

observer_list_ is initialized in the constructor and we don't need or use any
non-const methods.  This seems like the correct thing to do (and it's common in
our codebase.)  Any reason why I wouldn't want to use const here?

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:225:
DISABLE_RUNNABLE_METHOD_REFCOUNT(GpuDataManager);
On 2011/10/26 00:21:29, James Hawkins wrote:
> Assuming it's not safe to delete yet, add a TODO to remove.

Removed, I overlooked this in my prior CL.

[James Hawkins, 2011-10-26 01:45:28]

LGTM

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.h (right):

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:98: void AddObserver(Observer* observer);
On 2011/10/26 01:35:31, csilv wrote:
> On 2011/10/26 00:21:29, James Hawkins wrote:
> > |observer| must not be NULL?  Here and RemoveObserver().
> 
> Wouldn't observer != NULL be assumed?  ;-)  Done.

I understand where you're coming from, but I've recently gotten into the habit
of always documenting pointer variable requirements. For example, what if I'm
trying to use this in a situation where a pointer passed in may be NULL?  What
would happen in that case?  Documenting it just removes any possible ambiguity.

http://codereview.chromium.org/8390018/diff/6002/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:218: const
scoped_refptr<GpuDataManagerObserverList> observer_list_;
On 2011/10/26 01:35:31, csilv wrote:
> On 2011/10/26 00:21:29, James Hawkins wrote:
> > Why const?
> 
> observer_list_ is initialized in the constructor and we don't need or use any
> non-const methods.  This seems like the correct thing to do (and it's common
in
> our codebase.)  Any reason why I wouldn't want to use const here?

No, was just curious.

[commit-bot: I haz the power, 2011-10-26 01:55:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csilv@chromium.org/8390018/7002

[commit-bot: I haz the power, 2011-10-26 01:55:39]

Presubmit check for 8390018-7002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/gpu/gpu_data_manager.cc,content/browser/gpu/gpu_data_manager.h

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[csilv, 2011-10-26 17:26:48]

+apatrick for owners approval.

[csilv, 2011-10-28 18:53:45]

ping apatrick

[csilv, 2011-11-01 00:01:06]

+kbr for owner approval

[Ken Russell (switch to Gerrit), 2011-11-01 05:59:05]

lgtm