Remove raw base::DictionaryValue::Set in //extensions

Remove raw base::DictionaryValue::Set in //extensions

This change removes the deprecated raw pointer version of
base::DictionaryValue::Set in //extensions and replaces it with the unique
pointer version or other convenience functions where appropriate.

BUG=646113, 581865
Review-Url: https://codereview.chromium.org/2899743002
Cr-Commit-Position: refs/heads/master@{#477602}
Committed: https://chromium.googlesource.com/chromium/src/+/6ff270cabd2fe758de8cf57060dd660ae7d8ef68

[jdoerrie, 2017-05-22 13:40:24]

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

[commit-bot: I haz the power, 2017-05-22 13:40: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, 2017-05-22 13:57:47]

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

[commit-bot: I haz the power, 2017-05-22 13:57:48]

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

[jdoerrie, 2017-05-22 15:58:31]

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

[commit-bot: I haz the power, 2017-05-22 15:58:49]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-22 16:41:25]

Description was changed from

==========
Remove raw base::DictionaryValue::Set in //extensions

BUG=646113
==========

to

==========
Remove raw base::DictionaryValue::Set in //extensions

This change removes the deprecated raw pointer version of
base::DictionaryValue::Set in //extensions and replaces it with the unique
pointer version or other convenience functions where appropriate.

BUG=646113, 581865
==========

[commit-bot: I haz the power, 2017-05-22 18:20:14]

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

[commit-bot: I haz the power, 2017-05-22 18:20:15]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-05-30 09:57:27]

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

[commit-bot: I haz the power, 2017-05-30 09:57:39]

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, 2017-05-30 11:35:41]

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

[commit-bot: I haz the power, 2017-05-30 11:35:42]

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...)

[jdoerrie, 2017-06-02 13:19:33]

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

[commit-bot: I haz the power, 2017-06-02 13:19:53]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-06-02 13:28:09]

jdoerrie@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, vabr@chromium.org

[jdoerrie, 2017-06-02 13:28:10]

vabr@chromium.org: Please review the CL in detail.

rdevlin.cronin@chromium.org: Please spot-check and rubber-stamp.

[Devlin, 2017-06-02 15:38:49]

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1629: return
std::move(serialized_headers);
we shouldn't need std::move() here, I'd think?  Or does the compiler require it
since serialized_headers is a ListValue instead of a Value?

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:199: if
((extra_info_spec & ExtraInfoSpec::REQUEST_BODY) && request_body_)
since the bodies of these if's are now multi-line, wrap in {}

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:201:
base::MakeUnique<base::Value>(*request_body_));
Is the copy-constructor preferred over CreateDeepCopy()?  CreateDeepCopy()
already returns a std::unique_ptr<>, and doesn't have any comments in values.h
indicating it's undesirable...

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/comp...
File extensions/browser/computed_hashes.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/comp...
extensions/browser/computed_hashes.cc:140: auto block_hashes =
base::MakeUnique<base::ListValue>();
Since we're touching this code anyway, we should add a
block_hashes->Reserve(hashes.size());

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/comp...
extensions/browser/computed_hashes.cc:145:
block_hashes->GetList().emplace_back(std::move(encoded));
Is this preferred over Append()/AppendString()?  (I notice that AppendString
can't take an rvalue reference, but that could be easily remedied).

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_find_helper.cc:225:
base::MakeUnique<base::Value>(rect));
This was wrong before, but let's fix it :)
|rect| should be a std::unique_ptr<> so that we can just move it in, rather than
doing this copy.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/event...
File extensions/common/event_filter_unittest.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/event...
extensions/common/event_filter_unittest.cc:37: return
std::unique_ptr<base::Value>(dict.release());
return dict directly

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/event...
extensions/common/event_filter_unittest.cc:48: return
std::unique_ptr<EventMatcher>(new EventMatcher(
may as well base::MakeUnique<EventMatcher> while we're here.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/messa...
File extensions/common/message_bundle_unittest.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/messa...
extensions/common/message_bundle_unittest.cc:43: void SetDictionary(const
std::string& name,
This method seems a little unnecessary now.  It'd be cleaner and more concise to
just inline dict->Set(name, std::move(value)) everywhere.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/messa...
extensions/common/message_bundle_unittest.cc:99: }
broken before, but can we fix this brace?

[commit-bot: I haz the power, 2017-06-02 15:51:53]

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

[commit-bot: I haz the power, 2017-06-02 15:51:54]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-06-03 11:14:52]

Jan, thanks for the CL and code improvement! LGTM.

Devlin, thanks for the careful review. I tried to leave some clarifying answers
where my knowledge sufficed.

Cheers,
Vaclav

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1629: return
std::move(serialized_headers);
On 2017/06/02 15:38:48, Devlin wrote:
> we shouldn't need std::move() here, I'd think?  Or does the compiler require
it
> since serialized_headers is a ListValue instead of a Value?

Until C++14 we need move because of the upcast. See
https://groups.google.com/a/chromium.org/d/msg/cxx/gfV_Srujt6A/OvJ1bo4hBgAJ.

Actually, in this particular case, the need for move will disappear once
ListValue gets merged into Value (which is what jdoerrie@ is actively working
on).

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:201:
base::MakeUnique<base::Value>(*request_body_));
On 2017/06/02 15:38:48, Devlin wrote:
> Is the copy-constructor preferred over CreateDeepCopy()?  CreateDeepCopy()
> already returns a std::unique_ptr<>, and doesn't have any comments in values.h
> indicating it's undesirable...

