Remove use of deprecated base::ListValue::Append(Value*) overload in extensions.

Remove use of deprecated base::ListValue::Append(Value*) overload in extensions.

BUG=581865
Committed: https://crrev.com/f19502008401dc093629f3ca082f82edbc47e475
Cr-Commit-Position: refs/heads/master@{#418568}

[dcheng, 2016-09-10 06:14:27]

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

[commit-bot: I haz the power, 2016-09-10 06:14:37]

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-09-10 06:47:14]

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

[commit-bot: I haz the power, 2016-09-10 06:47:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2016-09-12 18:06:43]

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

[commit-bot: I haz the power, 2016-09-12 18:07:34]

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

[dcheng, 2016-09-12 18:08:54]

dcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[dcheng, 2016-09-12 18:08:55]

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:309:
args->Append(object_args->CreateDeepCopy());
This method has slightly more substantial changes. The original code abused the
fact that it could reach back into object_args and mutate it by removing the
first argument and then twiddling object_args to add the other needed key.

While I could have done the same thing with base::WrapUnique(), I think it's
better to be explicit about ownership here, even if it's at the cost of one
additional copy now, since there's a lot of moving in here.

[commit-bot: I haz the power, 2016-09-12 18:47:07]

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

[commit-bot: I haz the power, 2016-09-12 18:47:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-09-12 19:12:34]

A few places will break, since they were doing similar "pass ownership, then
modify" tricks.

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/commands.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/commands.cc:18:
std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
I'm assuming that future MakeUnique calls are orthogonal to this CL?

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/debugger/debugger_api.cc:709: return
std::move(dictionary);
isn't this a pessimising move?

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:199:
parameters->SetBoolean(key, false);
badness :(

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:309:
args->Append(object_args->CreateDeepCopy());
On 2016/09/12 18:08:54, dcheng wrote:
> This method has slightly more substantial changes. The original code abused
the
> fact that it could reach back into object_args and mutate it by removing the
> first argument and then twiddling object_args to add the other needed key.
> 
> While I could have done the same thing with base::WrapUnique(), I think it's
> better to be explicit about ownership here, even if it's at the cost of one
> additional copy now, since there's a lot of moving in here.

Yeah, this is more sane, and the copy isn't expensive.

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.cc:689: properties->SetInteger("frameId",
frame_id);
break

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.cc:700:
properties->SetBoolean("wasChecked", was_checked);
break

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.cc:708: properties->SetBoolean("checked",
item->checked());
break

[dcheng, 2016-09-12 20:26:20]

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

[dcheng, 2016-09-12 20:26:46]

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/commands.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/commands.cc:18:
std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
On 2016/09/12 19:12:34, Devlin wrote:
> I'm assuming that future MakeUnique calls are orthogonal to this CL?

This CL has been in progress for awhile: previously, we didn't have the clang
plugin check for using auto* for pointer types (though it's still not enabled by
default, we're getting closer... I hope) and writing std::unique_ptr<T> x =
base::MakeUnique<T>(...) is strictly longer. Now that we're close, I feel
relatively comfortable using the auto x = base::MakeUnique<T> pattern. If you
feel strongly, I can try to be uniform about it in this CL.

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/debugger/debugger_api.cc:709: return
std::move(dictionary);
On 2016/09/12 19:12:34, Devlin wrote:
> isn't this a pessimising move?

std::move() is required when the return type is different. In this case, it's
easy enough to just make them match, so I've done so.

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.cc:689: properties->SetInteger("frameId",
frame_id);
On 2016/09/12 19:12:34, Devlin wrote:
> break

I fixed the test explosions from this... with a raw pointer =S

[commit-bot: I haz the power, 2016-09-12 20:27:16]

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

[Devlin, 2016-09-12 21:01:30]

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:199:
parameters->SetBoolean(key, false);
On 2016/09/12 19:12:34, Devlin wrote:
> badness :(

This one wasn't addressed?

[commit-bot: I haz the power, 2016-09-12 21:06:10]

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

[commit-bot: I haz the power, 2016-09-12 21:06:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2016-09-13 20:53:51]

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

[commit-bot: I haz the power, 2016-09-13 20:54:18]

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

[dcheng, 2016-09-13 20:54:20]

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:199:
parameters->SetBoolean(key, false);
On 2016/09/12 21:01:30, Devlin wrote:
> On 2016/09/12 19:12:34, Devlin wrote:
> > badness :(
> 
> This one wasn't addressed?

Yeah, I missed this failure in the test runs. Fixed.

[commit-bot: I haz the power, 2016-09-13 21:46:51]

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

[commit-bot: I haz the power, 2016-09-13 21:46:52]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-14 14:56:53]

lgtm

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/2323993004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/debugger/debugger_api.cc:709: return
std::move(dictionary);
On 2016/09/12 20:26:45, dcheng wrote:
> On 2016/09/12 19:12:34, Devlin wrote:
> > isn't this a pessimising move?
> 
> std::move() is required when the return type is different. In this case, it's
> easy enough to just make them match, so I've done so.

Ah, interesting.  I was wondering why there wasn't a compiler warning.

[dcheng, 2016-09-14 15:12:48]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-09-14 15:13:09]

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-09-14 15:18:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-14 15:20:00]

Description was changed from

==========
Remove use of deprecated base::ListValue::Append(Value*) overload in extensions.

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

to

==========
Remove use of deprecated base::ListValue::Append(Value*) overload in extensions.

BUG=581865

Committed: https://crrev.com/f19502008401dc093629f3ca082f82edbc47e475
Cr-Commit-Position: refs/heads/master@{#418568}
==========

[commit-bot: I haz the power, 2016-09-14 15:20:03]

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