DNC - JSON of flattenables, with field names.

Json everything!

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1911963008

[Brian Osman, 2016-04-22 19:03:20]

Description was changed from

==========
Json everything!

BUG=skia:
==========

to

==========
Json everything!

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein, 2016-04-27 13:55:03]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2016-04-27 13:55:05]

https://codereview.chromium.org/1911963008/diff/40001/include/core/SkWriteBuf...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1911963008/diff/40001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:27: #define SK_STRING_AND_VALUE(x) #x , x
Looks like this is used sparsely.

What are your thoughts on this vs. manually writing everything out?  Too much
miscellaneous baggage, like .get(), (int), arrays?

I can see arguments that both of two extremes are possibly friendliest to
debugger users:
   - literal quotation of the value as done here, to make cross-referencing with
the implementation easy;
   - use the same human friendly names as the constructors / factories to make
cross-referencing with the API easy.

[Brian Osman, 2016-04-27 14:12:00]

On 2016/04/27 13:55:05, mtklein wrote:
>
https://codereview.chromium.org/1911963008/diff/40001/include/core/SkWriteBuf...
> File include/core/SkWriteBuffer.h (right):
> 
>
https://codereview.chromium.org/1911963008/diff/40001/include/core/SkWriteBuf...
> include/core/SkWriteBuffer.h:27: #define SK_STRING_AND_VALUE(x) #x , x
> Looks like this is used sparsely.
> 
> What are your thoughts on this vs. manually writing everything out?  Too much
> miscellaneous baggage, like .get(), (int), arrays?
> 
> I can see arguments that both of two extremes are possibly friendliest to
> debugger users:
>    - literal quotation of the value as done here, to make cross-referencing
with
> the implementation easy;
>    - use the same human friendly names as the constructors / factories to make
> cross-referencing with the API easy.

... yeah. This change (obviously) involved a lot of typing, and I ended up
having different
ideas about policy as I went. Unfortunately, I ended up not being super
consistent, as a
result - I'd have to clean it up and use one strategy everywhere.

I originally created the macro, imagining I'd use that almost everywhere, and
then I ran
into all the sub-struct, array, accessor stuff you mentioned, and decided that
it would
make the json pretty ugly. I also agree that both extremes are useful, although
I think
I'd tend to pick the explicit human-readable version, if forced to express an
opinion.

It's also possible to move the names to the end of the calls, defaulting to
nullptr. Then
we could emit the type (as in the other change), unless a name is given. That
just lets us
name-ify all the calls consistently over time. (Although, this took me a couple
hours, so
it's probably better to just make the names required and do it once, correctly).

[mtklein, 2016-04-27 14:26:52]

On 2016/04/27 at 14:12:00, brianosman wrote:
> On 2016/04/27 13:55:05, mtklein wrote:
> >
https://codereview.chromium.org/1911963008/diff/40001/include/core/SkWriteBuf...
> > File include/core/SkWriteBuffer.h (right):
> > 
> >
https://codereview.chromium.org/1911963008/diff/40001/include/core/SkWriteBuf...
> > include/core/SkWriteBuffer.h:27: #define SK_STRING_AND_VALUE(x) #x , x
> > Looks like this is used sparsely.
> > 
> > What are your thoughts on this vs. manually writing everything out?  Too
much
> > miscellaneous baggage, like .get(), (int), arrays?
> > 
> > I can see arguments that both of two extremes are possibly friendliest to
> > debugger users:
> >    - literal quotation of the value as done here, to make cross-referencing
with
> > the implementation easy;
> >    - use the same human friendly names as the constructors / factories to
make
> > cross-referencing with the API easy.
> 
> ... yeah. This change (obviously) involved a lot of typing, and I ended up
having different
> ideas about policy as I went. Unfortunately, I ended up not being super
consistent, as a
> result - I'd have to clean it up and use one strategy everywhere.
> 
> I originally created the macro, imagining I'd use that almost everywhere, and
then I ran
> into all the sub-struct, array, accessor stuff you mentioned, and decided that
it would
> make the json pretty ugly. I also agree that both extremes are useful,
although I think
> I'd tend to pick the explicit human-readable version, if forced to express an
opinion.

SGTM.

> It's also possible to move the names to the end of the calls, defaulting to
nullptr. Then
> we could emit the type (as in the other change), unless a name is given. That
just lets us
> name-ify all the calls consistently over time. (Although, this took me a
couple hours, so
> it's probably better to just make the names required and do it once,
correctly).

I'd be happy to start with optional names, and I don't even think that's
necessarily a
long-term bad state to be in.  For many flattened effects, the field types are
plenty
expressive: consider SkComposeShader, SkLocalMatrixImageFilter... once you know
the type
name and the field types and values recursively, there's not much added by
naming the fields.

Now things like SkMatrixConvolutionImageFilter... kind of a mess without field
names.
I suspect as these effects become visible in the debugger we'll get a sense for
which need 
field names and just fill them in as we go.

Making these optional also does prevent people from relying on them for other
uses, which
is probably a feature.  My more devious part was already thinking about how to
use these
field names in other ways...

[Brian Osman, 2016-05-06 12:42:12]

Description was changed from

==========
Json everything!

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Json everything!

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========