Manual fixups in compositor code scoped_refptr operator T* removal.

Manual fixups in compositor code scoped_refptr operator T* removal.

BUG=110610
Committed: https://crrev.com/fba633145b886957109efd5c0d745c4af45d4d59
Cr-Commit-Position: refs/heads/master@{#292432}

[dcheng, 2014-08-28 16:42:37]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[dcheng, 2014-08-28 16:42:37]

Manual fixups for cases that the automated tool skipped.  I've also added a TODO
in UniqueNotifier because I wasn't 100% sure if it was intentional--it seems
like most code usually refs a task runner. Please let me know what the right
thing to do there is.

[danakj, 2014-08-28 16:50:42]

https://codereview.chromium.org/514123002/diff/1/cc/output/delegating_rendere...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/514123002/diff/1/cc/output/delegating_rendere...
cc/output/delegating_renderer.cc:116: scoped_refptr<ContextProvider>
context_provider =
can you make this a raw pointer instead? no need to ref here.

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:92: base::SequencedTaskRunner* task_runner,
doesn't this need to be a scoped_refptr too?

https://codereview.chromium.org/514123002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (left):

https://codereview.chromium.org/514123002/diff/1/ui/compositor/layer.cc#oldco...
ui/compositor/layer.cc:665: std::mem_fun(&LayerAnimator::StopAnimating));
isn't mem_fun going to deref the iterator also? why doesn't this work and
(*it)-> does?

[dcheng, 2014-08-28 17:21:18]

https://codereview.chromium.org/514123002/diff/1/cc/output/delegating_rendere...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/514123002/diff/1/cc/output/delegating_rendere...
cc/output/delegating_renderer.cc:116: scoped_refptr<ContextProvider>
context_provider =
On 2014/08/28 16:50:42, danakj wrote:
> can you make this a raw pointer instead? no need to ref here.

Done.

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:92: base::SequencedTaskRunner* task_runner,
On 2014/08/28 16:50:42, danakj wrote:
> doesn't this need to be a scoped_refptr too?

I've tried to keep this patch to the bare minimum that was needed to make Chrome
compile with https://codereview.chromium.org/510323002 applied. I'm happy to
address this in a followup, if that's OK.

(Most likely, no one ever passed a temporary scoped_refptr here, which is rare,
since most the ways to get a TaskRunner return a scoped_refptr)

https://codereview.chromium.org/514123002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (left):

https://codereview.chromium.org/514123002/diff/1/ui/compositor/layer.cc#oldco...
ui/compositor/layer.cc:665: std::mem_fun(&LayerAnimator::StopAnimating));
On 2014/08/28 at 16:50:42, danakj wrote:
> isn't mem_fun going to deref the iterator also? why doesn't this work and
(*it)-> does?

I believe mem_fun actually expects a pointer. for_each dereferences the iterator
into a scoped_refptr, but with the conversion operator disabled, no such
conversion is possible.

[danakj, 2014-08-28 17:23:56]

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:357: task_runner,
see here

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:92: base::SequencedTaskRunner* task_runner,
On 2014/08/28 17:21:18, dcheng (OOO) wrote:
> On 2014/08/28 16:50:42, danakj wrote:
> > doesn't this need to be a scoped_refptr too?
> 
> I've tried to keep this patch to the bare minimum that was needed to make
Chrome
> compile with https://codereview.chromium.org/510323002 applied. I'm happy to
> address this in a followup, if that's OK.
> 
> (Most likely, no one ever passed a temporary scoped_refptr here, which is
rare,
> since most the ways to get a TaskRunner return a scoped_refptr)

This method is what calls the constructor which is now a const scoped_refptr&.
So surely Create is casting this raw pointer to a scoepd_refptr?

[dcheng, 2014-08-28 17:29:00]

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h#...
cc/resources/tile_manager.h:92: base::SequencedTaskRunner* task_runner,
On 2014/08/28 17:23:55, danakj wrote:
> On 2014/08/28 17:21:18, dcheng (OOO) wrote:
> > On 2014/08/28 16:50:42, danakj wrote:
> > > doesn't this need to be a scoped_refptr too?
> > 
> > I've tried to keep this patch to the bare minimum that was needed to make
> Chrome
> > compile with https://codereview.chromium.org/510323002 applied. I'm happy to
> > address this in a followup, if that's OK.
> > 
> > (Most likely, no one ever passed a temporary scoped_refptr here, which is
> rare,
> > since most the ways to get a TaskRunner return a scoped_refptr)
> 
> This method is what calls the constructor which is now a const scoped_refptr&.
> So surely Create is casting this raw pointer to a scoepd_refptr?

It is, but this is "fine" since scoped_refptr is implicitly constructible from
T*.

I do think this should be fixed, but I'd prefer to split it out into a separate
patch--I have a master patch with something like +300/-300 lines that I'm
landing in smaller patches, so minimizing the diffs/conflicts would be helpful
to getting the immediate issue fixed.

[danakj, 2014-08-28 17:37:27]

On 2014/08/28 17:29:00, dcheng (OOO) wrote:
> https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h
> File cc/resources/tile_manager.h (right):
> 
>
https://codereview.chromium.org/514123002/diff/1/cc/resources/tile_manager.h#...
> cc/resources/tile_manager.h:92: base::SequencedTaskRunner* task_runner,
> On 2014/08/28 17:23:55, danakj wrote:
> > On 2014/08/28 17:21:18, dcheng (OOO) wrote:
> > > On 2014/08/28 16:50:42, danakj wrote:
> > > > doesn't this need to be a scoped_refptr too?
> > > 
> > > I've tried to keep this patch to the bare minimum that was needed to make
> > Chrome
> > > compile with https://codereview.chromium.org/510323002 applied. I'm happy
to
> > > address this in a followup, if that's OK.
> > > 
> > > (Most likely, no one ever passed a temporary scoped_refptr here, which is
> > rare,
> > > since most the ways to get a TaskRunner return a scoped_refptr)
> > 
> > This method is what calls the constructor which is now a const
scoped_refptr&.
> > So surely Create is casting this raw pointer to a scoepd_refptr?
> 
> It is, but this is "fine" since scoped_refptr is implicitly constructible from
> T*.
> 
> I do think this should be fixed, but I'd prefer to split it out into a
separate
> patch--I have a master patch with something like +300/-300 lines that I'm
> landing in smaller patches, so minimizing the diffs/conflicts would be helpful
> to getting the immediate issue fixed.

Oh.. I thought that is what you were changing here. I guess this is changing the
constructor cuz of FakeTileManager passing up a MessageLoopProxy::current()
which is a scoped_refptr. Ok LGTM

[dcheng, 2014-08-28 17:40:57]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 17:41:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/514123002/40001

[commit-bot: I haz the power, 2014-08-28 18:56:54]

Committed patchset #3 (id:40001) as 14f54be2f91bbaf8eb61ded395af100a3aa1c20a

[commit-bot: I haz the power, 2014-09-10 03:01:11]

Patchset 3 (id:??) landed as
https://crrev.com/fba633145b886957109efd5c0d745c4af45d4d59
Cr-Commit-Position: refs/heads/master@{#292432}