Remove implicit conversions from scoped_refptr to T* in cc/trees/

Committed: https://crrev.com/eea980418ca9c235cdd6a93dd601035616f45616
Cr-Commit-Position: refs/heads/master@{#291807}

[dcheng, 2014-08-25 20:18:51]

Unfortunately, we can't make scoped_refptr testable until the conversion
operator has been removed. While it would be possible to disable the conversion
operator and make it testable in the same patch, the automated tool tries to
skip rewriting code where manual cleanups would be useful. This includes things
like:

// What if ReturnsScopedRefPtr() creates a new object internally?
if (ReturnsScopedRefPtr())

Foo* Bar() {
  const scoped_refptr<Foo>& f = ReturnsScopedRefPtr();
  return f;  // Does the return type need to be rewritten? Or is .get() safe?
}

Doing this in one patch would make catching some of these instances (in
particular, a boolean test on a temporary scoped_refptr) more difficult. As a
result, the work is being split into several steps:
Step 1: Rewrite all implicit conversions to be explicit (or rewrite the receiver
type to scoped_refptr<T>, if the tool believes the conversion to be unsafe).
Step 2: Once a platform compiles cleanly without the conversion operator, #ifdef
the conversion operator out.
Step 3: Make scoped_refptr testable on that platform.
Step 4: Remove unnecessary calls to .get().

[danakj, 2014-08-25 20:25:01]

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1949: scoped_refptr<ContextProvider>
context_provider =
I don't see anything this is passed to needing a scoped_refptr. Can you not just
use .get() here?

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1952:
GetMaxTransferBufferUsageBytes(context_provider);
If not you need .get() here and in most of these places below in this method.

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_impl.h#n...
cc/trees/layer_tree_impl.h:76: scoped_refptr<ContextProvider> context_provider()
const;
I don't think we want to add references to this thing freely. What code is
requiring this?

https://codereview.chromium.org/500123003/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/500123003/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1238: scoped_refptr<ContextProvider> context_provider =
can you just use .get() here? there's no need to take a ref

[dcheng, 2014-08-25 20:56:58]

Given your comments, do you think it'd make sense to change
OutputSurface::context_provider() to return a const scoped_refptr<T>& (in a
separate patch)?

This avoids refcount churn in cases where someone just needs to .get() it, but
you still have the advantages of having a scoped_refptr<T> type returned.

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1949: scoped_refptr<ContextProvider>
context_provider =
On 2014/08/25 20:25:01, danakj wrote:
> I don't see anything this is passed to needing a scoped_refptr. Can you not
just
> use .get() here?

This is the tool's logic kicking in--context_provider() returns a
scoped_refptr<T>. Something like Foo* f = CreateFoo() would be unsafe. But since
this is just a simple accessor, it should be safe--I'll add a little logic to
try to skip rewriting the receiver type in these cases. I've also manually
reverted this to get().

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1952:
GetMaxTransferBufferUsageBytes(context_provider);
On 2014/08/25 20:25:01, danakj wrote:
> If not you need .get() here and in most of these places below in this method.

Yeah, that's the problem with this tool--it's single pass. I should have run it
twice at least.

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/500123003/diff/1/cc/trees/layer_tree_impl.h#n...
cc/trees/layer_tree_impl.h:76: scoped_refptr<ContextProvider> context_provider()
const;
On 2014/08/25 20:25:01, danakj wrote:
> I don't think we want to add references to this thing freely. What code is
> requiring this?

This is the logic to try to catch temporary scoped_refptrs being returned
kicking in again.

Foo* foo() { return ReturnsScopedRefPtr(); } would be potentially unsafe.

But I've reverted this as well and just changed the implementation to call
.get().

https://codereview.chromium.org/500123003/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/500123003/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1238: scoped_refptr<ContextProvider> context_provider =
On 2014/08/25 20:25:01, danakj wrote:
> can you just use .get() here? there's no need to take a ref

Done (this was converted for the same reason).

[danakj, 2014-08-25 22:07:32]

> Given your comments, do you think it'd make sense to change
> OutputSurface::context_provider() to return a const scoped_refptr<T>& (in a
> separate patch)?

I thought that something in cc impl needs to hold a ref on it, but I think maybe
I am remembering wrong. If there's nothing that needs that ref, then yes let's
change that.

[danakj, 2014-08-25 22:07:54]

LGTM

[dcheng, 2014-08-25 22:08:32]

On 2014/08/25 at 22:07:32, danakj wrote:
> > Given your comments, do you think it'd make sense to change
> > OutputSurface::context_provider() to return a const scoped_refptr<T>& (in a
> > separate patch)?
> 
> I thought that something in cc impl needs to hold a ref on it, but I think
maybe I am remembering wrong. If there's nothing that needs that ref, then yes
let's change that.

OK, I'll file a bug to investigate this in a follow patch. Thanks for the
review!

[dcheng, 2014-08-25 22:22:29]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:24:15]

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

[commit-bot: I haz the power, 2014-08-25 23:00:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[commit-bot: I haz the power, 2014-08-26 00:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 00:07:03]

Failed to commit the patch.

[dcheng, 2014-08-26 00:28:18]

Committed patchset #2 to pending queue manually as 1de20e8 (presubmit
successful).

[commit-bot: I haz the power, 2014-09-10 02:39:13]

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