Prototype code that turns any/every flattenable into JSON

Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

Changes to public API are just removals and changes to make
some functions virtual.

TBR=reed@google.com

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

Committed: https://skia.googlesource.com/skia/+/fad98562d8f9db63839a8d902a301b174320f27f

[Brian Osman, 2016-04-27 00:46:17]

Description was changed from

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

to

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

[Brian Osman, 2016-04-27 00:58:10]

Description was changed from

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

to

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/vS56Enuynqr.png

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

[Brian Osman, 2016-04-27 01:01:07]

brianosman@google.com changed reviewers:
+ bsalomon@google.com, ethannicholas@google.com, mtklein@google.com,
reed@google.com

[Brian Osman, 2016-04-27 01:01:08]

Something I tinkered with last week, and wanted to get opinions on. (I showed
this to a few folks already). Basically, we've been manually expanding the json
coverage of things in skiaserve but it's a manual and time-consuming process.
There are still lots of holes. Something like this lets us get a huge jump in
coverage.

[Brian Osman, 2016-04-27 01:01:26]

brianosman@google.com changed reviewers:
+ jcgregorio@google.com

[ethannicholas, 2016-04-27 13:54:41]

lgtm

[mtklein, 2016-04-27 13:54:54]

I like both approaches, and while I like the other better, I do agree this one
gets us 80% of the way there less invasively.  I'd greedily want to see if we
can make the other one work for us first before falling back to this option.

Both approaches require making most calls on SkWriteBuffer virtual, which I want
to specifically call out as unlikely to be a big deal.  The most
performance-sensitive use of SkWriteBuffer is in SkPaint.cpp (fill_out_rec), and
that's already quite allocate-y and virtual-call-y.

[reed1, 2016-04-27 13:59:57]

My guess is the virtuals are fine too. Do we have good nanobenches that time our
serialization step for some sample pictures (or even just timing each
Flattenable subclass separately: e.g. flatten gradients, blurs, etc.)?

[Brian Osman, 2016-04-27 14:06:32]

On 2016/04/27 13:54:54, mtklein wrote:
> I like both approaches, and while I like the other better, I do agree this one
> gets us 80% of the way there less invasively.  I'd greedily want to see if we
> can make the other one work for us first before falling back to this option.
> 
> Both approaches require making most calls on SkWriteBuffer virtual, which I
want
> to specifically call out as unlikely to be a big deal.  The most
> performance-sensitive use of SkWriteBuffer is in SkPaint.cpp (fill_out_rec),
and
> that's already quite allocate-y and virtual-call-y.

Thoughts: Yeah, I wasn't sure about the cost of virtual. When I discussed this
with
Brian, he seemed to think that this serialization was a pretty rare case, and
we're
likely to be bottlenecked by other things things when it happens (but it's worth
validating that).

Also: The current implementation, where I derive from the actual default
implementation
of SkWriteBuffer is not ideal OOP. I'd be happy to change SkWriteBuffer to an
interface,
then make SkBinaryWriteBuffer and SkJsonWriteBuffer implement that.

I also prefer the output from the other implementation, but possibly with some
changes
to make it nicer? (Comments on that coming separately...)

[mtklein, 2016-04-27 14:15:57]

On 2016/04/27 at 14:06:32, brianosman wrote:
> On 2016/04/27 13:54:54, mtklein wrote:
> > I like both approaches, and while I like the other better, I do agree this
one
> > gets us 80% of the way there less invasively.  I'd greedily want to see if
we
> > can make the other one work for us first before falling back to this option.
> > 
> > Both approaches require making most calls on SkWriteBuffer virtual, which I
want
> > to specifically call out as unlikely to be a big deal.  The most
> > performance-sensitive use of SkWriteBuffer is in SkPaint.cpp (fill_out_rec),
and
> > that's already quite allocate-y and virtual-call-y.
> 
> Thoughts: Yeah, I wasn't sure about the cost of virtual. When I discussed this
with
> Brian, he seemed to think that this serialization was a pretty rare case, and
we're
> likely to be bottlenecked by other things things when it happens (but it's
worth
> validating that).

