SkValue: to/from SkImage

SkValue: to/from SkImage

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

[hal.canary, 2016-01-21 21:01:32]

Description was changed from

==========
SkValue: to/from SkImage
==========

to

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

[hal.canary, 2016-01-21 21:02:29]

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

[hal.canary, 2016-01-21 21:02:30]

trying to keep the next few CLs small.

[mtklein, 2016-01-21 21:21:30]

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.cpp
File src/effects/SkToFromValue.cpp (right):

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:47: REQUIRE(   val.type() == SkValue::Bytes
Guess we'd better put isData() out in public for now.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:141: enum { kEncoded, kUniqueID };  //
SkValue::Image
?

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:145: if (!encoded || 0 == encoded->size()) {
why 0 == encoded->size()?

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:150: val.set(kUniqueID,
SkValue::FromU32(image->uniqueID()));
This seems like a bad idea, especially if we ignore it in SkFromValue().

Those uniqueIDs are unique to the current process, but have nothing to do with
the image itself.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:154: template<> bool SkFromValue<
SkAutoTUnref<SkImage> >(
template<> bool SkFromValue<SkAutoTUnref<SkImage>>(const SkValue& val,
SkAutoTUnref<SkImage>* dst) {

is 100 columns.

[hal.canary, 2016-01-21 22:03:23]

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.cpp
File src/effects/SkToFromValue.cpp (right):

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:47: REQUIRE(   val.type() == SkValue::Bytes
On 2016/01/21 at 21:21:30, mtklein wrote:
> Guess we'd better put isData() out in public for now.

done

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:141: enum { kEncoded, kUniqueID };  //
SkValue::Image
On 2016/01/21 at 21:21:29, mtklein wrote:
> ?

moved.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:145: if (!encoded || 0 == encoded->size()) {
On 2016/01/21 at 21:21:30, mtklein wrote:
> why 0 == encoded->size()?

removed.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:150: val.set(kUniqueID,
SkValue::FromU32(image->uniqueID()));
On 2016/01/21 at 21:21:30, mtklein wrote:
> This seems like a bad idea, especially if we ignore it in SkFromValue().
> 
> Those uniqueIDs are unique to the current process, but have nothing to do with
the image itself.

a serializer can use them to de-dup images.  Or anything else with a uniqueID.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:154: template<> bool SkFromValue<
SkAutoTUnref<SkImage> >(
On 2016/01/21 at 21:21:30, mtklein wrote:
> template<> bool SkFromValue<SkAutoTUnref<SkImage>>(const SkValue& val,
SkAutoTUnref<SkImage>* dst) {
> 
> is 100 columns.

yes but my *editor* doesn't know about how C++11 allows >> to sometimes not be a
shift operator.

[hal.canary, 2016-01-21 22:27:05]

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

[commit-bot: I haz the power, 2016-01-21 22:27:16]

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

[commit-bot: I haz the power, 2016-01-22 00:27:34]

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

[commit-bot: I haz the power, 2016-01-22 00:27:34]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_TIMED_OUT, no build URL)

[hal.canary, 2016-01-22 13:18:45]

Description was changed from

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

to

==========
SkValue: to/from SkImage

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

[mtklein, 2016-01-22 14:09:18]

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.cpp
File src/effects/SkToFromValue.cpp (right):

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:144: SkAutoTUnref<SkData>
encoded(image->encode());
you know, this is probably the most controversial line of the whole CL.
it's not clear to me that we should be encoding these images.
that means,
   1) they must be encoded and decoded if you're going to use SkValues, which
may not be necessary
   2) the client gets no choice in how that encoding or decoding happens

Let's leave images for later to think more about.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:150: val.set(kUniqueID,
SkValue::FromU32(image->uniqueID()));
On 2016/01/21 22:03:23, Hal Canary wrote:
> On 2016/01/21 at 21:21:30, mtklein wrote:
> > This seems like a bad idea, especially if we ignore it in SkFromValue().
> > 
> > Those uniqueIDs are unique to the current process, but have nothing to do
with
> the image itself.
> 
> a serializer can use them to de-dup images.  Or anything else with a uniqueID.

this unique ID is not part of the value of the image.  it does not belong in its
value.

https://codereview.chromium.org/1613053006/diff/1/src/effects/SkToFromValue.c...
src/effects/SkToFromValue.cpp:154: template<> bool SkFromValue<
SkAutoTUnref<SkImage> >(
On 2016/01/21 22:03:23, Hal Canary wrote:
> On 2016/01/21 at 21:21:30, mtklein wrote:
> > template<> bool SkFromValue<SkAutoTUnref<SkImage>>(const SkValue& val,
> SkAutoTUnref<SkImage>* dst) {
> > 
> > is 100 columns.
> 
> yes but my *editor* doesn't know about how C++11 allows >> to sometimes not be
a
> shift operator.

are you seriously proposing that we write code strangely to coddle the
eccentricities of the editor you have chosen to use?