Remove use of weakptr for RemovableDeviceNotificationsMac

Remove use of weakptr for RemovableDeviceNotificationsMac

BUG=


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

[vandebo (ex-Chrome), 2012-09-18 22:41:56]

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_mac.mm
(right):

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_mac.mm:150:
GetDiskInfoAndUpdate(notifications,
Does this work? raw pointer passed to function that cast const ref to refptr? 
Maybe the above functions should just take the raw pointer?

[sail, 2012-09-18 22:52:49]

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_mac.mm
(right):

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_mac.mm:150:
GetDiskInfoAndUpdate(notifications,
On 2012/09/18 22:41:56, vandebo wrote:
> Does this work? raw pointer passed to function that cast const ref to refptr? 

Yep! When the raw pointer is passed to GetDiskInfoAndUpdate() it creates a new
scoped_refptr<> object. This object will ensure that the
RemovableDeviceNotificationsMac instance will stay alive while the task is
pending.

> Maybe the above functions should just take the raw pointer?

I think using refcounted object is better than raw pointers since it guarantees
that the instance will be alive when the task complets.

We could use the global object and not pass a pointer at all. I think this is
not as great because then that force this object to be a singleton.

[vandebo (ex-Chrome), 2012-09-18 23:03:47]

LGTM

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_mac.mm
(right):

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_mac.mm:150:
GetDiskInfoAndUpdate(notifications,
On 2012/09/18 22:52:49, sail wrote:
> On 2012/09/18 22:41:56, vandebo wrote:
> > Does this work? raw pointer passed to function that cast const ref to
refptr? 
> 
> Yep! When the raw pointer is passed to GetDiskInfoAndUpdate() it creates a new
> scoped_refptr<> object. This object will ensure that the
> RemovableDeviceNotificationsMac instance will stay alive while the task is
> pending.
> 
> > Maybe the above functions should just take the raw pointer?
> 
> I think using refcounted object is better than raw pointers since it
guarantees
> that the instance will be alive when the task completes.

What I meant is that maybe GetDiskInfoAndUpdate could take a raw pointer.  Bind
will take a reference when it stores the value.  callback.h isn't very clear on
this point, but the way I understand things, if the type of one of the arguments
is refcount'able, then it takes a reference when it stores it for currying. I
don't think it has to be a scoped_refptr for that to work. By creating a
scoped_refptr in the argument list you're doing an extra ref unref that isn't
strictly needed.  But I could be wrong about this.

> We could use the global object and not pass a pointer at all. I think this is
> not as great because then that force this object to be a singleton.

Agreed.

[sail, 2012-09-18 23:28:50]

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_mac.mm
(right):

https://codereview.chromium.org/10948019/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_mac.mm:150:
GetDiskInfoAndUpdate(notifications,
On 2012/09/18 23:03:47, vandebo wrote:
> On 2012/09/18 22:52:49, sail wrote:
> > On 2012/09/18 22:41:56, vandebo wrote:
> > > Does this work? raw pointer passed to function that cast const ref to
> refptr? 
> > 
> > Yep! When the raw pointer is passed to GetDiskInfoAndUpdate() it creates a
new
> > scoped_refptr<> object. This object will ensure that the
> > RemovableDeviceNotificationsMac instance will stay alive while the task is
> > pending.
> > 
> > > Maybe the above functions should just take the raw pointer?
> > 
> > I think using refcounted object is better than raw pointers since it
> guarantees
> > that the instance will be alive when the task completes.
> 
> What I meant is that maybe GetDiskInfoAndUpdate could take a raw pointer. 
Bind
> will take a reference when it stores the value.  callback.h isn't very clear
on
> this point, but the way I understand things, if the type of one of the
arguments
> is refcount'able, then it takes a reference when it stores it for currying. I
> don't think it has to be a scoped_refptr for that to work. By creating a
> scoped_refptr in the argument list you're doing an extra ref unref that isn't
> strictly needed.  But I could be wrong about this.

Ahh, I see what you mean.
I tried simply removing scoped_refptr<> from GetDiskInfoAndUpdate() but it
didn't work. It seems like base::bind() isn't smart enough to automatically do
this.

> 
> > We could use the global object and not pass a pointer at all. I think this
is
> > not as great because then that force this object to be a singleton.
> 
> Agreed.
>

[commit-bot: I haz the power, 2012-09-18 23:29:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10948019/1

[commit-bot: I haz the power, 2012-09-18 23:29:13]

Presubmit check for 10948019-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.2s to calculate.

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

[sail, 2012-09-18 23:32:23]

+thakis for chrome/* OWNERS approval

[Nico, 2012-09-19 03:51:11]

lgtm stamp

[commit-bot: I haz the power, 2012-09-19 04:44:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10948019/1

[commit-bot: I haz the power, 2012-09-19 08:00:51]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-19 16:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10948019/1

[commit-bot: I haz the power, 2012-09-19 18:18:59]

Change committed as 157569