Correct threading documentation example.

Correct threading documentation example.

Calling a callback with a raw pointer argument while binding a scoped_refptr to
it requires using base::RetainedRef. Correct the same in threading
documentation. Also, introduce std::move in the example to demonstrate
preventing an extra AddRef/Release pair.

BUG=None

Review-Url: https://codereview.chromium.org/2944313004
Cr-Commit-Position: refs/heads/master@{#482065}
Committed: https://chromium.googlesource.com/chromium/src/+/09f8883243cb82866287f401457d10523d0f2d05

[karandeepb, 2017-06-22 00:59:08]

Description was changed from

==========
Correct threading documentation example.
==========

to

==========
Correct threading documentation example.

Calling a callback with a raw pointer argument while binding a scoped_refptr to
it requires using base::RetainedRef. Correct the same in threading
documentation. Also, introduce std::move in the example to demonstrate
preventing an extra AddRef/Release pair.

BUG=None
==========

[karandeepb, 2017-06-22 01:04:38]

karandeepb@chromium.org changed reviewers:
+ lazyboy@chromium.org

[karandeepb, 2017-06-22 01:04:39]

Istiaque: Can you do a sanity check?

[lazyboy, 2017-06-23 20:43:27]

https://codereview.chromium.org/2944313004/diff/1/docs/design/threading.md
File docs/design/threading.md (right):

https://codereview.chromium.org/2944313004/diff/1/docs/design/threading.md#ne...
docs/design/threading.md:246: // Moving |param| prevents an extra
AddRef()/Release() pair.
Nice, I think you should separate out this std::move tip, possibly to
DoSomething3, to keep things simple, then gradually advancing...

The simple example here is demonstrating how you could bind a function that
takes raw T* from scoped_refptr. And your change to RetainedRef by itself is
great/enough for the simpler case. The older code doesn't even compile, right?

https://codereview.chromium.org/2944313004/diff/1/docs/design/threading.md#ne...
docs/design/threading.md:258: // Note how this takes a raw pointer. The
important part is that using
The comment here before was emphasizing that instead of using:
void DoSomethingOnAnotherThread(scoped_refptr<SomeParamObject> param),
i.e. using scoped_refptr<T> as argument would result in an additional
AddRef/Release vs a raw T* pointer argument. So I believe we should leave this
as is.

The mention of the requirement of RetainedRef should go to the callsites (i.e.
line 249 and 255), because regardless of saving AddRef/Release or not, it *is*
required to be able to use use T*

[karandeepb, 2017-06-23 22:45:36]

Patchset #2 (id:20001) has been deleted

[karandeepb, 2017-06-23 22:47:30]

Patchset #2 (id:40001) has been deleted

[karandeepb, 2017-06-23 22:48:46]

https://codereview.chromium.org/2944313004/diff/1/docs/design/threading.md
File docs/design/threading.md (right):

https://codereview.chromium.org/2944313004/diff/1/docs/design/threading.md#ne...
docs/design/threading.md:246: // Moving |param| prevents an extra
AddRef()/Release() pair.
On 2017/06/23 20:43:27, lazyboy wrote:
> Nice, I think you should separate out this std::move tip, possibly to
> DoSomething3, to keep things simple, then gradually advancing...
> 
> The simple example here is demonstrating how you could bind a function that
> takes raw T* from scoped_refptr. And your change to RetainedRef by itself is
> great/enough for the simpler case. The older code doesn't even compile, right?

Done. Yeah it doesn't compile.

https://codereview.chromium.org/2944313004/diff/1/docs/design/threading.md#ne...
docs/design/threading.md:258: // Note how this takes a raw pointer. The
important part is that using
On 2017/06/23 20:43:27, lazyboy wrote:
> The comment here before was emphasizing that instead of using:
> void DoSomethingOnAnotherThread(scoped_refptr<SomeParamObject> param),
> i.e. using scoped_refptr<T> as argument would result in an additional
> AddRef/Release vs a raw T* pointer argument. So I believe we should leave this
> as is.
> 
> The mention of the requirement of RetainedRef should go to the callsites (i.e.
> line 249 and 255), because regardless of saving AddRef/Release or not, it *is*
> required to be able to use use T*

Done.

[lazyboy, 2017-06-23 22:58:51]

lgtm

[karandeepb, 2017-06-23 23:00:38]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-06-23 23:01:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 23:15:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498258838435310,
"parent_rev": "22a693288bbead58a292a5cb6d00e7aa3de2aac4", "commit_rev":
"09f8883243cb82866287f401457d10523d0f2d05"}

[commit-bot: I haz the power, 2017-06-23 23:15:43]

Description was changed from

==========
Correct threading documentation example.

Calling a callback with a raw pointer argument while binding a scoped_refptr to
it requires using base::RetainedRef. Correct the same in threading
documentation. Also, introduce std::move in the example to demonstrate
preventing an extra AddRef/Release pair.

BUG=None
==========

to

==========
Correct threading documentation example.

Calling a callback with a raw pointer argument while binding a scoped_refptr to
it requires using base::RetainedRef. Correct the same in threading
documentation. Also, introduce std::move in the example to demonstrate
preventing an extra AddRef/Release pair.

BUG=None

Review-Url: https://codereview.chromium.org/2944313004
Cr-Commit-Position: refs/heads/master@{#482065}
Committed:
https://chromium.googlesource.com/chromium/src/+/09f8883243cb82866287f401457d...
==========

[commit-bot: I haz the power, 2017-06-23 23:15:45]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/09f8883243cb82866287f401457d...