Switch //components away from base::ListValue::Append(Value*) overload.

Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
R=danakj@chromium.org
TBR=blundell@chromium.org

Committed: https://crrev.com/32fd7c4ca1b445b9f0044bed9b8686aee59ceefe
Cr-Commit-Position: refs/heads/master@{#414932}

[dcheng, 2016-08-26 21:36:48]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 21:37:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-08-26 21:40:04]

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

[dcheng, 2016-08-26 21:40:04]

danakj@, mind doing an overall review? I'll TBR the actual OWNERS.

[dcheng, 2016-08-26 21:51:45]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 21:52:42]

Dry run: 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, 2016-08-26 22:10:58]

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

[commit-bot: I haz the power, 2016-08-26 22:10:59]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dcheng, 2016-08-26 22:52:48]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[dcheng, 2016-08-26 22:53:00]

Really fixed now!

[commit-bot: I haz the power, 2016-08-26 22:54:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-26 22:59:24]

LGTM

https://codereview.chromium.org/2287733002/diff/40001/components/cloud_device...
File components/cloud_devices/common/description_items_inl.h (right):

https://codereview.chromium.org/2287733002/diff/40001/components/cloud_device...
components/cloud_devices/common/description_items_inl.h:70:
std::unique_ptr<base::DictionaryValue> option_value(
If I had my way these would all be auto foo =
base::MakeUnique<base::DictionaryValue>() so we don't write the type so much

https://codereview.chromium.org/2287733002/diff/40001/components/domain_relia...
File components/domain_reliability/scheduler.cc (left):

https://codereview.chromium.org/2287733002/diff/40001/components/domain_relia...
components/domain_reliability/scheduler.cc:192: // Using release instead of Pass
because Pass can't implicitly upcast.
:D

https://codereview.chromium.org/2287733002/diff/40001/components/signin/core/...
File components/signin/core/browser/account_tracker_service_unittest.cc (left):

https://codereview.chromium.org/2287733002/diff/40001/components/signin/core/...
components/signin/core/browser/account_tracker_service_unittest.cc:961:
update->Append(dict);
this append got lost

[dcheng, 2016-08-26 23:03:38]

https://codereview.chromium.org/2287733002/diff/40001/components/cloud_device...
File components/cloud_devices/common/description_items_inl.h (right):

https://codereview.chromium.org/2287733002/diff/40001/components/cloud_device...
components/cloud_devices/common/description_items_inl.h:70:
std::unique_ptr<base::DictionaryValue> option_value(
On 2016/08/26 22:59:24, danakj wrote:
> If I had my way these would all be auto foo =
> base::MakeUnique<base::DictionaryValue>() so we don't write the type so much

Yeah, I'm getting carpal tunnel from all these changes so wanted to minimize the
scope a bit. I figure we can try this in a followup change (once vmpstr@ has the
auto* plugin check enabled).

https://codereview.chromium.org/2287733002/diff/40001/components/signin/core/...
File components/signin/core/browser/account_tracker_service_unittest.cc (left):

https://codereview.chromium.org/2287733002/diff/40001/components/signin/core/...
components/signin/core/browser/account_tracker_service_unittest.cc:961:
update->Append(dict);
On 2016/08/26 22:59:24, danakj wrote:
> this append got lost

Fixed this already in PS3 =)

[dcheng, 2016-08-26 23:04:15]

Description was changed from

==========
Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
==========

to

==========
Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
R=danakj@chromium.org
TBR=blundell@chromium.org
==========

[dcheng, 2016-08-26 23:04:15]

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

[dcheng, 2016-08-26 23:04:29]

TBRing blundell@ for OWNERS.

[commit-bot: I haz the power, 2016-08-27 00:24:42]

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

[commit-bot: I haz the power, 2016-08-27 00:24:42]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-08-27 01:04:41]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-08-27 01:05:30]

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, 2016-08-27 11:16:21]

Description was changed from

==========
Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
R=danakj@chromium.org
TBR=blundell@chromium.org
==========

to

==========
Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
R=danakj@chromium.org
TBR=blundell@chromium.org
==========

[commit-bot: I haz the power, 2016-08-27 11:16:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-27 11:17:53]

Description was changed from

==========
Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
R=danakj@chromium.org
TBR=blundell@chromium.org
==========

to

==========
Switch //components away from base::ListValue::Append(Value*) overload.

This overload is deprecated: prefer the std::unique_ptr<Value> version,
which helps the compiler enforce that ownership transfer occurs.

BUG=581865
R=danakj@chromium.org
TBR=blundell@chromium.org

