[Doodle] Add conversion to dictionary for DoodleImage and DoodleConfig

[Doodle] Add conversion to dictionary for DoodleImage and DoodleConfig

This also adds a bunch of tests for the conversion to/from dict.

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

[Marc Treib, 2017-03-13 11:19:17]

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

[commit-bot: I haz the power, 2017-03-13 11:19:30]

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 11:22:31]

Description was changed from

==========
[Doodle] Add conversion to dictionary for DoodleImage and DoodleConfig

This also adds a bunch of tests for the parsing.

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

to

==========
[Doodle] Add conversion to dictionary for DoodleImage and DoodleConfig

This also adds a bunch of tests for the conversion to/from dict.

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

[Marc Treib, 2017-03-13 11:22:44]

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

[Marc Treib, 2017-03-13 11:22:45]

PTAL!

[Marc Treib, 2017-03-13 11:23:38]

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

[commit-bot: I haz the power, 2017-03-13 11:23:50]

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

[fhorschig, 2017-03-13 11:42:05]

lgtm!

[commit-bot: I haz the power, 2017-03-13 12:20:44]

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

[commit-bot: I haz the power, 2017-03-13 12:20:45]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-03-13 12:28:59]

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

[Marc Treib, 2017-03-13 12:28:59]

+sfiera for committer's stamp, once again. PTAL!

[sfiera, 2017-03-13 13:07:53]

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

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:247:
DoodleConfig::FromDictionary(*config.ToDictionary(), base::nullopt);
Should fields be preserved when round-tripping with a different base URL? Should
they be changed?

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:253: 
Invalid URLs? I think you have a DCHECK when making a dict with one (e.g.
{"url": "bad::scheme"}), and I'm not sure we should be DCHECKing validity of
data from a backend.

[Marc Treib, 2017-03-13 13:29:12]

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

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:247:
DoodleConfig::FromDictionary(*config.ToDictionary(), base::nullopt);
On 2017/03/13 13:07:53, sfiera wrote:
> Should fields be preserved when round-tripping with a different base URL?
Should
> they be changed?

They should be preserved - the resolving only applies to relative URLs, and we
always persist absolute ones. (The whole resolving is a bit of a hack...)

I've added a different base_url here and added a comment to explain.

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:253: 
On 2017/03/13 13:07:53, sfiera wrote:
> Invalid URLs? I think you have a DCHECK when making a dict with one (e.g.
> {"url": "bad::scheme"}), and I'm not sure we should be DCHECKing validity of
> data from a backend.

Which DCHECK do you mean?
I've added tests with invalid URLs; the code already handled that.

[Marc Treib, 2017-03-13 13:29:34]

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

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

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

[sfiera, 2017-03-13 13:46:15]

LGTM

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

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:253: 
On 2017/03/13 13:29:12, Marc Treib wrote:
> On 2017/03/13 13:07:53, sfiera wrote:
> > Invalid URLs? I think you have a DCHECK when making a dict with one (e.g.
> > {"url": "bad::scheme"}), and I'm not sure we should be DCHECKing validity of
> > data from a backend.
> 
> Which DCHECK do you mean?
> I've added tests with invalid URLs; the code already handled that.

GURL::spec() DCHECKs on non-empty, invalid URLs. You call spec() in
ToDictionary(). "url" was the wrong example; I think "search_url" would trigger
it.

[Marc Treib, 2017-03-13 14:03:45]

Patchset #4 (id:60001) has been deleted

[Marc Treib, 2017-03-13 14:04:27]

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

[Marc Treib, 2017-03-13 14:04:32]

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

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:253: 
On 2017/03/13 13:46:15, sfiera wrote:
> On 2017/03/13 13:29:12, Marc Treib wrote:
> > On 2017/03/13 13:07:53, sfiera wrote:
> > > Invalid URLs? I think you have a DCHECK when making a dict with one (e.g.
> > > {"url": "bad::scheme"}), and I'm not sure we should be DCHECKing validity
of
> > > data from a backend.
> > 
> > Which DCHECK do you mean?
> > I've added tests with invalid URLs; the code already handled that.
> 
> GURL::spec() DCHECKs on non-empty, invalid URLs. You call spec() in
> ToDictionary(). "url" was the wrong example; I think "search_url" would
trigger
> it.

Ah - the GURL construction handles that, it returns empty GURLs for invalid
input. I've added EXPECTs here to make sure the GURLs are empty.
Good catch though, thanks!

[commit-bot: I haz the power, 2017-03-13 14:04:36]

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

[sfiera, 2017-03-13 14:10:24]

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

https://codereview.chromium.org/2748653002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types_unittest.cc:253: 
On 2017/03/13 14:04:32, Marc Treib wrote:
> On 2017/03/13 13:46:15, sfiera wrote:
> > On 2017/03/13 13:29:12, Marc Treib wrote:
> > > On 2017/03/13 13:07:53, sfiera wrote:
> > > > Invalid URLs? I think you have a DCHECK when making a dict with one
(e.g.
> > > > {"url": "bad::scheme"}), and I'm not sure we should be DCHECKing
validity
> of
> > > > data from a backend.
> > > 
> > > Which DCHECK do you mean?
> > > I've added tests with invalid URLs; the code already handled that.
> > 
> > GURL::spec() DCHECKs on non-empty, invalid URLs. You call spec() in
> > ToDictionary(). "url" was the wrong example; I think "search_url" would
> trigger
> > it.
> 
> Ah - the GURL construction handles that, it returns empty GURLs for invalid
> input. I've added EXPECTs here to make sure the GURLs are empty.
> Good catch though, thanks!

Hmm. I wonder how I knew about this, then—I could swear I've hit that DCHECK
before, but now I don't even know how to create an invalid GURL like that :)

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

The CQ bit was unchecked by treib@chromium.org

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

The CQ bit was checked by treib@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
fhorschig@chromium.org, sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2748653002/#ps80001
(title: "check invalid URL parsing result")

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

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 16:06:16]

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

[commit-bot: I haz the power, 2017-03-13 16:06:17]

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

[Marc Treib, 2017-03-14 08:01:34]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-03-14 08:01:54]

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-14 08:40:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489478494603710,
"parent_rev": "689574a569c0f9a83b918c011f05bebde3267183", "commit_rev":
"42b88efd1fc0721c8c97e1d5dfb89e4b9ce58fa9"}

[commit-bot: I haz the power, 2017-03-14 08:41:17]

Description was changed from

==========
[Doodle] Add conversion to dictionary for DoodleImage and DoodleConfig

This also adds a bunch of tests for the conversion to/from dict.

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

to

==========
[Doodle] Add conversion to dictionary for DoodleImage and DoodleConfig

This also adds a bunch of tests for the conversion to/from dict.

BUG=690467

Review-Url: https://codereview.chromium.org/2748653002
Cr-Commit-Position: refs/heads/master@{#456655}
Committed:
https://chromium.googlesource.com/chromium/src/+/42b88efd1fc0721c8c97e1d5dfb8...
==========

[commit-bot: I haz the power, 2017-03-14 08:41:19]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/42b88efd1fc0721c8c97e1d5dfb8...