Refactor JSONValueConverter Out of DisplayLayout

Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498
Committed: https://crrev.com/8c912b533f434aa8509495bf0dd6e2dd177d0fce
Cr-Commit-Position: refs/heads/master@{#382398}

[robliao, 2016-03-17 19:40:45]

Description was changed from

==========
Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498
==========

to

==========
Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498
==========

[robliao, 2016-03-17 19:40:45]

robliao@chromium.org changed reviewers:
+ oshima@chromium.org

[robliao, 2016-03-17 19:42:27]

oshima: Here's the first in a sequence of three changes to share code between
Windows and ChromeOS.

1) [This CL] Refactor JSONValueConverter out of DisplayLayout
2) Move DisplayLayout and DisplayLayoutBuilder to ui/gfx
3) MultiDPI WIndows Change with DisplayLayout

Once the tests pass for (2), I'll send that out next. It's a much larger change
as it includes a namespace change.

Thanks!

[oshima, 2016-03-17 22:06:24]

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

[commit-bot: I haz the power, 2016-03-17 22:07:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808253004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808253004/1

[commit-bot: I haz the power, 2016-03-17 22:41:09]

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

[commit-bot: I haz the power, 2016-03-17 22:41:10]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-03-17 23:19:39]

https://codereview.chromium.org/1808253004/diff/1/ash/display/display_layout.h
File ash/display/display_layout.h (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/display_layout....
ash/display/display_layout.h:85: ScopedVector<DisplayPlacement> placement_list;
can you change this to std::vector?

The reason for ScopedVector was JSONConvert which doesn't support std::vector.

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter.cc
File ash/display/json_converter.cc (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:50: //     The key is missing - output is left
unchanged
can we have a separate function that checks for this? It makes it a bit hard to
follow, and there are very few cases that key may not exist in the code below.

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:127: if (!list_item->GetAsDictionary(&item_values)
|| item_values == nullptr)
do you need || item_values == nullptr?

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:150: if (!value.GetAsDictionary(&dict_value) ||
dict_value == nullptr)
ditto

[robliao, 2016-03-18 21:39:57]

https://codereview.chromium.org/1808253004/diff/1/ash/display/display_layout.h
File ash/display/display_layout.h (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/display_layout....
ash/display/display_layout.h:85: ScopedVector<DisplayPlacement> placement_list;
On 2016/03/17 23:19:38, oshima wrote:
> can you change this to std::vector?
> 
> The reason for ScopedVector was JSONConvert which doesn't support std::vector.

This will be a large change, available on
https://codereview.chromium.org/1819533002/

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter.cc
File ash/display/json_converter.cc (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:50: //     The key is missing - output is left
unchanged
On 2016/03/17 23:19:38, oshima wrote:
> can we have a separate function that checks for this? It makes it a bit hard
to
> follow, and there are very few cases that key may not exist in the code below.

This oddly enough is the behavior of JSONValueConverter (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
and 
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
)

Having said that, I suspect we would end up doing something similar to this
regardless of what we decide to do with regards to a missing key since we would
rubber stamp the below for each key.

So the real question is what missing keys should trigger an error?

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:127: if (!list_item->GetAsDictionary(&item_values)
|| item_values == nullptr)
On 2016/03/17 23:19:38, oshima wrote:
> do you need || item_values == nullptr?

That's a good question. I just pulled this from the existing
ConvertFromValue/ConvertToValue code.

Looking at the GetAsDictionary implementations, it looks like we can remove
this.

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:150: if (!value.GetAsDictionary(&dict_value) ||
dict_value == nullptr)
On 2016/03/17 23:19:38, oshima wrote:
> ditto

Resolved. See above.

[oshima, 2016-03-18 21:53:24]

haven't looked the patch yet because I'm still firefighting with DrMemory
redness. I'll try to review this before 4pm.

https://codereview.chromium.org/1808253004/diff/1/ash/display/display_layout.h
File ash/display/display_layout.h (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/display_layout....
ash/display/display_layout.h:85: ScopedVector<DisplayPlacement> placement_list;
On 2016/03/18 21:39:57, robliao wrote:
> On 2016/03/17 23:19:38, oshima wrote:
> > can you change this to std::vector?
> > 
> > The reason for ScopedVector was JSONConvert which doesn't support
std::vector.
> 
> This will be a large change, available on
> https://codereview.chromium.org/1819533002/

Acknowledged.

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter.cc
File ash/display/json_converter.cc (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:50: //     The key is missing - output is left
unchanged
On 2016/03/18 21:39:57, robliao wrote:
> On 2016/03/17 23:19:38, oshima wrote:
> > can we have a separate function that checks for this? It makes it a bit hard
> to
> > follow, and there are very few cases that key may not exist in the code
below.
> 
> This oddly enough is the behavior of JSONValueConverter (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
> and 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
> )
> 
> Having said that, I suspect we would end up doing something similar to this
> regardless of what we decide to do with regards to a missing key since we
would
> rubber stamp the below for each key.
> 
> So the real question is what missing keys should trigger an error?
> 

It should use just a default value, maybe error message in case we need to
diagnose the feedback. We had to update the format once in a while, and we tries
to be backward compatible.

[robliao, 2016-03-18 22:55:58]

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter.cc
File ash/display/json_converter.cc (right):

https://codereview.chromium.org/1808253004/diff/1/ash/display/json_converter....
ash/display/json_converter.cc:50: //     The key is missing - output is left
unchanged
On 2016/03/18 21:53:23, oshima wrote:
> On 2016/03/18 21:39:57, robliao wrote:
> > On 2016/03/17 23:19:38, oshima wrote:
> > > can we have a separate function that checks for this? It makes it a bit
hard
> > to
> > > follow, and there are very few cases that key may not exist in the code
> below.
> > 
> > This oddly enough is the behavior of JSONValueConverter (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
> > and 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
> > )
> > 
> > Having said that, I suspect we would end up doing something similar to this
> > regardless of what we decide to do with regards to a missing key since we
> would
> > rubber stamp the below for each key.
> > 
> > So the real question is what missing keys should trigger an error?
> > 
> 
> It should use just a default value, maybe error message in case we need to
> diagnose the feedback. We had to update the format once in a while, and we
tries
> to be backward compatible.

Sounds like this is what we want then. The input is the default value and so
we'll keep the default if we can't find the field. Added a warning message.

[robliao, 2016-03-19 00:05:24]

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

[commit-bot: I haz the power, 2016-03-19 00:05:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808253004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808253004/40001

[commit-bot: I haz the power, 2016-03-19 01:02:18]

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

[commit-bot: I haz the power, 2016-03-19 01:02:19]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-03-21 19:16:53]

lgtm with nit

https://codereview.chromium.org/1808253004/diff/40001/ash/display/json_conver...
File ash/display/json_converter.cc (right):

https://codereview.chromium.org/1808253004/diff/40001/ash/display/json_conver...
ash/display/json_converter.cc:9: #include <unordered_map>
do you need this (and map) ?

[robliao, 2016-03-21 20:13:02]

https://codereview.chromium.org/1808253004/diff/40001/ash/display/json_conver...
File ash/display/json_converter.cc (right):

https://codereview.chromium.org/1808253004/diff/40001/ash/display/json_conver...
ash/display/json_converter.cc:9: #include <unordered_map>
On 2016/03/21 19:16:53, oshima wrote:
> do you need this (and map) ?
Nope. Done. These were needed during code transition. We don't need these
anymore.

[robliao, 2016-03-21 20:13:12]

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

[commit-bot: I haz the power, 2016-03-21 20:13:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808253004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808253004/60001

[commit-bot: I haz the power, 2016-03-21 20:57:45]

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

[commit-bot: I haz the power, 2016-03-21 20:57:46]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-03-21 21:26:51]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-03-21 21:26:52]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1808253004/#ps60001
(title: "Clean up some Includes")

[commit-bot: I haz the power, 2016-03-21 21:27:11]

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

[commit-bot: I haz the power, 2016-03-21 21:34:25]

Description was changed from

==========
Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498
==========

to

==========
Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498
==========

[commit-bot: I haz the power, 2016-03-21 21:34:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-21 21:36:04]

Description was changed from

==========
Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498
==========

to

==========
Refactor JSONValueConverter Out of DisplayLayout

Separate the JSON processing code out of DisplayLayout in preparing to move to
ui/gfx since ui/gfx has no JSON dependency.

BUG=595498

Committed: https://crrev.com/8c912b533f434aa8509495bf0dd6e2dd177d0fce
Cr-Commit-Position: refs/heads/master@{#382398}
==========

[commit-bot: I haz the power, 2016-03-21 21:36:06]

Patchset 4 (id:??) landed as
https://crrev.com/8c912b533f434aa8509495bf0dd6e2dd177d0fce
Cr-Commit-Position: refs/heads/master@{#382398}