Committed: https://crrev.com/32fd7c4ca1b445b9f0044bed9b8686aee59ceefe
Cr-Commit-Position: refs/heads/master@{#414932}
==========

[commit-bot: I haz the power, 2016-08-27 11:17:55]

Patchset 4 (id:??) landed as
https://crrev.com/32fd7c4ca1b445b9f0044bed9b8686aee59ceefe
Cr-Commit-Position: refs/heads/master@{#414932}

[blundell, 2016-08-29 07:39:15]

lgtm

https://codereview.chromium.org/2287733002/diff/60001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_codec.cc (right):

https://codereview.chromium.org/2287733002/diff/60001/components/bookmarks/br...
components/bookmarks/browser/bookmark_codec.cc:152: return std::move(value);
just to verify my own understanding of how this stuff works, this std::move call
isn't actually necessary because |value| is going out of scope here, right?

https://codereview.chromium.org/2287733002/diff/60001/components/cloud_device...
File components/cloud_devices/common/description_items_inl.h (right):

https://codereview.chromium.org/2287733002/diff/60001/components/cloud_device...
components/cloud_devices/common/description_items_inl.h:73:
options_list->Append(std::move(option_value));
similar question for my own understanding: is the std::move call here necessary?

https://codereview.chromium.org/2287733002/diff/60001/components/error_page/c...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2287733002/diff/60001/components/error_page/c...
components/error_page/common/localized_error.cc:513: list->Insert(0,
suggestion_list_item.release());
I assume that this is targeted for followup?

[vinodsonkusare77_gmail.com, 2016-08-30 06:33:39]

https://youtu.be/ZB29OFBS2w8?list=UUWQUZfAdokV83KKjLl-sDeQ

On Mon, Aug 29, 2016 at 1:09 PM, <blundell@chromium.org> wrote:

> lgtm
>
>
> https://codereview.chromium.org/2287733002/diff/60001/
> components/bookmarks/browser/bookmark_codec.cc
> File components/bookmarks/browser/bookmark_codec.cc (right):
>
> https://codereview.chromium.org/2287733002/diff/60001/
> components/bookmarks/browser/bookmark_codec.cc#newcode152
> components/bookmarks/browser/bookmark_codec.cc:152: return
> std::move(value);
> just to verify my own understanding of how this stuff works, this
> std::move call isn't actually necessary because |value| is going out of
> scope here, right?
>
> https://codereview.chromium.org/2287733002/diff/60001/
> components/cloud_devices/common/description_items_inl.h
> File components/cloud_devices/common/description_items_inl.h (right):
>
> https://codereview.chromium.org/2287733002/diff/60001/
> components/cloud_devices/common/description_items_inl.h#newcode73
> components/cloud_devices/common/description_items_inl.h:73:
> options_list->Append(std::move(option_value));
> similar question for my own understanding: is the std::move call here
> necessary?
>
> https://codereview.chromium.org/2287733002/diff/60001/
> components/error_page/common/localized_error.cc
> File components/error_page/common/localized_error.cc (right):
>
> https://codereview.chromium.org/2287733002/diff/60001/
> components/error_page/common/localized_error.cc#newcode513
> components/error_page/common/localized_error.cc:513: list->Insert(0,
> suggestion_list_item.release());
> I assume that this is targeted for followup?
>
> https://codereview.chromium.org/2287733002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "DevTools Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to devtools-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2016-08-30 17:41:55]

https://codereview.chromium.org/2287733002/diff/60001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_codec.cc (right):

https://codereview.chromium.org/2287733002/diff/60001/components/bookmarks/br...
components/bookmarks/browser/bookmark_codec.cc:152: return std::move(value);
On 2016/08/29 07:39:15, blundell wrote:
> just to verify my own understanding of how this stuff works, this std::move
call
> isn't actually necessary because |value| is going out of scope here, right?

It is, because the type of the local doesn't match the type of the return value.
In C++14, this is no longer required.

https://codereview.chromium.org/2287733002/diff/60001/components/cloud_device...
File components/cloud_devices/common/description_items_inl.h (right):

https://codereview.chromium.org/2287733002/diff/60001/components/cloud_device...
components/cloud_devices/common/description_items_inl.h:73:
options_list->Append(std::move(option_value));
On 2016/08/29 07:39:15, blundell wrote:
> similar question for my own understanding: is the std::move call here
necessary?

It is: one way to think of it is move constructors / assignment operators can
only be called on unnamed objects (in other words, temporaries). In that sense,
std::move() is a 'name eraser'. The internal C++ totws are a pretty good
explainer (see #55 and #77)

https://codereview.chromium.org/2287733002/diff/60001/components/error_page/c...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2287733002/diff/60001/components/error_page/c...
components/error_page/common/localized_error.cc:513: list->Insert(0,
suggestion_list_item.release());
On 2016/08/29 07:39:15, blundell wrote:
> I assume that this is targeted for followup?

Yes, there's no equivalent for Insert() atm.

[blundell, 2016-08-31 09:03:31]

Thanks for the explanations!