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

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

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

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

[dcheng, 2014-08-26 18:27:49]

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

[dcheng, 2014-08-26 18:27:49]

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

[gab, 2014-08-26 19:29:56]

Some constructors highlighted below should be updated to take a scoped_refptr as
this is what they end up storing the pointer into.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager.cc:90: io_task_runner.get(),
JsonPrefStore's constructor should take a scoped_refptr instead as that's what
this pointer then gets stored into.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager.cc:135: io_task_runner.get(),
Same as above.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager.cc:142: io_task_runner.get(),
Same as above.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager.cc:223: io_task_runner.get(),
Same as above.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:94:
registry_verifier_(profile_pref_registry_.get()),
RegistryVerifier() should take a scoped_refptr as this is what it gets stored as
in the constructor.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:155:
pref_service_factory.Create(profile_pref_registry_.get()));
PrefServiceFactory::Create() should take a scoped_refptr as this is what it
ultimately gets stored into.

https://codereview.chromium.org/506153002/diff/1/chrome/browser/prefs/profile...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:169:
pref_service_factory.Create(profile_pref_registry_.get()));
Same as above.

[dcheng, 2014-08-26 19:36:25]

Would you be OK landing this CL as is? I'd rather avoid doing the conversion in
this patch, in order to get one platform conversion free ASAP (Linux is almost
done). Doing it in this patch would also drag in a potentially large wide net of
dependencies (PrefServiceFactory is in base).

[gab, 2014-08-27 18:20:19]

On 2014/08/26 19:36:25, dcheng (OOO) wrote:
> Would you be OK landing this CL as is? I'd rather avoid doing the conversion
in
> this patch, in order to get one platform conversion free ASAP (Linux is almost
> done). Doing it in this patch would also drag in a potentially large wide net
of
> dependencies (PrefServiceFactory is in base).

Well it doesn't make much sense to have constructors take in raw pointers and
shove them into scoped_refptr and it makes even less sense if the caller has a
scoped_refptr and extracts the raw pointer from it...

Can your tool be augmented to find such cases and fix the constructors?

Thanks,
Gab

[dcheng, 2014-08-27 18:31:02]

On 2014/08/27 at 18:20:19, gab wrote:
> On 2014/08/26 19:36:25, dcheng (OOO) wrote:
> > Would you be OK landing this CL as is? I'd rather avoid doing the conversion
in
> > this patch, in order to get one platform conversion free ASAP (Linux is
almost
> > done). Doing it in this patch would also drag in a potentially large wide
net of
> > dependencies (PrefServiceFactory is in base).
> 
> Well it doesn't make much sense to have constructors take in raw pointers and
shove them into scoped_refptr and it makes even less sense if the caller has a
scoped_refptr and extracts the raw pointer from it...

Sure, but that's effectively what the code is already doing, just implicitly.

> 
> Can your tool be augmented to find such cases and fix the constructors?

It could, but not without a lot of modifications to the tool and the wrapper
that runs it across Chrome. Since it's based on AST matching, I'd have to
capture the fact that certain things are constructor calls and capture all
constructor definitions, and then match them up in a postprocessing step to
figure out what constructor arguments actually need to be rewritten. This would
be at least several days of effort, if not more. This is out of the scope of the
current effort.

> 
> Thanks,
> Gab

As mentioned, I'm more than happy to fix it up in a followup patch due to the
fact that I would need to loop in multiple owners on the review. My primary goal
is to disable the conversion operator project-wide as quickly as possible.

[gab, 2014-08-27 18:33:58]

Okay, lgtm w/ follow-up patch to fix constructors.

[dcheng, 2014-08-27 18:35:53]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-27 18:36:53]

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

[commit-bot: I haz the power, 2014-08-27 23:25:55]

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

[commit-bot: I haz the power, 2014-08-27 23:25:56]

Failed to commit the patch.

[dcheng, 2014-08-28 00:54:37]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 00:55:40]

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

[commit-bot: I haz the power, 2014-08-28 00:56:53]

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

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

Failed to commit the patch.

[dcheng, 2014-08-28 00:58:31]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 01:00:02]

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

[commit-bot: I haz the power, 2014-08-28 01:02:20]

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

[commit-bot: I haz the power, 2014-08-28 01:02:22]

Failed to commit the patch.

[dcheng, 2014-08-28 01:07:35]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 01:09:15]

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

[commit-bot: I haz the power, 2014-08-28 01:10:48]

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

[commit-bot: I haz the power, 2014-08-28 01:10:52]

Failed to commit the patch.

[dcheng, 2014-08-28 01:15:25]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 01:15:58]

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

[commit-bot: I haz the power, 2014-08-28 01:17:13]

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

[commit-bot: I haz the power, 2014-08-28 01:17:15]

Failed to commit the patch.

[dcheng, 2014-08-28 01:22:19]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 01:23:23]

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

[commit-bot: I haz the power, 2014-08-28 01:24:47]

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

[commit-bot: I haz the power, 2014-08-28 01:24:48]

Failed to commit the patch.

[dcheng, 2014-08-28 01:31:04]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 01:31:48]

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

[commit-bot: I haz the power, 2014-08-28 01:33:12]

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

[commit-bot: I haz the power, 2014-08-28 01:33:13]

Failed to commit the patch.

[dcheng, 2014-08-28 02:23:15]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-28 02:24:15]

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

[commit-bot: I haz the power, 2014-08-28 02:25:26]

Committed patchset #1 (id:1) as 67f28f26ae64ae3ed854eb7f5b660f36041ba5b5

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

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