Indeed, CreateDeepCopy is deprecated in favour of the copy constructor (see
https://codereview.chromium.org/2781983003/). The reason is that Value is being
migrated out of heap-pointers to plain values, and once there, CreateDeepCopy
will be identical with copy constructor and hence unneeded.

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_find_helper.cc:225:
base::MakeUnique<base::Value>(rect));
On 2017/06/02 15:38:48, Devlin wrote:
> This was wrong before, but let's fix it :)
> |rect| should be a std::unique_ptr<> so that we can just move it in, rather
than
> doing this copy.

My suggestion is to keep |rect| just a DictionaryValue and move it. base::Value
has a move constructor, so this should still be efficient, and the code will be
ready for the future form of the base::Value API, which is to be used without
heap allocation if possible.

[Devlin, 2017-06-05 13:27:33]

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:201:
base::MakeUnique<base::Value>(*request_body_));
On 2017/06/03 11:14:52, vabr (hobby-only until 5 June) wrote:
> On 2017/06/02 15:38:48, Devlin wrote:
> > Is the copy-constructor preferred over CreateDeepCopy()?  CreateDeepCopy()
> > already returns a std::unique_ptr<>, and doesn't have any comments in
values.h
> > indicating it's undesirable...
> 
> Indeed, CreateDeepCopy is deprecated in favour of the copy constructor (see
> https://codereview.chromium.org/2781983003/). The reason is that Value is
being
> migrated out of heap-pointers to plain values, and once there, CreateDeepCopy
> will be identical with copy constructor and hence unneeded.

SG.  Can we add a comment to DictionaryValue::CreateDeepCopy()?  (Okay to do in
a separate CL)

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_find_helper.cc:225:
base::MakeUnique<base::Value>(rect));
On 2017/06/03 11:14:52, vabr (hobby-only until 5 June) wrote:
> On 2017/06/02 15:38:48, Devlin wrote:
> > This was wrong before, but let's fix it :)
> > |rect| should be a std::unique_ptr<> so that we can just move it in, rather
> than
> > doing this copy.
> 
> My suggestion is to keep |rect| just a DictionaryValue and move it.
base::Value
> has a move constructor, so this should still be efficient, and the code will
be
> ready for the future form of the base::Value API, which is to be used without
> heap allocation if possible.

That's fine, too - either way, we avoid the copy, which is what my concern is.

[jdoerrie, 2017-06-06 12:23:13]

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

[commit-bot: I haz the power, 2017-06-06 12:23:20]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-06-06 12:40:23]

