Move serialize/deserialize logic onto Entry.

Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The idea is we'll end up with two instances of the catalog per user, linked, one with the user-specific entries, and one with the entries that came with the distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors. Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=

Committed: https://crrev.com/96bdd00a3c4db5cdf69e3e3e5a875c31ac413224
Cr-Commit-Position: refs/heads/master@{#382633}

[Ben Goodger (Google), 2016-03-22 14:35:45]

Description was changed from

==========
Move serialize/deserialize logic onto Entry.

R=sky@chromium.org

BUG=
==========

to

==========
Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The
idea is we'll end up with two instances of the catalog per user, linked, one
with the user-specific entries, and one with the entries that came with the
distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors.
Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=
==========

[sky, 2016-03-22 15:10:03]

https://codereview.chromium.org/1818373003/diff/1/mojo/services/catalog/entry.h
File mojo/services/catalog/entry.h (right):

https://codereview.chromium.org/1818373003/diff/1/mojo/services/catalog/entry...
mojo/services/catalog/entry.h:33: static DeserializeResult Deserialize(const
base::DictionaryValue& value,
It seems like you only check the return value for SUCCESS. WDYT of having this
return a scoped_ptr<Entry> and on failure return null? Or did I miss where you
differentiate the return value?

[Ben Goodger (Google), 2016-03-22 15:22:09]

On 2016/03/22 15:10:03, sky wrote:
>
https://codereview.chromium.org/1818373003/diff/1/mojo/services/catalog/entry.h
> File mojo/services/catalog/entry.h (right):
> 
>
https://codereview.chromium.org/1818373003/diff/1/mojo/services/catalog/entry...
> mojo/services/catalog/entry.h:33: static DeserializeResult Deserialize(const
> base::DictionaryValue& value,
> It seems like you only check the return value for SUCCESS. WDYT of having this
> return a scoped_ptr<Entry> and on failure return null? Or did I miss where you
> differentiate the return value?

I'm not differentiating return value (yet). Though perhaps it doesn't matter as
long as this code logs the reason.

Returning scoped_ptr is sort of meaningless as the object is currently copied
into the catalog. I still haven't worked out how I want the model to look.

[Ben Goodger (Google), 2016-03-22 15:34:44]

On 2016/03/22 15:22:09, Ben Goodger (Google) wrote:
> On 2016/03/22 15:10:03, sky wrote:
> >
>
https://codereview.chromium.org/1818373003/diff/1/mojo/services/catalog/entry.h
> > File mojo/services/catalog/entry.h (right):
> > 
> >
>
https://codereview.chromium.org/1818373003/diff/1/mojo/services/catalog/entry...
> > mojo/services/catalog/entry.h:33: static DeserializeResult Deserialize(const
> > base::DictionaryValue& value,
> > It seems like you only check the return value for SUCCESS. WDYT of having
this
> > return a scoped_ptr<Entry> and on failure return null? Or did I miss where
you
> > differentiate the return value?
> 
> I'm not differentiating return value (yet). Though perhaps it doesn't matter
as
> long as this code logs the reason.
> 
> Returning scoped_ptr is sort of meaningless as the object is currently copied
> into the catalog. I still haven't worked out how I want the model to look.

OK I thought about it more and I don't think the error codes are meaningful to
report back as long as this code LOGs the specific errors. Updated as you
suggested.

[sky, 2016-03-22 16:24:44]

LGTM

[Ben Goodger (Google), 2016-03-22 17:48:19]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2016-03-22 17:48:45]

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

[commit-bot: I haz the power, 2016-03-22 19:23:14]

Description was changed from

==========
Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The
idea is we'll end up with two instances of the catalog per user, linked, one
with the user-specific entries, and one with the entries that came with the
distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors.
Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=
==========

to

==========
Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The
idea is we'll end up with two instances of the catalog per user, linked, one
with the user-specific entries, and one with the entries that came with the
distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors.
Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=
==========

[commit-bot: I haz the power, 2016-03-22 19:23:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-22 19:24:41]

Description was changed from

==========
Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The
idea is we'll end up with two instances of the catalog per user, linked, one
with the user-specific entries, and one with the entries that came with the
distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors.
Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=
==========

to

==========
Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The
idea is we'll end up with two instances of the catalog per user, linked, one
with the user-specific entries, and one with the entries that came with the
distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors.
Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=

Committed: https://crrev.com/96bdd00a3c4db5cdf69e3e3e5a875c31ac413224
Cr-Commit-Position: refs/heads/master@{#382633}
==========

[commit-bot: I haz the power, 2016-03-22 19:24:42]

Patchset 2 (id:??) landed as
https://crrev.com/96bdd00a3c4db5cdf69e3e3e5a875c31ac413224
Cr-Commit-Position: refs/heads/master@{#382633}