SkMultiSKP: version 2

SkMultiSKP: version 2

Measurable size improvement.

BUG=skia:5370
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003

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

[hal.canary, 2016-08-19 11:41:16]

Description was changed from

==========
SkMultiSKP: version 2

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

to

==========
SkMultiSKP: version 2

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003
==========

[hal.canary, 2016-08-19 16:58:54]

Description was changed from

==========
SkMultiSKP: version 2

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003
==========

to

==========
SkMultiSKP: version 2

Measurable size improvement.
==========

[hal.canary, 2016-08-19 17:01:38]

Description was changed from

==========
SkMultiSKP: version 2

Measurable size improvement.
==========

to

==========
SkMultiSKP: version 2

Measurable size improvement.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003
==========

[hal.canary, 2016-08-19 17:01:48]

Patchset #1 (id:1) has been deleted

[hal.canary, 2016-08-19 17:01:52]

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

[hal.canary, 2016-08-19 17:01:58]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-19 17:02:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-08-19 17:02:33]

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

[hal.canary, 2016-08-19 17:02:34]

PTAL

[commit-bot: I haz the power, 2016-08-19 17:30:31]

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

[commit-bot: I haz the power, 2016-08-19 17:30:31]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-08-22 11:50:54]

Description was changed from

==========
SkMultiSKP: version 2

Measurable size improvement.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003
==========

to

==========
SkMultiSKP: version 2

Measurable size improvement.

BUG=skia:5370

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003
==========

[tomhudson, 2016-08-23 12:12:19]

lgtm

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
File src/utils/SkMultiPictureDocument.cpp (right):

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
src/utils/SkMultiPictureDocument.cpp:45: SkTArray<sk_sp<SkPicture>> fPages;
Switching from std::vector<> back to SkTArray<> is better? Convenient? Necessary
because sk_sp<> (and then we use another one for SkSize just for consistency)?
Accidental?

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
File src/utils/SkMultiPictureDocumentReader.cpp (right):

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
src/utils/SkMultiPictureDocumentReader.cpp:36: good &= sizeof(size) ==
stream->read(&size, sizeof(size));
This is probably correct precedence, but can we throw in parentheses to make it
clear? Or are you philosophically opposed to that?

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
File src/utils/SkMultiPictureDocumentReader.h (right):

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
src/utils/SkMultiPictureDocumentReader.h:28: /** Call this after calling init()
*/
Does this mean "you must call this after calling init()"? Or "the return value
is only valid after calling init()"? Or ... ?

[hal.canary, 2016-08-23 13:47:34]

Cary, also see src/utils/SkMultiPictureDocument.h

[tomhudson, 2016-08-23 13:52:41]

Per our 8:30am discussion please also put a snippet in the header about your
intent - there are known shortcomings, this is debugging support code rather
than something intended for production.

[hal.canary, 2016-08-23 15:53:39]

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
File src/utils/SkMultiPictureDocument.cpp (right):

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
src/utils/SkMultiPictureDocument.cpp:45: SkTArray<sk_sp<SkPicture>> fPages;
On 2016/08/23 12:12:19, tomhudson wrote:
> Switching from std::vector<> back to SkTArray<> is better? Convenient?
Necessary
> because sk_sp<> (and then we use another one for SkSize just for consistency)?
> Accidental?

Accidental.  I have no strong preference.

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
File src/utils/SkMultiPictureDocumentReader.cpp (right):

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
src/utils/SkMultiPictureDocumentReader.cpp:36: good &= sizeof(size) ==
stream->read(&size, sizeof(size));
On 2016/08/23 12:12:19, tomhudson wrote:
> This is probably correct precedence, but can we throw in parentheses
> to make it clear? Or are you philosophically opposed to that?

I understood that the secret implicit Skia style guide philosophically
opposes unnecessary parentheses.   Therefore I have taped a table of
operator precedence to my monitor.

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
File src/utils/SkMultiPictureDocumentReader.h (right):

https://codereview.chromium.org/2255333003/diff/40001/src/utils/SkMultiPictur...
src/utils/SkMultiPictureDocumentReader.h:28: /** Call this after calling init()
*/
On 2016/08/23 12:12:19, tomhudson wrote:
> Does this mean "you must call this after calling init()"? Or "the return value
> is only valid after calling init()"? Or ... ?

 /** Call this after calling init() (otherwise you'll always get zero). */

fixed

[hal.canary, 2016-08-23 15:53:51]

On 2016/08/23 13:52:41, tomhudson wrote:
> Per our 8:30am discussion please also put a snippet in the header about your
> intent - there are known shortcomings, this is debugging support code rather
> than something intended for production.

done

[hal.canary, 2016-08-23 15:53:58]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2016-08-23 15:53:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/2255333003/#ps60001
(title: "2016-08-23 (Tuesday) 11:53:05 EDT")

[commit-bot: I haz the power, 2016-08-23 15:54:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 16:15:06]

Description was changed from

==========
SkMultiSKP: version 2

Measurable size improvement.

BUG=skia:5370

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003
==========

to

==========
SkMultiSKP: version 2

Measurable size improvement.

BUG=skia:5370

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255333003

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

[commit-bot: I haz the power, 2016-08-23 16:15:07]

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