sketch SkValue API

sketch SkValue API

I think we may be trying to take too many steps at once.
Let's try starting from an API and evolving from there?

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

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

[mtklein_C, 2016-01-14 15:40:06]

Description was changed from

==========
sketch SkValue API

I think we may be trying to take too many steps at once.
Let's try starting from an API and evolving from there?

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

to

==========
sketch SkValue API

I think we may be trying to take too many steps at once.
Let's try starting from an API and evolving from there?

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

[mtklein_C, 2016-01-14 15:49:05]

mtklein@chromium.org changed reviewers:
+ halcanary@google.com

[hal.canary, 2016-01-14 16:11:45]

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h
File src/core/SkValue.h (right):

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:19: Byte , S16 , U16 , S32 , U32 , S64 , U64 , F32 , F64 ,
I thought you preferred U8 to byte.

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:37: SkValue(const SkValue&);
what does it mean to copy a SkValue that has complex type?  a deep copy? 
reference count?

[mtklein, 2016-01-14 16:45:15]

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

[mtklein, 2016-01-14 16:45:15]

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h
File src/core/SkValue.h (right):

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:19: Byte , S16 , U16 , S32 , U32 , S64 , U64 , F32 , F64 ,
I was thinking of adding Bit too, as a signal to Objects that they may be stored
hyper-efficiently.  E.g.

class Object {
...
Map fMap;
uint64_t fBits;
...
void set(Key k, Value v) {
   if (v.type() == Bit && k < 64) {
      // set kth bit in fBits to v.bit()
   } else {
      // store as generic value in fMap
   }
}

Wouldn't want to implement that until it's useful, but it might be nice to start
off with a Bit type to see if it gets used.

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:19: Byte , S16 , U16 , S32 , U32 , S64 , U64 , F32 , F64 ,
On 2016/01/14 16:11:45, Hal Canary wrote:
> I thought you preferred U8 to byte.

I thought so too.  But then I realized I don't think I've ever seen a use of
int8_t in Skia that's not better a uint8_t or an int.  When we work with bytes,
we don't really care or think about their sign.

For comparison, Reader32/Writer32/ReadBuffer/WriteBuffer all don't support S8. 
Streams have readS8() method that's completely unused except to implement
readU8().

If you prefer S8, U8, that seems fine to me.  But Bytes still seems like it
wants to be Bytes, not split into S8s/U8s.

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:37: SkValue(const SkValue&);
On 2016/01/14 16:11:45, Hal Canary wrote:
> what does it mean to copy a SkValue that has complex type?  a deep copy? 
> reference count?

From an API perspective, it doesn't matter how we copy, as long as it's copied
(that is, we produce an observably-equivalent SkValue whose lifetime is
decoupled from the original's).

I'm open to either implementation, though I probably wouldn't want SkValues to
be publicly ref-countable.  I don't mind if we ref-count things like Bytes and
Object.  E.g. Bytes can be implemented with SkData.

[hal.canary, 2016-01-14 19:16:37]

halcanary@google.com changed reviewers:
- mtklein@google.com

[hal.canary, 2016-01-14 19:16:38]

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h
File src/core/SkValue.h (right):

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:48: static SkValue FromBytes(const void*, size_t);
does this copy?

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:49: static SkValue Object(Type);
 static SkValue List(int reserve = 0);

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:62: void set(Key, SkValue);
set(Key, SkValue&&);
set(Key, const SkValue&);

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:65: 
void append(const Value&);
void append(Value&&);

[hal.canary, 2016-01-14 19:26:21]

lgtm

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h
File src/core/SkValue.h (right):

https://codereview.chromium.org/1589953003/diff/40001/src/core/SkValue.h#newc...
src/core/SkValue.h:65: 
const SkValue* getFromList(int);

[mtklein, 2016-01-14 19:30:15]

The CQ bit was checked by mtklein@google.com

[mtklein, 2016-01-14 19:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from halcanary@google.com
Link to the patchset: https://codereview.chromium.org/1589953003/#ps60001
(title: "hal")

[commit-bot: I haz the power, 2016-01-14 19:30:16]

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

[commit-bot: I haz the power, 2016-01-14 19:43:15]

Description was changed from

==========
sketch SkValue API

I think we may be trying to take too many steps at once.
Let's try starting from an API and evolving from there?

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

to

==========
sketch SkValue API

I think we may be trying to take too many steps at once.
Let's try starting from an API and evolving from there?

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/+/f074e707491e4e80f971e52c84d4f3e294564edb
==========

[commit-bot: I haz the power, 2016-01-14 19:43:16]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/f074e707491e4e80f971e52c84d4f3e294564edb