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

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

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

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

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

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

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

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/507483003/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_launcher.cc (right):

https://codereview.chromium.org/507483003/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_launcher.cc:407: const
scoped_refptr<Extension> extension = GetLocalizedExtensionForDisplay();
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
GetLocalizedExtensionForDisplay() 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.)

[benwells, 2014-08-26 05:22:22]

https://codereview.chromium.org/507483003/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_launcher.cc (right):

https://codereview.chromium.org/507483003/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_launcher.cc:407: const
scoped_refptr<Extension> extension = GetLocalizedExtensionForDisplay();
On 2014/08/26 00:18:25, 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
> GetLocalizedExtensionForDisplay() 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.)

From looking at the impl, it looks like this is right - but unusual for
extensions code which normally uses bare pointers.

https://codereview.chromium.org/507483003/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_launcher.cc:408: if (!extension) {
Does this function need changes so it isn't doing implicit conversions?

Edit: actually this function doesn't compile as is, see bot output.

[dcheng, 2014-08-26 05:28:06]

PTAL

https://codereview.chromium.org/507483003/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_launcher.cc (right):

https://codereview.chromium.org/507483003/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_launcher.cc:408: if (!extension) {
On 2014/08/26 05:22:21, benwells wrote:
> Does this function need changes so it isn't doing implicit conversions?
> 
> Edit: actually this function doesn't compile as is, see bot output.

Yeah, I was fixing it locally since I just noticed this a few minutes ago as
well. Done.

[benwells, 2014-08-26 05:37:28]

lgtm

[dcheng, 2014-08-26 05:39:16]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-26 05:40:56]

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

[commit-bot: I haz the power, 2014-08-26 06:38:55]

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

[commit-bot: I haz the power, 2014-08-26 07:59:29]

Committed patchset #3 (40001) as bc1d23d7e2890195701ea1e8e04adbb33fe45796

[commit-bot: I haz the power, 2014-09-10 02:41:21]

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