Manual fixups in drive code for scoped_refptr operator T* removal.

Manual fixups in drive code for scoped_refptr operator T* removal.

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

[dcheng, 2014-08-28 17:49:10]

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

[dcheng, 2014-08-28 17:49:10]

Manual fixes for cases the automated conversion tool skipped.

Since these arguments were being turned from a scoped_refptr to a T*, back to a
scoped_refptr, I've opted to just pass them as a const scoped_refptr<T>&, as a
signal to the caller that the arguments will be ref'ed.

[hashimoto, 2014-08-29 04:15:06]

lgtm

qq: Is it our policy to generally prefer "const scoped_refptr<T>&" to "T*" as
argument type and return type?

[dcheng, 2014-08-29 06:26:41]

On 2014/08/29 at 04:15:06, hashimoto wrote:
> lgtm
> 
> qq: Is it our policy to generally prefer "const scoped_refptr<T>&" to "T*" as
argument type and return type?

I would say it depends on the context. I would only use it as an argument type
if the function is going to take a ref to it somehow. As a return type, I would
probably only use it for accessors that return a scoped_refptr directly. It
would be nice to use it in more places to indicate that the caller should
probably ref something (like Blink's PassRefPtr) but unfortunately you cannot
return a const scoped_refptr<T>& to a temporary--this is one of the limitations
of not having an explicit ref pointer type for passing like Blink.

[hashimoto, 2014-08-29 08:12:46]

lgtm++, thanks
On 2014/08/29 06:26:41, dcheng (OOO) wrote:
> On 2014/08/29 at 04:15:06, hashimoto wrote:
> > lgtm
> > 
> > qq: Is it our policy to generally prefer "const scoped_refptr<T>&" to "T*"
as
> argument type and return type?
> 
> I would say it depends on the context. I would only use it as an argument type
> if the function is going to take a ref to it somehow. As a return type, I
would
Then it seems, ideally, our policy is to use scoped_refptr's ctor which takes T*
only when creating a new instance of T
and use copy ctor or operator= in other cases.
> probably only use it for accessors that return a scoped_refptr directly. It
> would be nice to use it in more places to indicate that the caller should
> probably ref something (like Blink's PassRefPtr) but unfortunately you cannot
> return a const scoped_refptr<T>& to a temporary--this is one of the
limitations
> of not having an explicit ref pointer type for passing like Blink.

[dcheng, 2014-08-29 08:14:43]

The CQ bit was checked by dcheng@chromium.org

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

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

[dcheng, 2014-08-29 08:27:07]

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

[dcheng, 2014-08-29 08:27:07]

On 2014/08/29 at 08:12:46, hashimoto wrote:
> lgtm++, thanks
> On 2014/08/29 06:26:41, dcheng (OOO) wrote:
> > On 2014/08/29 at 04:15:06, hashimoto wrote:
> > > lgtm
> > > 
> > > qq: Is it our policy to generally prefer "const scoped_refptr<T>&" to "T*"
as
> > argument type and return type?
> > 
> > I would say it depends on the context. I would only use it as an argument
type
> > if the function is going to take a ref to it somehow. As a return type, I
would
> Then it seems, ideally, our policy is to use scoped_refptr's ctor which takes
T* only when creating a new instance of T
> and use copy ctor or operator= in other cases.
> > probably only use it for accessors that return a scoped_refptr directly. It
> > would be nice to use it in more places to indicate that the caller should
> > probably ref something (like Blink's PassRefPtr) but unfortunately you
cannot
> > return a const scoped_refptr<T>& to a temporary--this is one of the
limitations
> > of not having an explicit ref pointer type for passing like Blink.

+rsleevi because I know he had some thoughts on this too.

I will talk to base owners about maybe adding some examples to
base/ref_counted.h. Here are some examples of what I think are ideal:

// Some examples where it makes sense to pass by const ref.
class A {
 public:
  // Pass as const ref since the constructor takes a ref to it.
  explicit A(const scoped_refptr<TaskRunner>& runner) : runner_(runner) {}

  // Pass as const ref since the setter refs it.
  void set_task_runner(const scoped_refptr<TaskRunner>& runner) { runner_ =
runner; }

  // Return as const ref. Forces callers to recognize that this thing is
refcounted,
  // but does not cause refcount churn if they don't need to take a reference
since no
  // scoped_refptr is constructed if they just dereference it with get().
  const scoped_refptr<TaskRunner>& runner() const { return runner_; }

 private:
  scoped_refptr<TaskRunner> runner_;
};

// Some examples where it doesn't make sense to pass by const ref.
class B {
 public:
  // Constructor doesn't ref |e|--it just saves some information from it.
  explicit B(const Extension* e) : id_(e->id()) {}

  // Can't return by const ref because can't return a reference to the temporary
  // scoped_refptr wrapping NULL.
  scoped_refptr<Extension> GetActiveExtension() const {
    return !id_.empty() ? GetExtensionById(id_) : NULL;
  }

 private:
  std::string id_;
};

[Ryan Sleevi, 2014-08-29 08:41:22]

On 2014/08/29 08:27:07, dcheng (OOO) wrote:
> On 2014/08/29 at 08:12:46, hashimoto wrote:
> > lgtm++, thanks
> > On 2014/08/29 06:26:41, dcheng (OOO) wrote:
> > > On 2014/08/29 at 04:15:06, hashimoto wrote:
> > > > lgtm
> > > > 
> > > > qq: Is it our policy to generally prefer "const scoped_refptr<T>&" to
"T*"
> as
> > > argument type and return type?
> > > 
> > > I would say it depends on the context. I would only use it as an argument
> type
> > > if the function is going to take a ref to it somehow.

I would add that I do feel strongly it should be viral (which matches C++11 and
other systems)

Namely, if you have

void TakesAFoo(Foo*) {
  // take a ref
}

void CallSomethingThatTakesAFoo(Foo* foo) {
  TakesAFoo(foo);
}

void MoreStuffInAChain(Foo* foo) {
  CallSomethingThatTakesAFoo(foo);
}

void AReallyLongChain(Foo* foo) {
  MoreStuffInAChain(foo);
}

Then each one of those method sigs would become const scoped_refptr<Foo>&



> > > As a return type, I
> would
> > Then it seems, ideally, our policy is to use scoped_refptr's ctor which
takes
> T* only when creating a new instance of T
> > and use copy ctor or operator= in other cases.
> > > probably only use it for accessors that return a scoped_refptr directly.
It
> > > would be nice to use it in more places to indicate that the caller should
> > > probably ref something (like Blink's PassRefPtr) but unfortunately you
> cannot
> > > return a const scoped_refptr<T>& to a temporary--this is one of the
> limitations
> > > of not having an explicit ref pointer type for passing like Blink.
> 
> +rsleevi because I know he had some thoughts on this too.
> 
> I will talk to base owners about maybe adding some examples to
> base/ref_counted.h. Here are some examples of what I think are ideal:
> 
> // Some examples where it makes sense to pass by const ref.
> class A {
>  public:
>   // Pass as const ref since the constructor takes a ref to it.
>   explicit A(const scoped_refptr<TaskRunner>& runner) : runner_(runner) {}
> 
>   // Pass as const ref since the setter refs it.
>   void set_task_runner(const scoped_refptr<TaskRunner>& runner) { runner_ =
> runner; }
> 
>   // Return as const ref. Forces callers to recognize that this thing is
> refcounted,
>   // but does not cause refcount churn if they don't need to take a reference
> since no
>   // scoped_refptr is constructed if they just dereference it with get().
>   const scoped_refptr<TaskRunner>& runner() const { return runner_; }
> 
>  private:
>   scoped_refptr<TaskRunner> runner_;
> };
> 
> // Some examples where it doesn't make sense to pass by const ref.
> class B {
>  public:
>   // Constructor doesn't ref |e|--it just saves some information from it.
>   explicit B(const Extension* e) : id_(e->id()) {}

Two caveats here:
1) Sometimes you should just be passing int extension_id rather than "const
Extension*"
2) The above comment re: viralness means you shouldn't just pass it to something
else that 'takes a ref'

That is, if your 'lower level' code is const scoped_refptr<>, your 'higher
level' code that calls that is.
If the lower level code is just T*, then the higher level code can just be T*

But, as dcheng notes, it's hard to enforce this without Blink-style explicit
handling for PassRefPtr<> and friends.

> 
>   // Can't return by const ref because can't return a reference to the
temporary
>   // scoped_refptr wrapping NULL.
>   scoped_refptr<Extension> GetActiveExtension() const {
>     return !id_.empty() ? GetExtensionById(id_) : NULL;
>   }
> 
>  private:
>   std::string id_;
> };

[commit-bot: I haz the power, 2014-08-29 13:34:18]

Committed patchset #3 (id:40001) as a96ef4d03a7dc6cae38be46772fe563d7c8f0de4

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

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