Agreed.  Until recently, the only place we used serialization in non-debugging
situations
was that function in SkPaint.cpp, as a key to identify text strikes.  We've
since picked
up Blimp, but this sort of thing is unlikely to be anywhere near their
bottleneck.

 
> Also: The current implementation, where I derive from the actual default
implementation
> of SkWriteBuffer is not ideal OOP. I'd be happy to change SkWriteBuffer to an
interface,
> then make SkBinaryWriteBuffer and SkJsonWriteBuffer implement that.

Yeah, my experiences with SkCanvas and SkReadBuffer make me prefer an interface.

> I also prefer the output from the other implementation, but possibly with some
changes
> to make it nicer? (Comments on that coming separately...)

As long as we keep those names restricted to being debugger-only, I don't see
anything
preventing us from launch-and-iterate here.  But it is tempting to use those
names for 
other purposes that need longer-term stability...

[mtklein, 2016-04-27 14:17:09]

On 2016/04/27 at 13:59:57, reed wrote:
> My guess is the virtuals are fine too. Do we have good nanobenches that time
our serialization step for some sample pictures (or even just timing each
Flattenable subclass separately: e.g. flatten gradients, blurs, etc.)?

I don't think we have benchmarks covering any part of picture or effect
serialization or deserialization.

[Brian Osman, 2016-04-28 14:21:13]

Description was changed from

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/vS56Enuynqr.png

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

to

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

[Brian Osman, 2016-04-28 14:22:33]

Converted to an interface, made a few changes to get the output more
human-readable. Updated the 'After' screenshot to show that (and also show a
more complex case of nested types, with a draw-looper leading to another paint,
etc...)

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

https://codereview.chromium.org/1920423002/diff/40001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:35: virtual uint32_t* reserve(size_t size) = 0;
All of the other "binary" parts (and implementation details) of the interface
are only used in places where we can be sure that we have an
SkBinaryWriteBuffer. This function is used in exactly one place, when flattening
an SkPaint. I've arranged things so we should never encounter it in the JSON
implementation, but also made the JSON version safe. FYI.

https://codereview.chromium.org/1920423002/diff/40001/tools/debugger/SkDrawCo...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/1920423002/diff/40001/tools/debugger/SkDrawCo...
tools/debugger/SkDrawCommand.cpp:67: #define SKDEBUGCANVAS_ATTRIBUTE_VALUES     
      "values"
This was previously "flattened", and I think Mike's suggestion of "fields" would
be even better. Unfortunately, that makes the resulting JSON hard to parse (for
humans). Using an identifier that comes lexicographically after "name" ensures
that we get:

"name" : "SkFoo",
"values" : {
# Big block of data, possibly with more nested flattenables
},

... rather than the opposite.

[Brian Osman, 2016-04-28 14:22:41]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-28 14:22:48]

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

[commit-bot: I haz the power, 2016-04-28 14:24:19]

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

[commit-bot: I haz the power, 2016-04-28 14:24:21]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[Brian Osman, 2016-04-28 14:49:33]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-28 14:49:42]

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

[Brian Osman, 2016-04-28 15:36:06]

The CQ bit was unchecked by brianosman@google.com

[mtklein, 2016-04-29 17:55:43]

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

