Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1391023005:
SkRemote (Closed)
DescriptionSkRemote
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/2e2ea38bb77d42bab292919de5d80286cc354782
Patch Set 1 #Patch Set 2 : color #Patch Set 3 : stroke, text, fill, etc. #Patch Set 4 : fake dimensions #Patch Set 5 : dtor #Patch Set 6 : final #Patch Set 7 : names #Patch Set 8 : f #Patch Set 9 : fmt #Patch Set 10 : embed type in ID #Patch Set 11 : cache #Patch Set 12 : always/never #Patch Set 13 : rebase #Patch Set 14 : swap #Patch Set 15 : tidy #Patch Set 16 : windows #Patch Set 17 : fix? #Patch Set 18 : another approach? #Patch Set 19 : cant be static #Patch Set 20 : function type? #Patch Set 21 : with hash fix #Patch Set 22 : rebase #Patch Set 23 : fmt and lookup #Patch Set 24 : tweaks for hal #Patch Set 25 : type asserts in Server #Patch Set 26 : fold assertions into map type #Patch Set 27 : AutoUndefine -> LookupScope #Patch Set 28 : protocol #Patch Set 29 : tweaks #
Total comments: 11
Patch Set 30 : hal #
Messages
Total messages: 79 (37 generated)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/150001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/170001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtklein@chromium.org changed reviewers: + halcanary@google.com - mtklein@google.com
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/270001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/290001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/310001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/330001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/350001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/370001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/390001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/410001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/430001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/490001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/510001
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/530001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/550001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/550001
OK, I feel pretty good about this at PS 29 as a first start. I think the key pieces are present, and will just need to be fleshed out. Want to take another look? If there're any design aspects still unclear, I'd be happy to go/meet/mtklein.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1391023005/diff/550001/dm/DMSrcSink.h File dm/DMSrcSink.h (left): https://codereview.chromium.org/1391023005/diff/550001/dm/DMSrcSink.h#oldcode307 dm/DMSrcSink.h:307: class ViaDeferred : public Via { Was this class never used? Was ViaDeferred::draw() declared but never defined? https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h File src/core/SkRemote.h (right): https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:19: struct Encoder { /** describe class */ https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:22: struct Misc { Should this be SkRemote::Misc rather than SkRemote::Encoder::Misc? https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:73: class LookupScope { /** comments explaining what this class does. */ https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:87: struct Cache { /** describe class */ https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:101: class Client final : public SkCanvas { /** describe class */ https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote_prot... File src/core/SkRemote_protocol.h (right): https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote_prot... src/core/SkRemote_protocol.h:31: uint64_t val() const { return fVal & ~((uint64_t)0xFF << 56); } given a type and a value, how does a decoder create an ID? (other than a union or memcpy).
The CQ bit was checked by mtklein@google.com to run a CQ dry run
https://codereview.chromium.org/1391023005/diff/550001/dm/DMSrcSink.h File dm/DMSrcSink.h (left): https://codereview.chromium.org/1391023005/diff/550001/dm/DMSrcSink.h#oldcode307 dm/DMSrcSink.h:307: class ViaDeferred : public Via { On 2015/10/16 16:57:54, Hal Canary wrote: > Was this class never used? > > Was ViaDeferred::draw() declared but never defined? Sort of. It's left over from when we were testing SkDeferredCanvas. We removed the definitions and use of ViaDeferred, but not its declaration. We could delete this in another CL if you like. https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h File src/core/SkRemote.h (right): https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:19: struct Encoder { On 2015/10/16 16:57:54, Hal Canary wrote: > /** describe class */ Left myself some TODOs. Not _quite_ confident enough about the design yet to write it up yet. https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote_prot... File src/core/SkRemote_protocol.h (right): https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote_prot... src/core/SkRemote_protocol.h:31: uint64_t val() const { return fVal & ~((uint64_t)0xFF << 56); } On 2015/10/16 16:57:55, Hal Canary wrote: > given a type and a value, how does a decoder create an ID? (other than a union > or memcpy). I had been imagining it couldn't, but good point, that's silly. I've added another constructor.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/570001
https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h File src/core/SkRemote.h (right): https://codereview.chromium.org/1391023005/diff/550001/src/core/SkRemote.h#ne... src/core/SkRemote.h:22: struct Misc { On 2015/10/16 16:57:55, Hal Canary wrote: > Should this be SkRemote::Misc rather than SkRemote::Encoder::Misc? Whoops, I seem to have clicked Cancel instead of Save on this one... Done. Was thinking this myself.
lgtm
The CQ bit was unchecked by mtklein@google.com
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023005/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023005/570001
Message was sent while issue was closed.
Committed patchset #30 (id:570001) as https://skia.googlesource.com/skia/+/2e2ea38bb77d42bab292919de5d80286cc354782 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