Thanks for the review so far!

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:199: if
((extra_info_spec & ExtraInfoSpec::REQUEST_BODY) && request_body_)
On 2017/06/02 15:38:48, Devlin wrote:
> since the bodies of these if's are now multi-line, wrap in {}

Done.

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:201:
base::MakeUnique<base::Value>(*request_body_));
On 2017/06/05 13:27:33, Devlin wrote:
> On 2017/06/03 11:14:52, vabr (hobby-only until 5 June) wrote:
> > On 2017/06/02 15:38:48, Devlin wrote:
> > > Is the copy-constructor preferred over CreateDeepCopy()?  CreateDeepCopy()
> > > already returns a std::unique_ptr<>, and doesn't have any comments in
> values.h
> > > indicating it's undesirable...
> > 
> > Indeed, CreateDeepCopy is deprecated in favour of the copy constructor (see
> > https://codereview.chromium.org/2781983003/). The reason is that Value is
> being
> > migrated out of heap-pointers to plain values, and once there,
CreateDeepCopy
> > will be identical with copy constructor and hence unneeded.
> 
> SG.  Can we add a comment to DictionaryValue::CreateDeepCopy()?  (Okay to do
in
> a separate CL)

Done: http://crrev.com/c/525792

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/comp...
File extensions/browser/computed_hashes.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/comp...
extensions/browser/computed_hashes.cc:140: auto block_hashes =
base::MakeUnique<base::ListValue>();
On 2017/06/02 15:38:48, Devlin wrote:
> Since we're touching this code anyway, we should add a
> block_hashes->Reserve(hashes.size());

Done.

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/comp...
extensions/browser/computed_hashes.cc:145:
block_hashes->GetList().emplace_back(std::move(encoded));
On 2017/06/02 15:38:48, Devlin wrote:
> Is this preferred over Append()/AppendString()?  (I notice that AppendString
> can't take an rvalue reference, but that could be easily remedied).

Yes, Append* won't be part of the new API. To make this more explicit I also
added deprecation comments for this in http://crrev.com/c/525792.

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_find_helper.cc:225:
base::MakeUnique<base::Value>(rect));
On 2017/06/05 13:27:33, Devlin wrote:
> On 2017/06/03 11:14:52, vabr (hobby-only until 5 June) wrote:
> > On 2017/06/02 15:38:48, Devlin wrote:
> > > This was wrong before, but let's fix it :)
> > > |rect| should be a std::unique_ptr<> so that we can just move it in,
rather
> > than
> > > doing this copy.
> > 
> > My suggestion is to keep |rect| just a DictionaryValue and move it.
> base::Value
> > has a move constructor, so this should still be efficient, and the code will
> be
> > ready for the future form of the base::Value API, which is to be used
without
> > heap allocation if possible.
> 
> That's fine, too - either way, we avoid the copy, which is what my concern is.

I added a call to std::move to avoid the copy here. DictionaryValues currently
still store unique_ptrs under the hood, which is why the heap allocation can't
be avoided yet.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/event...
File extensions/common/event_filter_unittest.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/event...
extensions/common/event_filter_unittest.cc:37: return
std::unique_ptr<base::Value>(dict.release());
On 2017/06/02 15:38:48, Devlin wrote:
> return dict directly

Done.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/event...
extensions/common/event_filter_unittest.cc:48: return
std::unique_ptr<EventMatcher>(new EventMatcher(
On 2017/06/02 15:38:48, Devlin wrote:
> may as well base::MakeUnique<EventMatcher> while we're here.

Done.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/messa...
File extensions/common/message_bundle_unittest.cc (right):

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/messa...
extensions/common/message_bundle_unittest.cc:43: void SetDictionary(const
std::string& name,
On 2017/06/02 15:38:49, Devlin wrote:
> This method seems a little unnecessary now.  It'd be cleaner and more concise
to
> just inline dict->Set(name, std::move(value)) everywhere.

Done.

https://codereview.chromium.org/2899743002/diff/50001/extensions/common/messa...
extensions/common/message_bundle_unittest.cc:99: }
On 2017/06/02 15:38:48, Devlin wrote:
> broken before, but can we fix this brace?

Done, inlined empty_tree so braces are not necessary anymore.

[Devlin, 2017-06-06 14:07:47]

lgtm!

https://codereview.chromium.org/2899743002/diff/70001/extensions/common/exten...
File extensions/common/extension_l10n_util_unittest.cc (right):

https://codereview.chromium.org/2899743002/diff/70001/extensions/common/exten...
extensions/common/extension_l10n_util_unittest.cc:436: base::ListValue* handlers
= manifest.SetList(
nit: Can we restructure this to construct handlers first and then move it in to
avoid needing to use SetList()?

auto handler = base::MakeUnique<base::DictionaryValue>();
handler->SetString(keys::kPageActionDefaultTitle,
                   "__MSG_file_handler_title__");
auto handlers = base::MakeUnique<base::ListValue>();
handlers->GetList().push_back(std::move(handler));
manifest.Set(keys::kFileBrowserHandlers, std::move(handlers));

or similar?

Note: This means you'll have to re-get the handler dict below for the assertion
on line 457, but I think we should do that anyway, since the raw ptr we use now
is potentially unsafe if the list resizes at any point.

[commit-bot: I haz the power, 2017-06-06 14:11:16]

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

[commit-bot: I haz the power, 2017-06-06 14:11:17]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-06-06 14:47:34]

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

[commit-bot: I haz the power, 2017-06-06 14:47:54]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-06-06 14:48:32]

https://codereview.chromium.org/2899743002/diff/70001/extensions/common/exten...
File extensions/common/extension_l10n_util_unittest.cc (right):

https://codereview.chromium.org/2899743002/diff/70001/extensions/common/exten...
extensions/common/extension_l10n_util_unittest.cc:436: base::ListValue* handlers
= manifest.SetList(
On 2017/06/06 14:07:47, Devlin wrote:
> nit: Can we restructure this to construct handlers first and then move it in
to
> avoid needing to use SetList()?
> 
> auto handler = base::MakeUnique<base::DictionaryValue>();
> handler->SetString(keys::kPageActionDefaultTitle,
>                    "__MSG_file_handler_title__");
> auto handlers = base::MakeUnique<base::ListValue>();
> handlers->GetList().push_back(std::move(handler));
> manifest.Set(keys::kFileBrowserHandlers, std::move(handlers));
> 
> or similar?
> 
> Note: This means you'll have to re-get the handler dict below for the
assertion
> on line 457, but I think we should do that anyway, since the raw ptr we use
now
> is potentially unsafe if the list resizes at any point.

Done, re-getting the handlers is unfortunately still a bit awkward, given the
current lack of appropriate getters for DictionaryValues. This will be addressed
soon, however.

[commit-bot: I haz the power, 2017-06-06 16:37:21]

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

[commit-bot: I haz the power, 2017-06-06 16:37:22]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-06-07 10:25:26]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-06-07 10:25:26]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2899743002/#ps90001
(title: "Addressed nit")

[commit-bot: I haz the power, 2017-06-07 10:25:43]

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-07 10:31:48]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1496831126069020,
"parent_rev": "4738339a048ab887b2bc229a025c2369886632b3", "commit_rev":
"6ff270cabd2fe758de8cf57060dd660ae7d8ef68"}

[commit-bot: I haz the power, 2017-06-07 10:32:07]

Description was changed from

==========
Remove raw base::DictionaryValue::Set in //extensions

This change removes the deprecated raw pointer version of
base::DictionaryValue::Set in //extensions and replaces it with the unique
pointer version or other convenience functions where appropriate.

BUG=646113, 581865
==========

to

==========
Remove raw base::DictionaryValue::Set in //extensions

This change removes the deprecated raw pointer version of
base::DictionaryValue::Set in //extensions and replaces it with the unique
pointer version or other convenience functions where appropriate.

BUG=646113, 581865

Review-Url: https://codereview.chromium.org/2899743002
Cr-Commit-Position: refs/heads/master@{#477602}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ff270cabd2fe758de8cf57060dd...
==========

[commit-bot: I haz the power, 2017-06-07 10:32:08]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/6ff270cabd2fe758de8cf57060dd...