https://codereview.chromium.org/1920423002/diff/60001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:71: class SkBinaryWriteBuffer : public
SkWriteBuffer {
final?

https://codereview.chromium.org/1920423002/diff/60001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:95: void writeDataAsByteArray(SkData* data)
override {
Now that this is virtual, it's a little confusing to also inline it.

We might want to move writeDataAsByteArray(), writeFunctionPtr() and
writePaint() into the .cpp?

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkDrawCo...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkDrawCo...
tools/debugger/SkDrawCommand.cpp:607: {
Just curious, does this scope have any effect?

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
File tools/debugger/SkJsonWriteBuffer.cpp (right):

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:111: this->append("flattenable", "<null>");
Does this happen?  Might wanna SkASSERT(false)?

Is there a reason we don't pass Json::Value()?

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:116: SkString hexColor =
SkStringPrintf("#%08x", color);
This leading # tempts me to parse this as a CSS color, but they only do #RRGGBB,
right?  (This is #AARRGGBB.)

If we're trying to look similar to CSS, let's go all the way to rgba(r, g, b, a)
where r,g,b are [0,255] and a is [0,1]?

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:149: #ifndef SK_IGNORE_TO_STRING
As far as I can tell, no one defines this.  Is that why we have/need no backup
plan here?

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:195: void
SkJsonWriteBuffer::writeImage(const SkImage* image) {
Do we have a way to side-channel the original bitmaps/images as standalone .pngs
(or however they were originally encoded) and serve them the same way we do the
raw flattened effects?

I think for SkImage it's as easy as calling ->encode(), and SkBitmap is probably
just SkImage::MakeFromBitmap(bitmap)->encode().

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:202: this->append("typeface",
Json::Value());
Might be nice to surface up some basics:
   - getFamilyName()
   - fontStyle()
   - uniqueID()

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:206: // TODO: Store a reference to the
UrlDataManager in this class, then call back into
Haven't you done this here?

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
File tools/debugger/SkJsonWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.h:22: class SkJsonWriteBuffer : public
SkWriteBuffer {
final?

[Brian Osman, 2016-04-29 19:59:48]

Addressed most of the feedback. I think there's definitely still room to
fine-tune the json for various types, but this is quite a bit better.

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkDrawCo...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkDrawCo...
tools/debugger/SkDrawCommand.cpp:607: {
On 2016/04/29 17:55:42, mtklein wrote:
> Just curious, does this scope have any effect?

Nope. I think it's leftover kruft from an earlier version.

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
File tools/debugger/SkJsonWriteBuffer.cpp (right):

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:111: this->append("flattenable", "<null>");
On 2016/04/29 17:55:42, mtklein wrote:
> Does this happen?  Might wanna SkASSERT(false)?
> 
> Is there a reason we don't pass Json::Value()?

1) Until I switched to using make_json_paint, yes. The paint serializes all the
filters (if any of them are present), so you do end up with null flattenables. I
could imagine other code doing the same thing.
2) My (somewhat silly) thinking on the explicit null tag was to clearly
disambiguate a null pointer from the case of a flattenable that doesn't write
any fields in flatten().

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:116: SkString hexColor =
SkStringPrintf("#%08x", color);
On 2016/04/29 17:55:42, mtklein wrote:
> This leading # tempts me to parse this as a CSS color, but they only do
#RRGGBB,
> right?  (This is #AARRGGBB.)
> 
> If we're trying to look similar to CSS, let's go all the way to rgba(r, g, b,
a)
> where r,g,b are [0,255] and a is [0,1]?

Yeah - for some reason I was thinking this was legal CSS, but it's not. I think
I'll do the rgba() thing, although that's trading structure for simpler use-case
on the JS end.

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:149: #ifndef SK_IGNORE_TO_STRING
On 2016/04/29 17:55:42, mtklein wrote:
> As far as I can tell, no one defines this.  Is that why we have/need no backup
> plan here?

I was just basing this on every other call-site (where this check is included).
Although it's probably better/simpler to just emit structured JSON of the matrix
anyway.

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:195: void
SkJsonWriteBuffer::writeImage(const SkImage* image) {
On 2016/04/29 17:55:42, mtklein wrote:
> Do we have a way to side-channel the original bitmaps/images as standalone
.pngs
> (or however they were originally encoded) and serve them the same way we do
the
> raw flattened effects?
> 
> I think for SkImage it's as easy as calling ->encode(), and SkBitmap is
probably
> just SkImage::MakeFromBitmap(bitmap)->encode().

Actually, the code that's in SkDrawCommand does that, so that now that I have
the UrlDataManager, I can do the same mutually-recursive thing I'm doing for
SkPaint.

https://codereview.chromium.org/1920423002/diff/60001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:206: // TODO: Store a reference to the
UrlDataManager in this class, then call back into
On 2016/04/29 17:55:42, mtklein wrote:
> Haven't you done this here?

Acknowledged.

[Brian Osman, 2016-04-29 19:59:55]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 20:00:08]

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

[mtklein, 2016-04-29 20:05:13]

https://codereview.chromium.org/1920423002/diff/80001/tools/debugger/SkJsonWr...
File tools/debugger/SkJsonWriteBuffer.cpp (right):

https://codereview.chromium.org/1920423002/diff/80001/tools/debugger/SkJsonWr...
tools/debugger/SkJsonWriteBuffer.cpp:190: extern bool flatten(const SkImage&
image, Json::Value* target, UrlDataManager& urlDataManager);
Let's put these declarations in SkDrawCommand.h?  There's nothing sneaky about
them, rght?

[reed1, 2016-05-02 12:44:39]

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:32: virtual bool isCrossProcess() const = 0;
Can/should we consider making this non-virtual, and always storing a flags field
in the base-class?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:35: virtual void writeDataAsByteArray(SkData* data)
= 0;
does the "SkData" version need to be virtual?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:42: virtual void write32(int32_t value) = 0;
How is this different from writeInt? Lets either simplify or document why we
have both.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:44: virtual void writeEncodedString(const void*
value, size_t byteLength,
Do we use writeEncodedString?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:46: virtual void writeFunctionPtr(void* ptr) = 0;
do we still need writeFunctionPtr?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:49: virtual void writeColor(const SkColor& color) =
0;
take param by value?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:58: virtual size_t writeStream(SkStream* stream,
size_t length) = 0;
doe writeStream need to be virtual? Doesn't it just devolve into writeByteArray?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:59: virtual void writeBitmap(const SkBitmap&
bitmap) = 0;
Does writeBitmap need to be virtual? i.e. does it produce something different
than what writeImage produces?

[mtklein, 2016-05-02 13:55:15]

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:26: 
We might try moving this whole file to include/private?

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:32: virtual bool isCrossProcess() const = 0;
On 2016/05/02 at 12:44:39, reed1 wrote:
> Can/should we consider making this non-virtual, and always storing a flags
field in the base-class?

If it's at all possible I'd prefer an interface here to an abstract base class.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:35: virtual void writeDataAsByteArray(SkData* data)
= 0;
On 2016/05/02 at 12:44:39, reed1 wrote:
> does the "SkData" version need to be virtual?

Making these virtual gives us finer type information in the debugger.

E.g. this shows up as "xx_data": "..." rather than "xx_byteArray": "...".

I don't think this is particularly important... if "xx_byteArray" is unclear, 
the best answer is to thread through the full field name.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:36: virtual void writeBool(bool value) = 0;;
Stray ;.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:44: virtual void writeEncodedString(const void*
value, size_t byteLength,
On 2016/05/02 at 12:44:39, reed1 wrote:
> Do we use writeEncodedString?

No.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:46: virtual void writeFunctionPtr(void* ptr) = 0;
On 2016/05/02 at 12:44:39, reed1 wrote:
> do we still need writeFunctionPtr?

No.  Its one use, in tests/ImageFilterTest.cpp, is both incorrect and
unnecessary.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:58: virtual size_t writeStream(SkStream* stream,
size_t length) = 0;
On 2016/05/02 at 12:44:39, reed1 wrote:
> doe writeStream need to be virtual? Doesn't it just devolve into
writeByteArray?

Yes; no.  If we devolve into writeByteArray(), that requires that we copy the
stream to a temporary contiguous buffer.  writeStream() avoids that extra copy.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:59: virtual void writeBitmap(const SkBitmap&
bitmap) = 0;
On 2016/05/02 at 12:44:39, reed1 wrote:
> Does writeBitmap need to be virtual? i.e. does it produce something different
than what writeImage produces?

Yes, today writeBitmap() serializes some formats as raw pixels, while
writeImage() skips them.

[reed1, 2016-05-02 14:04:42]

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:32: virtual bool isCrossProcess() const = 0;
On 2016/05/02 13:55:15, mtklein wrote:
> On 2016/05/02 at 12:44:39, reed1 wrote:
> > Can/should we consider making this non-virtual, and always storing a flags
> field in the base-class?
> 
> If it's at all possible I'd prefer an interface here to an abstract base
class.

What are the advantages? I was motivated to just simplify cases where subclasses
would replicate the same logic (and reduce the shear number of virtuals). Not
critical, seems like a small plus.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:35: virtual void writeDataAsByteArray(SkData* data)
= 0;
On 2016/05/02 13:55:15, mtklein wrote:
> On 2016/05/02 at 12:44:39, reed1 wrote:
> > does the "SkData" version need to be virtual?
> 
> Making these virtual gives us finer type information in the debugger.
> 
> E.g. this shows up as "xx_data": "..." rather than "xx_byteArray": "...".
> 
> I don't think this is particularly important... if "xx_byteArray" is unclear, 
> the best answer is to thread through the full field name.

Yea, I thought about that too, but thinking further ahead, I think that
distinction seems more like an impl detail of the caller, and not conveying
(much) information to the reader (the reader object/client, not the code
reviewer).

As another example, we could tag the buffer that a block came from a stream and
not from an array, but to the reader object, I think they are the same.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:58: virtual size_t writeStream(SkStream* stream,
size_t length) = 0;
On 2016/05/02 13:55:15, mtklein wrote:
> On 2016/05/02 at 12:44:39, reed1 wrote:
> > doe writeStream need to be virtual? Doesn't it just devolve into
> writeByteArray?
> 
> Yes; no.  If we devolve into writeByteArray(), that requires that we copy the
> stream to a temporary contiguous buffer.  writeStream() avoids that extra
copy.

Good point about efficiency. I wonder if that could be inverted, and we just ask
each stream subclass to efficiently write themselves to the buffer? Just
thinking out loud (and thinking about sharing common code/logic).

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:59: virtual void writeBitmap(const SkBitmap&
bitmap) = 0;
On 2016/05/02 13:55:15, mtklein wrote:
> On 2016/05/02 at 12:44:39, reed1 wrote:
> > Does writeBitmap need to be virtual? i.e. does it produce something
different
> than what writeImage produces?
> 
> Yes, today writeBitmap() serializes some formats as raw pixels, while
> writeImage() skips them.

Yea, but that distinction seems more historical than important (if it is
important, I would like to understand more). I'm fine if we leave this as is for
now, and investigate the differences in a separate thread/CL.

[Brian Osman, 2016-05-02 14:49:12]

Klein hit on most of my responses before I could finish writing them. Just
wanted to throw in my 2 cents on a few of these...

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:35: virtual void writeDataAsByteArray(SkData* data)
= 0;
On 2016/05/02 14:04:42, reed1 wrote:
> On 2016/05/02 13:55:15, mtklein wrote:
> > On 2016/05/02 at 12:44:39, reed1 wrote:
> > > does the "SkData" version need to be virtual?
> > 
> > Making these virtual gives us finer type information in the debugger.
> > 
> > E.g. this shows up as "xx_data": "..." rather than "xx_byteArray": "...".
> > 
> > I don't think this is particularly important... if "xx_byteArray" is
unclear, 
> > the best answer is to thread through the full field name.
> 
> Yea, I thought about that too, but thinking further ahead, I think that
> distinction seems more like an impl detail of the caller, and not conveying
> (much) information to the reader (the reader object/client, not the code
> reviewer).
> 
> As another example, we could tag the buffer that a block came from a stream
and
> not from an array, but to the reader object, I think they are the same.

As discussed - I had split it off to capture the entry-point used. There's only
one user of read/writeData, though, so it really comes down to the pure
interface vs. abstract base class, I think. An earlier version of my change has
some shared code in the base-class, and I ended up moving towards an
interface... I don't have a real strong opinion either way, except that we be
consistent (share where it makes sense, or not at all).

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:42: virtual void write32(int32_t value) = 0;
On 2016/05/02 12:44:39, reed1 wrote:
> How is this different from writeInt? Lets either simplify or document why we
> have both.

They appear to be totally interchangeable in their use-cases. The underlying
writer also has write32 (which is used at this level for all 32-bit integral
types), but also has writeInt, etc... We can definitely narrow down, though it
may be better to do that in a follow-up change, because of how much calling code
will be touched.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:49: virtual void writeColor(const SkColor& color) =
0;
On 2016/05/02 12:44:39, reed1 wrote:
> take param by value?

Sure.

[mtklein, 2016-05-02 15:04:43]

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:35: virtual void writeDataAsByteArray(SkData* data)
= 0;
On 2016/05/02 at 14:49:12, Brian Osman wrote:
> On 2016/05/02 14:04:42, reed1 wrote:
> > On 2016/05/02 13:55:15, mtklein wrote:
> > > On 2016/05/02 at 12:44:39, reed1 wrote:
> > > > does the "SkData" version need to be virtual?
> > > 
> > > Making these virtual gives us finer type information in the debugger.
> > > 
> > > E.g. this shows up as "xx_data": "..." rather than "xx_byteArray": "...".
> > > 
> > > I don't think this is particularly important... if "xx_byteArray" is
unclear, 
> > > the best answer is to thread through the full field name.
> > 
> > Yea, I thought about that too, but thinking further ahead, I think that
> > distinction seems more like an impl detail of the caller, and not conveying
> > (much) information to the reader (the reader object/client, not the code
> > reviewer).
> > 
> > As another example, we could tag the buffer that a block came from a stream
and
> > not from an array, but to the reader object, I think they are the same.
> 
> As discussed - I had split it off to capture the entry-point used. There's
only one user of read/writeData, though, so it really comes down to the pure
interface vs. abstract base class, I think. An earlier version of my change has
some shared code in the base-class, and I ended up moving towards an
interface... I don't have a real strong opinion either way, except that we be
consistent (share where it makes sense, or not at all).

My preference for an interface here is mostly because I'm tired of working with
class hierarchies where there is a complex relationship between the base class
and sub class, like SkCanvas and or SkReadBuffer.  It's hard to write a correct
subclass of those types.  Usually these things only get complicated when the
base class maintains state itself.

I don't have any objection to adding (really, keeping) stateless non-virtual
aliases to reduce the surface of the interface, e.g.

void writeDataAsByteArray(SkData* data) {
   this->writeByteArray(data->data(), data->size());
}

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:42: virtual void write32(int32_t value) = 0;
On 2016/05/02 at 14:49:12, Brian Osman wrote:
> On 2016/05/02 12:44:39, reed1 wrote:
> > How is this different from writeInt? Lets either simplify or document why we
> > have both.
> 
> They appear to be totally interchangeable in their use-cases. The underlying
writer also has write32 (which is used at this level for all 32-bit integral
types), but also has writeInt, etc... We can definitely narrow down, though it
may be better to do that in a follow-up change, because of how much calling code
will be touched.

Seems like another fine candidate to alias on the interface, then later remove
if we still care.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:49: virtual void writeColor(const SkColor& color) =
0;
On 2016/05/02 at 14:49:12, Brian Osman wrote:
> On 2016/05/02 12:44:39, reed1 wrote:
> > take param by value?
> 
> Sure.

I suspect points and rects are also faster to pass by value, fwiw.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:58: virtual size_t writeStream(SkStream* stream,
size_t length) = 0;
> Good point about efficiency. I wonder if that could be inverted, and we just
ask each stream subclass to efficiently write themselves to the buffer? Just
thinking out loud (and thinking about sharing common code/logic).

We've got more streams than write buffers, so I think we'd end up with more,
more complex code that way.
Given that we pass a 'length' parameter through here, the method we've got is
probably the one best way
for all streams: allocate length bytes, then read() that many from the stream.

https://codereview.chromium.org/1920423002/diff/120001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:59: virtual void writeBitmap(const SkBitmap&
bitmap) = 0;
On 2016/05/02 at 14:04:42, reed1 wrote:
> On 2016/05/02 13:55:15, mtklein wrote:
> > On 2016/05/02 at 12:44:39, reed1 wrote:
> > > Does writeBitmap need to be virtual? i.e. does it produce something
different
> > than what writeImage produces?
> > 
> > Yes, today writeBitmap() serializes some formats as raw pixels, while
> > writeImage() skips them.
> 
> Yea, but that distinction seems more historical than important (if it is
important, I would like to understand more). I'm fine if we leave this as is for
now, and investigate the differences in a separate thread/CL.

Agreed.  Generally I want to always do
SkImage::MakeFromBitmap(bitmap)->encode(), and similar.  Not very interested in
thinking of SkBitmaps as special.

[Brian Osman, 2016-05-03 13:50:46]

The CQ bit was checked by brianosman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 13:50:52]

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

[commit-bot: I haz the power, 2016-05-03 14:09:15]

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

[commit-bot: I haz the power, 2016-05-03 14:09:16]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-05-03 14:13:52]

Description was changed from

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

I have another version of this change that goes *much* further, and
requires an explicit name on every call to any of the SkWriteBuffer
members. That lets us come up with a naming standard, and makes the
resulting JSON very useful, but at the expense of a huge amount of code,
and forcing all call-sites to supply a string that's dropped on the floor
by the default code-path (ie the 99.9% case).

You can see the other code here: https://codereview.chromium.org/1911963008/

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

to

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

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

[Brian Osman, 2016-05-03 14:16:07]

I think this version is better. I actually moved several of the static functions
from SkDrawCommand.cpp into the header (and adjusted their naming to match a
couple that were already exposed). That means that we're only using one set of
code to translate complex types into structured JSON, rather than having two
sets of code that might do slightly different things.

A quick code survey actually reveals three more candidates for removal from the
interface, though: intArray, pointArray, and IRect. :/

[mtklein, 2016-05-04 15:35:09]

This way still lgtm.

https://codereview.chromium.org/1920423002/diff/180001/include/core/SkWriteBu...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/180001/include/core/SkWriteBu...
include/core/SkWriteBuffer.h:31: virtual bool isCrossProcess() const = 0;
Just a thought: another way to reduce the burden on SkWriteBuffer subclasses
without having to maintain state here is to default this to

    virtual bool isCrossProcess() const { return false; }

That way only the buffer that cares need care.

https://codereview.chromium.org/1920423002/diff/180001/tools/debugger/SkJsonW...
File tools/debugger/SkJsonWriteBuffer.h (right):

https://codereview.chromium.org/1920423002/diff/180001/tools/debugger/SkJsonW...
tools/debugger/SkJsonWriteBuffer.h:2: * Copyright 2012 Google Inc.
Probably not?

[Brian Osman, 2016-05-04 17:22:07]

The CQ bit was checked by brianosman@google.com

[Brian Osman, 2016-05-04 17:22:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
ethannicholas@google.com, mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1920423002/#ps200001
(title: "Fix copyright dates in new files")

[commit-bot: I haz the power, 2016-05-04 17:22:18]

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

[commit-bot: I haz the power, 2016-05-04 17:23:48]

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

[commit-bot: I haz the power, 2016-05-04 17:23:49]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[Brian Osman, 2016-05-04 17:58:25]

Description was changed from

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

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

to

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

Changes to public API are just removals and changes to make
some functions virtual.

TBR=reed@google.com

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

[Brian Osman, 2016-05-04 17:59:02]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-05-04 17:59:10]

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

[commit-bot: I haz the power, 2016-05-04 18:06:31]

Description was changed from

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

Changes to public API are just removals and changes to make
some functions virtual.

TBR=reed@google.com

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

to

==========
Prototype code that turns any/every flattenable into JSON

This makes inspecting things in SkDebugger far more useful - any filter
or other complex object on the paint is ultimately visible. You still
have to do some guess work to figure out what the fields actually mean,
but you can at least cross-reference with the code in flatten().

Screenshots:
Before: https://screenshot.googleplex.com/a6JM5HBBe6G.png
After : https://screenshot.googleplex.com/XQfr4YJ6mnH.png

Changes to public API are just removals and changes to make
some functions virtual.

TBR=reed@google.com

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

Committed:
https://skia.googlesource.com/skia/+/fad98562d8f9db63839a8d902a301b174320f27f
==========

[commit-bot: I haz the power, 2016-05-04 18:06:32]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/fad98562d8f9db63839a8d902a301b174320f27f

[hal.canary, 2016-05-04 18:17:56]

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

[hal.canary, 2016-05-04 18:17:57]

++awesome.