Use weak pointer to where possible

Use weak pointer where possible.
* Use weak pointer for cros settings callback
* Pass weak pointer to the thread pool task so that calling
 back to UI thread is safe.

BUG=349083
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278263

[oshima, 2014-06-18 01:26:42]

another try to eliminate a couple of more Unretained

[Alexander Alekseev, 2014-06-18 12:58:23]

On 2014/06/18 01:26:42, oshima wrote:
> another try to eliminate a couple of more Unretained

It seems that it has been already (partially?) commited in
https://codereview.chromium.org/322233002 .

[bshe, 2014-06-18 13:33:31]

On 2014/06/18 12:58:23, alemate wrote:
> On 2014/06/18 01:26:42, oshima wrote:
> > another try to eliminate a couple of more Unretained
> 
> It seems that it has been already (partially?) commited in
> https://codereview.chromium.org/322233002 .

Thanks for taking care of this.
It looks like you reverted your previous CL and this one looks the same to
previous one (or the diff is tiny and I missed it).

[Alexander Alekseev, 2014-06-18 19:11:19]

https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1043:
weak_ptr,
This seems to be dangerous, as WeakPtr is not thread-safe.

|weak_ptr| was created on UI thread, and will be destroyed on UI thread.

We are on Worker Pool now. So, a new WeakPtr is created here, and it will be
owned (and destroyed) by current thread.

Correct, but ugly idea: WallpaperManager could actually be:

class WallpaperManager {
 public:
  ~WallpaperManager() {
    impl.destroy(); // Mark internal state "destroyed"
  }
 private:
  scoped_ptr<WallpaperManagerImpl> impl;
};

And then WallpaperManagerImpl might actually be RefCountedThreadSafe.
But we'll have to check "destroyed" state in all cross-threads methods...

[oshima, 2014-06-18 19:25:25]

https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1043:
weak_ptr,
On 2014/06/18 19:11:18, alemate wrote:
> This seems to be dangerous, as WeakPtr is not thread-safe.

It's safe to pass WeakPtr between threads. The dereferencing has to be done on
the same thread it was craeted. See the document in base/memory/weak_ptr.h.

> |weak_ptr| was created on UI thread, and will be destroyed on UI thread.
> 
> We are on Worker Pool now. So, a new WeakPtr is created here, and it will be
> owned (and destroyed) by current thread.
> 
> Correct, but ugly idea: WallpaperManager could actually be:
> 
> class WallpaperManager {
>  public:
>   ~WallpaperManager() {
>     impl.destroy(); // Mark internal state "destroyed"
>   }
>  private:
>   scoped_ptr<WallpaperManagerImpl> impl;
> };
> 
> And then WallpaperManagerImpl might actually be RefCountedThreadSafe.
> But we'll have to check "destroyed" state in all cross-threads methods...

[Alexander Alekseev, 2014-06-18 19:57:58]

lgtm

https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1043:
weak_ptr,
On 2014/06/18 19:25:25, oshima wrote:
> On 2014/06/18 19:11:18, alemate wrote:
> > This seems to be dangerous, as WeakPtr is not thread-safe.
> 
> It's safe to pass WeakPtr between threads. The dereferencing has to be done on
> the same thread it was craeted. See the document in base/memory/weak_ptr.h.


Yes, you're right.

[bshe, 2014-06-18 21:13:01]

On 2014/06/18 19:57:58, alemate wrote:
> lgtm
> 
>
https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
(right):
> 
>
https://codereview.chromium.org/341713004/diff/20001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1043:
> weak_ptr,
> On 2014/06/18 19:25:25, oshima wrote:
> > On 2014/06/18 19:11:18, alemate wrote:
> > > This seems to be dangerous, as WeakPtr is not thread-safe.
> > 
> > It's safe to pass WeakPtr between threads. The dereferencing has to be done
on
> > the same thread it was craeted. See the document in base/memory/weak_ptr.h.
> 
> 
> Yes, you're right.

lgtm

[oshima, 2014-06-18 21:14:25]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-18 21:15:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/341713004/20001

[commit-bot: I haz the power, 2014-06-19 04:08:55]

Change committed as 278263