Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1535993003:
hide old SaveFlags (Closed)
Descriptionhide old SaveFlags, but keep them available (for now) for Android
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1535993003
requires https://codereview.chromium.org/1537203002/# to land in chrome first (android should be safe)
Committed: https://skia.googlesource.com/skia/+/bada1885da479d948f065182d6dfa85a1140bda5
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : address comments #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : expose dontcliptolayer for android #Patch Set 7 : move private enum earlier in the class #Patch Set 8 : #Patch Set 9 : aaaaaargh #
Messages
Total messages: 55 (25 generated)
Description was changed from ========== hide old SaveFlags BUG=skia: ========== to ========== hide old SaveFlags BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by reed@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/1535993003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/1
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-Arm7-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 reed@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/1535993003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== hide old SaveFlags BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hide old SaveFlags, but keep them available (for now) for Android BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
reed@google.com changed reviewers: + fmalita@chromium.org, scroggo@google.com
Description was changed from ========== hide old SaveFlags, but keep them available (for now) for Android BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hide old SaveFlags, but keep them available (for now) for Android BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... requires https://codereview.chromium.org/1537203002/# to land in chrome first (android should be safe) ==========
https://codereview.chromium.org/1535993003/diff/20001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1535993003/diff/20001/include/core/SkCanvas.h... include/core/SkCanvas.h:41: #define SK_SUPPORT_LEGACY_SAVEFLAGS Any danger to define multiple times (do we need a #ifndef guard)? https://codereview.chromium.org/1535993003/diff/20001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1535993003/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1277: SkPaint* paintPtr = nullptr; SkLazyPaint? https://codereview.chromium.org/1535993003/diff/20001/src/core/SkPicturePlayb... File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/1535993003/diff/20001/src/core/SkPicturePlayb... src/core/SkPicturePlayback.cpp:19: // matches old SkCanvas::SaveFlags Can we static_assert this (while SK_SUPPORT_LEGACY_SAVEFLAGS is still around)?
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535993003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/20001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-12-20 08:39 UTC
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/1535993003/diff/20001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1535993003/diff/20001/include/core/SkCanvas.h... include/core/SkCanvas.h:41: #define SK_SUPPORT_LEGACY_SAVEFLAGS On 2015/12/19 18:20:21, f(malita) wrote: > Any danger to define multiple times (do we need a #ifndef guard)? Done. https://codereview.chromium.org/1535993003/diff/20001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1535993003/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1277: SkPaint* paintPtr = nullptr; On 2015/12/19 18:20:21, f(malita) wrote: > SkLazyPaint? Restoring previous version of that idea. https://codereview.chromium.org/1535993003/diff/20001/src/core/SkPicturePlayb... File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/1535993003/diff/20001/src/core/SkPicturePlayb... src/core/SkPicturePlayback.cpp:19: // matches old SkCanvas::SaveFlags On 2015/12/19 18:20:21, f(malita) wrote: > Can we static_assert this (while SK_SUPPORT_LEGACY_SAVEFLAGS is still around)? Done.
The CQ bit was checked by reed@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/1535993003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/40001
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-Arm7-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 reed@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/1535993003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/60001
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-Arm7-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 reed@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/1535993003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mike@reedtribe.org changed reviewers: + mike@reedtribe.org
It will be fun to see if we can remove the SaveFlag values entirely from SkCanvas.h, and only have them in a separate header in Android. Something to try perhaps after this lands. (chrome side has already landed)
ptal
LGTM On 2015/12/20 11:54:26, mikerreed wrote: > It will be fun to see if we can remove the SaveFlag values entirely from > SkCanvas.h, and only have them in a separate header in Android. Something to try > perhaps after this lands. (chrome side has already landed) One thing that seems to be missing is a way to express SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG + kClipToLayer_SaveFlag using the new SaveLayerFlags API.
On 2015/12/21 13:41:36, f(malita) wrote: > LGTM > > On 2015/12/20 11:54:26, mikerreed wrote: > > It will be fun to see if we can remove the SaveFlag values entirely from > > SkCanvas.h, and only have them in a separate header in Android. Something to > try > > perhaps after this lands. (chrome side has already landed) > > One thing that seems to be missing is a way to express > SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG + kClipToLayer_SaveFlag using the new > SaveLayerFlags API. Hmmm, I see. If we expose that (even behind a guard) then we could remove the legacy enum entirely. Good idea.
On 2015/12/21 14:43:48, reed1 wrote: > On 2015/12/21 13:41:36, f(malita) wrote: > > LGTM > > > > On 2015/12/20 11:54:26, mikerreed wrote: > > > It will be fun to see if we can remove the SaveFlag values entirely from > > > SkCanvas.h, and only have them in a separate header in Android. Something to > > try > > > perhaps after this lands. (chrome side has already landed) > > > > One thing that seems to be missing is a way to express > > SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG + kClipToLayer_SaveFlag using the new > > SaveLayerFlags API. > > Hmmm, I see. If we expose that (even behind a guard) then we could remove the > legacy enum entirely. Good idea. Just uploaded an Android CL to minimize reliance on SkCanvas::SaveFlags. Should be trivial to switch to SaveLayerRec when it gets kClipToLayer_SaveFlag semantics.
The CQ bit was checked by reed@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/1535993003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/100001
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-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
The CQ bit was checked by reed@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/1535993003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/120001
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-Arm7-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 reed@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/1535993003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/160001
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM++
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535993003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535993003/160001
Message was sent while issue was closed.
Description was changed from ========== hide old SaveFlags, but keep them available (for now) for Android BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... requires https://codereview.chromium.org/1537203002/# to land in chrome first (android should be safe) ========== to ========== hide old SaveFlags, but keep them available (for now) for Android BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... requires https://codereview.chromium.org/1537203002/# to land in chrome first (android should be safe) Committed: https://skia.googlesource.com/skia/+/bada1885da479d948f065182d6dfa85a1140bda5 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/bada1885da479d948f065182d6dfa85a1140bda5 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
