[Doodle] Move parsing from fetcher into the types themselves

[Doodle] Move parsing from fetcher into the types themselves

BUG=690467
Review-Url: https://codereview.chromium.org/2740013005
Cr-Commit-Position: refs/heads/master@{#456343}
Committed: https://chromium.googlesource.com/chromium/src/+/e5a8f430055ca5b8604f5c852306fe686b6e8732

[Marc Treib, 2017-03-10 14:13:55]

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

[Marc Treib, 2017-03-10 14:14:06]

treib@chromium.org changed reviewers:
+ fhorschig@chromium.org

[Marc Treib, 2017-03-10 14:14:06]

PTAL!

[commit-bot: I haz the power, 2017-03-10 14:14:12]

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

[Marc Treib, 2017-03-10 14:15:36]

(Should also add some tests for the parsing specifically - I'll do that once we
also have ToDictionary methods. For now, this is covered fairly well by the
fetcher unit tests.)

[commit-bot: I haz the power, 2017-03-10 14:45:28]

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

[commit-bot: I haz the power, 2017-03-10 14:45:29]

Dry run: This issue passed the CQ dry run.

[fhorschig, 2017-03-10 17:54:01]

lgtm w/ nit

https://codereview.chromium.org/2740013005/diff/1/components/doodle/doodle_ty...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2740013005/diff/1/components/doodle/doodle_ty...
components/doodle/doodle_types.cc:110: auto large_image = ParseImage(dict,
"large_image", base_url);
nit: That's a borderline-use of auto since the return type is a
base::Optional which hides pretty well that this is a struct
and not an actual image.

[Marc Treib, 2017-03-13 09:10:00]

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

[commit-bot: I haz the power, 2017-03-13 09:10:09]

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

[Marc Treib, 2017-03-13 09:10:22]

nit addressed

https://codereview.chromium.org/2740013005/diff/1/components/doodle/doodle_ty...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2740013005/diff/1/components/doodle/doodle_ty...
components/doodle/doodle_types.cc:110: auto large_image = ParseImage(dict,
"large_image", base_url);
On 2017/03/10 17:54:01, fhorschig wrote:
> nit: That's a borderline-use of auto since the return type is a
> base::Optional which hides pretty well that this is a struct
> and not an actual image.

Good catch - this was mostly me being too lazy to type.
Done.

[Marc Treib, 2017-03-13 09:10:41]

treib@chromium.org changed reviewers:
+ sfiera@chromium.org

[Marc Treib, 2017-03-13 09:10:42]

+sfiera for committer's approval, PTAL!

[sfiera, 2017-03-13 09:31:16]

LGTM

https://codereview.chromium.org/2740013005/diff/20001/components/doodle/doodl...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2740013005/diff/20001/components/doodle/doodl...
components/doodle/doodle_types.cc:58: return base::nullopt;
Doesn't seem like you're gaining anything by using Optional here. You have the
notion of an invalid DoodleImage already (!url.is_valid()) and it would simplify
DoodleConfig::FromDictionary() if you returned DoodleImage() here.

[Marc Treib, 2017-03-13 09:37:34]

https://codereview.chromium.org/2740013005/diff/20001/components/doodle/doodl...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2740013005/diff/20001/components/doodle/doodl...
components/doodle/doodle_types.cc:58: return base::nullopt;
On 2017/03/13 09:31:16, sfiera wrote:
> Doesn't seem like you're gaining anything by using Optional here. You have the
> notion of an invalid DoodleImage already (!url.is_valid()) and it would
simplify
> DoodleConfig::FromDictionary() if you returned DoodleImage() here.

I was going for consistency with DoodleConfig. But you're right, it's a bit
inconsistent now.
I'd prefer to go in the other direction though: Let's get rid of invalid
DoodleImage and DoodleConfig instances and instead use Optional where necessary.
I'll address that in a follow-up.

[Marc Treib, 2017-03-13 09:37:45]

The CQ bit was unchecked by treib@chromium.org

[Marc Treib, 2017-03-13 09:37:46]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-03-13 09:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
fhorschig@chromium.org
Link to the patchset: https://codereview.chromium.org/2740013005/#ps20001
(title: "-auto")

[commit-bot: I haz the power, 2017-03-13 09:37:51]

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-03-13 09:42:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489397866127760,
"parent_rev": "c81b87fafaf8470819b51c4b159ff16e6af6a6f3", "commit_rev":
"e5a8f430055ca5b8604f5c852306fe686b6e8732"}

[commit-bot: I haz the power, 2017-03-13 09:42:52]

Description was changed from

==========
[Doodle] Move parsing from fetcher into the types themselves

BUG=690467
==========

to

==========
[Doodle] Move parsing from fetcher into the types themselves

BUG=690467

Review-Url: https://codereview.chromium.org/2740013005
Cr-Commit-Position: refs/heads/master@{#456343}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5a8f430055ca5b8604f5c852306...
==========

[commit-bot: I haz the power, 2017-03-13 09:42:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e5a8f430055ca5b8604f5c852306...