Make ValueDeserializer::Deserialize return scoped_ptr

Make ValueDeserializer::Deserialize return scoped_ptr

Make ValueDeserializer::Deserialize return scoped_ptr
as almost all consumers already use scoped_ptr and it
is also better way to do it.

TBR=jam@chromium.org

Committed: https://crrev.com/ba04525cccccae530aa899882227d81b037eebff
Cr-Commit-Position: refs/heads/master@{#354458}

[Olli Raula, 2015-10-12 12:20:18]

olli.raula@intel.com changed reviewers:
+ thestig@chromium.org

[Olli Raula, 2015-10-12 12:25:12]

Could you review base/* and optionally chrome/*. I will add more reviewers after
your comments.

[Lei Zhang, 2015-10-12 22:42:21]

I only reviewed base/ and chrome/ and came up with the following comments. I can
review the rest in the next patch set and we can probably TBR.

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/kiosk_external_updater.cc (right):

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:56: *error_code =
KioskExternalUpdater::ERROR_INVALID_MANIFEST;
Sorry to bug you about these possibly existing issues. Feel free to punt / file
bugs. Basically any place where we release(), I get suspicious. :)

I think we need to free |extensions| here?

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:60:
parsed_manifest->Swap(dict_value);
After we swap, do we need to free the pointer that got swapped in?

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/default_app_order.cc (right):

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/default_app_order.cc:79: LOG(WARNING) <<
"Expect a JSON list in file " << path.value();
Is |value| leaked here?

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/component_installers_unittest.cc (right):

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/component_installers_unittest.cc:78:
deserializer.Deserialize(NULL, &error).release()));
Can we provide some easier way to convert the return value to a
scoped_ptr<base::DictionaryValue>?

We also need to do the same in:
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc
chrome/browser/ui/cocoa/extensions/extension_install_prompt_test_utils.mm
chrome/browser/themes/browser_theme_pack_unittest.cc
chrome/browser/extensions/user_script_listener_unittest.cc
chrome/browser/extensions/extension_icon_manager_unittest.cc
chrome/browser/extensions/extension_action_icon_factory_unittest.cc

and it may also make the code prettier in:
chrome/browser/extensions/external_pref_loader.cc
chrome/browser/extensions/extension_service_unittest.cc

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc
(right):

https://codereview.chromium.org/1394993004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc:39:
scoped_ptr<base::Value> value = nullptr;
no need to initialize to nullptr.

[Olli Raula, 2015-10-13 07:19:33]

Thanks, I will check these things from all files and get back when ready. Just
for clarification, what "punt" means here: "Feel free to punt / file bugs."

[Lei Zhang, 2015-10-13 07:37:03]

On 2015/10/13 07:19:33, olli.raula wrote:
> Thanks, I will check these things from all files and get back when ready. Just
> for clarification, what "punt" means here: "Feel free to punt / file bugs."

Punt -> defer working on the issue to a later time, and possible deferring to
someone else.

I would say if you don't want to address all the comments, then file a bug,
describe the problem and note the places where the problems exist. In the code,
write comments and include the bug # for reference. If you want to take the bug,
assign it to yourself and add TODO(username) in the comments.

[Olli Raula, 2015-10-13 10:51:39]

"Can we provide some easier way to convert the return value to a
scoped_ptr<base::DictionaryValue>?"

I was thinking that already with those JSONReader patches because there are
similar cases. It would be nice to have a version which returns a
scoped_ptr<base::DictionaryValue> from jsonreader::read and deserializer. How
this sounds to you? Or just some function somewhere which do the conversion,
where?. 
in_reply_to: 5640471028170752
send_mail: 1
subject: Make ValueDeserializer::Deserialize return scoped_ptr

[Olli Raula, 2015-10-13 10:51:57]

"Can we provide some easier way to convert the return value to a
scoped_ptr<base::DictionaryValue>?"

I was thinking that already with those JSONReader patches because there are
similar cases. It would be nice to have a version which returns a
scoped_ptr<base::DictionaryValue> from jsonreader::read and deserializer. How
this sounds to you? Or just some function somewhere which do the conversion,
where?.

[Olli Raula, 2015-10-13 11:17:57]

On 2015/10/13 10:51:57, Olli Raula wrote:
> "Can we provide some easier way to convert the return value to a
> scoped_ptr<base::DictionaryValue>?"
> 
> I was thinking that already with those JSONReader patches because there are
> similar cases. It would be nice to have a version which returns a
> scoped_ptr<base::DictionaryValue> from jsonreader::read and deserializer. How
> this sounds to you? Or just some function somewhere which do the conversion,
> where?.

Something like this:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
to scoped_ptr. It is worth of implementation? Should I leave also that to
different cl?

[Lei Zhang, 2015-10-13 21:26:43]

On 2015/10/13 10:51:39, Olli Raula wrote:
> "Can we provide some easier way to convert the return value to a
> scoped_ptr<base::DictionaryValue>?"
> 
> I was thinking that already with those JSONReader patches because there are
> similar cases. It would be nice to have a version which returns a
> scoped_ptr<base::DictionaryValue> from jsonreader::read and deserializer. How
> this sounds to you? Or just some function somewhere which do the conversion,
> where?. 
> in_reply_to: 5640471028170752
> send_mail: 1
> subject: Make ValueDeserializer::Deserialize return scoped_ptr

Well, we already have base/value_conversions.h, maybe stuff a helper function in
there? How about this?

// If |in_value| is a DictionaryValue, move |in_value| into |out_value| and
return true. Otherwise leave |in_value| alone and return false.
bool GetValueAsDictionaryValue(
    scoped_ptr<Value>* in_value,
    scoped_ptr<DictionaryValue>* out_value);

[Olli Raula, 2015-10-14 07:11:57]

> // If |in_value| is a DictionaryValue, move |in_value| into |out_value| and
> return true. Otherwise leave |in_value| alone and return false.
> bool GetValueAsDictionaryValue(
>     scoped_ptr<Value>* in_value,
>     scoped_ptr<DictionaryValue>* out_value);

Thx, do you have some typo in this? It do not make sense to do it only if
in_value is dictionaryvalue as in value should be value.

[Lei Zhang, 2015-10-14 08:32:52]

On 2015/10/14 07:11:57, Olli Raula wrote:
> > // If |in_value| is a DictionaryValue, move |in_value| into |out_value| and
> > return true. Otherwise leave |in_value| alone and return false.
> > bool GetValueAsDictionaryValue(
> >     scoped_ptr<Value>* in_value,
> >     scoped_ptr<DictionaryValue>* out_value);
> 
> Thx, do you have some typo in this? It do not make sense to do it only if
> in_value is dictionaryvalue as in value should be value.

So we are talking about cases like this right?

scoped_ptr<base::DictionaryValue>
root(static_cast<base::DictionaryValue*>(deserializer.Deserialize(NULL,
&error).release()));

Instead, we'd have:

scoped_ptr<base::DictionaryValue> root;
scoped_ptr<base::Value> in_value = deserializer.Deserialize(NULL, &error);
// Assume GetValueAsDictionaryValue() handles the NULL |root_value| case
if (!GetValueAsDictionaryValue(&in_value, &root)) {
  in_value.reset();
  // Handle error
}

// Otherwise, |root| contains |in_value|'s DictionaryValue
// and |in_value| is empty.

Does this make sense?

[Olli Raula, 2015-10-14 08:53:41]

On 2015/10/14 08:32:52, Lei Zhang wrote:
> On 2015/10/14 07:11:57, Olli Raula wrote:
> > > // If |in_value| is a DictionaryValue, move |in_value| into |out_value|
and
> > > return true. Otherwise leave |in_value| alone and return false.
> > > bool GetValueAsDictionaryValue(
> > >     scoped_ptr<Value>* in_value,
> > >     scoped_ptr<DictionaryValue>* out_value);
> > 
> > Thx, do you have some typo in this? It do not make sense to do it only if
> > in_value is dictionaryvalue as in value should be value.
> 
> So we are talking about cases like this right?
> 
> scoped_ptr<base::DictionaryValue>
> root(static_cast<base::DictionaryValue*>(deserializer.Deserialize(NULL,
> &error).release()));
> 
> Instead, we'd have:
> 
> scoped_ptr<base::DictionaryValue> root;
> scoped_ptr<base::Value> in_value = deserializer.Deserialize(NULL, &error);
> // Assume GetValueAsDictionaryValue() handles the NULL |root_value| case
> if (!GetValueAsDictionaryValue(&in_value, &root)) {
>   in_value.reset();
>   // Handle error
> }
> 
> // Otherwise, |root| contains |in_value|'s DictionaryValue
> // and |in_value| is empty.
> 
> Does this make sense?

Ah, now I got. "If |in_value| is a DictionaryValue" means that |in_value| is
type of dictvalue(but that time it is normal value). In other words, return true
if |in_value| can be cast to dictvalue.

I think I should use this
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&q=d...
instead of static_cast

[Lei Zhang, 2015-10-14 09:09:08]

On 2015/10/14 08:53:41, Olli Raula wrote:
> On 2015/10/14 08:32:52, Lei Zhang wrote:
> > On 2015/10/14 07:11:57, Olli Raula wrote:
> > > > // If |in_value| is a DictionaryValue, move |in_value| into |out_value|
> and
> > > > return true. Otherwise leave |in_value| alone and return false.
> > > > bool GetValueAsDictionaryValue(
> > > >     scoped_ptr<Value>* in_value,
> > > >     scoped_ptr<DictionaryValue>* out_value);
> > > 
> > > Thx, do you have some typo in this? It do not make sense to do it only if
> > > in_value is dictionaryvalue as in value should be value.
> > 
> > So we are talking about cases like this right?
> > 
> > scoped_ptr<base::DictionaryValue>
> > root(static_cast<base::DictionaryValue*>(deserializer.Deserialize(NULL,
> > &error).release()));
> > 
> > Instead, we'd have:
> > 
> > scoped_ptr<base::DictionaryValue> root;
> > scoped_ptr<base::Value> in_value = deserializer.Deserialize(NULL, &error);
> > // Assume GetValueAsDictionaryValue() handles the NULL |root_value| case
> > if (!GetValueAsDictionaryValue(&in_value, &root)) {
> >   in_value.reset();
> >   // Handle error
> > }
> > 
> > // Otherwise, |root| contains |in_value|'s DictionaryValue
> > // and |in_value| is empty.
> > 
> > Does this make sense?
> 
> Ah, now I got. "If |in_value| is a DictionaryValue" means that |in_value| is
> type of dictvalue(but that time it is normal value). In other words, return
true
> if |in_value| can be cast to dictvalue.
> 
> I think I should use this
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&q=d...
> instead of static_cast

Actually, now that you pointed me to there, doesn't this do what we want?

scoped_ptr<DictionaryValue> DictionaryValue::From(scoped_ptr<Value> value)

[Olli Raula, 2015-10-14 09:22:54]

> Actually, now that you pointed me to there, doesn't this do what we want?
> 
> scoped_ptr<DictionaryValue> DictionaryValue::From(scoped_ptr<Value> value)

Quick tests says yes, thanks

[Olli Raula, 2015-10-14 15:13:37]

I have to leave before all bots are ready but they should be atleast mostly
fine. It would be nice if you have time to check changes and rest of the files.

[Lei Zhang, 2015-10-14 16:46:14]

Again, from only reviewing the chrome/ changes:

https://codereview.chromium.org/1394993004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_external_updater.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:49: // TODO(Olli
Raula) possible memoryleak http://crbug.com/543015
memory leak is two words.

https://codereview.chromium.org/1394993004/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/recovery_component_installer.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/recovery_component_installer.cc:122: if
(root.get() && root->IsType(base::Value::TYPE_DICTIONARY)) {
Is this redundant?

https://codereview.chromium.org/1394993004/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_icon_factory_unittest.cc
(right):

https://codereview.chromium.org/1394993004/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_icon_factory_unittest.cc:113:
base::DictionaryValue::From(deserializer.Deserialize(NULL, &error));
Didn't this use to pass in |error_code| instead of NULL?

[Lei Zhang, 2015-10-14 16:55:35]

Reviewed the rest.

https://codereview.chromium.org/1394993004/diff/100001/chromeos/network/onc/o...
File chromeos/network/onc/onc_test_utils.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/chromeos/network/onc/o...
chromeos/network/onc/onc_test_utils.cc:41: scoped_ptr<base::DictionaryValue>
dict = NULL;
No need to initialize to NULL.

https://codereview.chromium.org/1394993004/diff/100001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils_unittest.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils_unittest.cc:25: scoped_ptr<base::Value>
result(nullptr);
no need to initialize a scoped_ptr to nullptr.

https://codereview.chromium.org/1394993004/diff/100001/chromeos/tools/onc_val...
File chromeos/tools/onc_validator/onc_validator.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/chromeos/tools/onc_val...
chromeos/tools/onc_validator/onc_validator.cc:93: base::Value* value =
deserializer.Deserialize(NULL, &json_error).release();
I think this can leak too.

https://codereview.chromium.org/1394993004/diff/100001/extensions/browser/ext...
File extensions/browser/extension_icon_image_unittest.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/extensions/browser/ext...
extensions/browser/extension_icon_image_unittest.cc:108:
base::DictionaryValue::From(deserializer.Deserialize(NULL, &error));
This used to pass in |error_code|

https://codereview.chromium.org/1394993004/diff/100001/extensions/browser/ima...
File extensions/browser/image_loader_unittest.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/extensions/browser/ima...
extensions/browser/image_loader_unittest.cc:87:
base::DictionaryValue::From(deserializer.Deserialize(NULL, &error));
ditto re: |error_code|

https://codereview.chromium.org/1394993004/diff/100001/extensions/common/exte...
File extensions/common/extension_l10n_util.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/extensions/common/exte...
extensions/common/extension_l10n_util.cc:42: scoped_ptr<base::Value> dictionary
=
Let's use DictionaryValue::From() to help do the extra check of making sure
|dictionary| is actually a dictionary.

https://codereview.chromium.org/1394993004/diff/100001/ui/app_list/search/dic...
File ui/app_list/search/dictionary_data_store.cc (right):

https://codereview.chromium.org/1394993004/diff/100001/ui/app_list/search/dic...
ui/app_list/search/dictionary_data_store.cc:70: base::Value* value =
We can use more scoped_ptrs here.

[Olli Raula, 2015-10-15 12:35:43]

Thanks for comments. They are all done now!

[Lei Zhang, 2015-10-15 17:36:11]

https://codereview.chromium.org/1394993004/diff/160001/chrome/browser/compone...
File chrome/browser/component_updater/recovery_component_installer.cc (right):

https://codereview.chromium.org/1394993004/diff/160001/chrome/browser/compone...
chrome/browser/component_updater/recovery_component_installer.cc:123: if (root)
{
We don't need this either. Just return |root| ... we don't need |root| either,
just return base::DictionaryValue::From(...)

https://codereview.chromium.org/1394993004/diff/160001/chromeos/tools/onc_val...
File chromeos/tools/onc_validator/onc_validator.cc (right):

https://codereview.chromium.org/1394993004/diff/160001/chromeos/tools/onc_val...
chromeos/tools/onc_validator/onc_validator.cc:94: TODO(Olli Raula)
I think you forgot some slashes here. And again, "memory leak"

I'm a little worried about this file since the try bots passed.

[Olli Raula, 2015-10-16 05:42:08]

Fixed

> I'm a little worried about this file since the try bots passed.

I made a todo and filed a bug about that.

[Lei Zhang, 2015-10-16 05:53:43]

LGTM

Please go ahead and TBR a top level owner for everything else.

[Olli Raula, 2015-10-16 05:55:58]

The CQ bit was checked by olli.raula@intel.com

[commit-bot: I haz the power, 2015-10-16 05:56:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394993004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394993004/180001

[Lei Zhang, 2015-10-16 05:57:53]

thestig@chromium.org changed reviewers:
+ jam@chromium.org

[Lei Zhang, 2015-10-16 05:57:53]

and actually +jam (for TBR)

[commit-bot: I haz the power, 2015-10-16 06:16:50]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-16 06:17:51]

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