Remove implicit conversions from scoped_refptr to T* in chrome/browser/extensions/api/

Remove implicit conversions from scoped_refptr to T* in chrome/browser/extensions/api/

This patch was generated by running the rewrite_scoped_refptr clang tool
on a Linux build.

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

[dcheng, 2014-08-26 00:50:20]

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

[dcheng, 2014-08-26 00:50:20]

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().

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:90:
scoped_refptr<ContentSettingsStore> store =
Note: this variable type was rewritten instead of simply adding .get(), because
generally speaking, there is no way of determining if this is safe or not.

If this is safe, please let me know, and I will change this to call .get()
instead of rewriting the variable type. Also consider changing
content_settings_store() to return a const scoped_refptr<>& instead--this will
help avoid spurious refcount churn for callers that really don't need to retain
a reference. (I can also make this change, but please let me know if I should or
not.)

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:241:
scoped_refptr<ContentSettingsStore> store =
Note: this variable type was rewritten instead of simply adding .get(), because
generally speaking, there is no way of determining if this is safe or not.

If this is safe, please let me know, and I will change this to call .get()
instead of rewriting the variable type. Also consider changing
content_settings_store() to return a const scoped_refptr<>& instead--this will
help avoid spurious refcount churn for callers that really don't need to retain
a reference. (I can also make this change, but please let me know if I should or
not.)

[Jeffrey Yasskin, 2014-08-26 00:57:51]

jyasskin@chromium.org changed reviewers:
+ rockot@chromium.org

[Jeffrey Yasskin, 2014-08-26 01:14:16]

lgtm to avoid slowing you down, but there may be a bug in your script.

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc
(right):

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc:58:
ON_CALL(*mock_adapter_.get(), GetDevice(mock_device_->GetAddress()))
Adding .get() shouldn't affect how this works, since it's getting dereferenced
either way.

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:90:
scoped_refptr<ContentSettingsStore> store =
On 2014/08/26 00:50:20, dcheng (OOO) wrote:
> Note: this variable type was rewritten instead of simply adding .get(),
because
> generally speaking, there is no way of determining if this is safe or not.
> 
> If this is safe, please let me know, and I will change this to call .get()
> instead of rewriting the variable type. Also consider changing
> content_settings_store() to return a const scoped_refptr<>& instead--this will
> help avoid spurious refcount churn for callers that really don't need to
retain
> a reference. (I can also make this change, but please let me know if I should
or
> not.)

The change that introduced the scoped_refptr return changed several other places
to store the result into a scoped_refptr, so I'd guess .get() isn't safe.
Returning by & might work, but +rockot to be sure.

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:178:
->Contains(*requested_permissions_.get())) {
You shouldn't need to add a .get() to something that's already dereferencing the
scoped_refptr.

[dcheng, 2014-08-26 01:26:01]

Do you think it'd make sense to add operator* to scoped_refptr? This would make
it behave more like scoped_ptr, etc.

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc
(right):

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc:58:
ON_CALL(*mock_adapter_.get(), GetDevice(mock_device_->GetAddress()))
On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
> Adding .get() shouldn't affect how this works, since it's getting dereferenced
> either way.

Before anything else, I'd like to say that I really hate macros.

I think this is actually necessary due to the way the gtest macro is
constructed:
#define GMOCK_ON_CALL_IMPL_(obj, call) \
    ((obj).gmock_##call).InternalDefaultActionSetAt(__FILE__, __LINE__, \
                                                    #obj, #call)
#define ON_CALL(obj, call) GMOCK_ON_CALL_IMPL_(obj, call)

(Un?)fortunately, scoped_refptr does not currently implement operator*, so I
think it's necessary to explicitly call .get() to unwrap the scoped_refptr so we
can pass a T& to gmock.

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/permissions/permissions_api.cc:178:
->Contains(*requested_permissions_.get())) {
On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
> You shouldn't need to add a .get() to something that's already dereferencing
the
> scoped_refptr.

scoped_refptr doesn't implement operator*, for better or worse.

[Jeffrey Yasskin, 2014-08-26 01:34:50]

Ah, yes. Anything that implements operator-> or pretends to be a
pointer should implement operator*.

On Mon, Aug 25, 2014 at 6:26 PM,  <dcheng@chromium.org> wrote:
> Do you think it'd make sense to add operator* to scoped_refptr? This would
> make
> it behave more like scoped_ptr, etc.
>
>
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> File
> chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc
> (right):
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc:58:
> ON_CALL(*mock_adapter_.get(), GetDevice(mock_device_->GetAddress()))
> On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
>>
>> Adding .get() shouldn't affect how this works, since it's getting
>
> dereferenced
>>
>> either way.
>
>
> Before anything else, I'd like to say that I really hate macros.
>
> I think this is actually necessary due to the way the gtest macro is
> constructed:
> #define GMOCK_ON_CALL_IMPL_(obj, call) \
>     ((obj).gmock_##call).InternalDefaultActionSetAt(__FILE__, __LINE__,
> \
>                                                     #obj, #call)
> #define ON_CALL(obj, call) GMOCK_ON_CALL_IMPL_(obj, call)
>
> (Un?)fortunately, scoped_refptr does not currently implement operator*,
> so I think it's necessary to explicitly call .get() to unwrap the
> scoped_refptr so we can pass a T& to gmock.
>
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/api/permissions/permissions_api.cc
> (right):
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/permissions/permissions_api.cc:178:
> ->Contains(*requested_permissions_.get())) {
> On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
>>
>> You shouldn't need to add a .get() to something that's already
>
> dereferencing the
>>
>> scoped_refptr.
>
>
> scoped_refptr doesn't implement operator*, for better or worse.
>
> https://codereview.chromium.org/501263002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-08-26 02:09:14]

On 2014/08/26 at 01:34:50, jyasskin wrote:
> Ah, yes. Anything that implements operator-> or pretends to be a
> pointer should implement operator*.

Is it OK if I simplify things for myself and commit this patch as is? I'll make
sure future patches don't have this issue, but I have quite a few CLs in flight
already =)

> 
> On Mon, Aug 25, 2014 at 6:26 PM,  <dcheng@chromium.org> wrote:
> > Do you think it'd make sense to add operator* to scoped_refptr? This would
> > make
> > it behave more like scoped_ptr, etc.
> >
> >
> >
> >
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> > File
> > chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc
> > (right):
> >
> >
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> > chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc:58:
> > ON_CALL(*mock_adapter_.get(), GetDevice(mock_device_->GetAddress()))
> > On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
> >>
> >> Adding .get() shouldn't affect how this works, since it's getting
> >
> > dereferenced
> >>
> >> either way.
> >
> >
> > Before anything else, I'd like to say that I really hate macros.
> >
> > I think this is actually necessary due to the way the gtest macro is
> > constructed:
> > #define GMOCK_ON_CALL_IMPL_(obj, call) \
> >     ((obj).gmock_##call).InternalDefaultActionSetAt(__FILE__, __LINE__,
> > \
> >                                                     #obj, #call)
> > #define ON_CALL(obj, call) GMOCK_ON_CALL_IMPL_(obj, call)
> >
> > (Un?)fortunately, scoped_refptr does not currently implement operator*,
> > so I think it's necessary to explicitly call .get() to unwrap the
> > scoped_refptr so we can pass a T& to gmock.
> >
> >
> >
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> > File chrome/browser/extensions/api/permissions/permissions_api.cc
> > (right):
> >
> >
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
> > chrome/browser/extensions/api/permissions/permissions_api.cc:178:
> > ->Contains(*requested_permissions_.get())) {
> > On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
> >>
> >> You shouldn't need to add a .get() to something that's already
> >
> > dereferencing the
> >>
> >> scoped_refptr.
> >
> >
> > scoped_refptr doesn't implement operator*, for better or worse.
> >
> > https://codereview.chromium.org/501263002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[Jeffrey Yasskin, 2014-08-26 02:10:54]

On Mon, Aug 25, 2014 at 7:09 PM,  <dcheng@chromium.org> wrote:
> On 2014/08/26 at 01:34:50, jyasskin wrote:
>>
>> Ah, yes. Anything that implements operator-> or pretends to be a
>> pointer should implement operator*.
>
>
> Is it OK if I simplify things for myself and commit this patch as is? I'll
> make
> sure future patches don't have this issue, but I have quite a few CLs in
> flight
> already =)

LGTM :)

>> On Mon, Aug 25, 2014 at 6:26 PM,  <dcheng@chromium.org> wrote:
>> > Do you think it'd make sense to add operator* to scoped_refptr? This
>> > would
>> > make
>> > it behave more like scoped_ptr, etc.
>> >
>> >
>> >
>> >
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
>>
>> > File
>> > chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc
>> > (right):
>> >
>> >
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
>>
>> > chrome/browser/extensions/api/bluetooth/bluetooth_private_apitest.cc:58:
>> > ON_CALL(*mock_adapter_.get(), GetDevice(mock_device_->GetAddress()))
>> > On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
>> >>
>> >> Adding .get() shouldn't affect how this works, since it's getting
>> >
>> > dereferenced
>> >>
>> >> either way.
>> >
>> >
>> > Before anything else, I'd like to say that I really hate macros.
>> >
>> > I think this is actually necessary due to the way the gtest macro is
>> > constructed:
>> > #define GMOCK_ON_CALL_IMPL_(obj, call) \
>> >     ((obj).gmock_##call).InternalDefaultActionSetAt(__FILE__, __LINE__,
>> > \
>> >                                                     #obj, #call)
>> > #define ON_CALL(obj, call) GMOCK_ON_CALL_IMPL_(obj, call)
>> >
>> > (Un?)fortunately, scoped_refptr does not currently implement operator*,
>> > so I think it's necessary to explicitly call .get() to unwrap the
>> > scoped_refptr so we can pass a T& to gmock.
>> >
>> >
>> >
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
>>
>> > File chrome/browser/extensions/api/permissions/permissions_api.cc
>> > (right):
>> >
>> >
>
>
https://codereview.chromium.org/501263002/diff/20001/chrome/browser/extension...
>>
>> > chrome/browser/extensions/api/permissions/permissions_api.cc:178:
>> > ->Contains(*requested_permissions_.get())) {
>> > On 2014/08/26 01:14:16, Jeffrey Yasskin wrote:
>> >>
>> >> You shouldn't need to add a .get() to something that's already
>> >
>> > dereferencing the
>> >>
>> >> scoped_refptr.
>> >
>> >
>> > scoped_refptr doesn't implement operator*, for better or worse.
>> >
>> > https://codereview.chromium.org/501263002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email to chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/501263002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-08-26 02:11:38]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-26 02:12:50]

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

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

Committed patchset #2 (20001) as c92bf28b7bcc899a352f0eba8518623711c91a3c

[commit-bot: I haz the power, 2014-09-10 02:40:07]

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