Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1307023004:
change colortable to use factory for reinflating, check for empty (Closed)
Descriptionchange colortable to use factory for reinflating, check for empty
BUG=525763
Committed: https://skia.googlesource.com/skia/+/b236d1a37bd8ee936587faa0a55128a1f901cb5a
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : doh #
Total comments: 1
Messages
Total messages: 26 (11 generated)
reed@google.com changed reviewers: + robertphillips@google.com
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/1307023004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307023004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1307023004/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/1307023004/diff/1/src/core/SkBitmap.cpp#newco... src/core/SkBitmap.cpp:1152: // only permit an empty table if we have no pixels seems some guard is missing here https://codereview.chromium.org/1307023004/diff/1/src/core/SkColorTable.cpp File src/core/SkColorTable.cpp (right): https://codereview.chromium.org/1307023004/diff/1/src/core/SkColorTable.cpp#n... src/core/SkColorTable.cpp:73: rm this ? https://codereview.chromium.org/1307023004/diff/1/src/core/SkColorTable.cpp#n... src/core/SkColorTable.cpp:115: We don't need a "buffer.validateAvailable(allocSize)" call here?
https://codereview.chromium.org/1307023004/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/1307023004/diff/1/src/core/SkBitmap.cpp#newco... src/core/SkBitmap.cpp:1152: // only permit an empty table if we have no pixels On 2015/08/28 15:58:53, robertphillips wrote: > seems some guard is missing here Done. https://codereview.chromium.org/1307023004/diff/1/src/core/SkColorTable.cpp File src/core/SkColorTable.cpp (right): https://codereview.chromium.org/1307023004/diff/1/src/core/SkColorTable.cpp#n... src/core/SkColorTable.cpp:73: On 2015/08/28 15:58:53, robertphillips wrote: > rm this ? just-in-case some (legacy) client needs this, we can just re-expose it in the header (temporarily). After this lands in chrome and android, will remove this. https://codereview.chromium.org/1307023004/diff/1/src/core/SkColorTable.cpp#n... src/core/SkColorTable.cpp:115: On 2015/08/28 15:58:53, robertphillips wrote: > We don't need a "buffer.validateAvailable(allocSize)" call here? readColorArray checks for the same thing.
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/1307023004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307023004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm + micro-nits https://codereview.chromium.org/1307023004/diff/20001/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/1307023004/diff/20001/src/core/SkBitmap.cpp#n... src/core/SkBitmap.cpp:1154: // require an empty ctable '!=' instead of '!'...'==' ? https://codereview.chromium.org/1307023004/diff/20001/src/core/SkBitmap.cpp#n... src/core/SkBitmap.cpp:1159: // require a non-empty ctable '<=' instead of '!'...'>' ?
https://codereview.chromium.org/1307023004/diff/20001/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/1307023004/diff/20001/src/core/SkBitmap.cpp#n... src/core/SkBitmap.cpp:1154: // require an empty ctable On 2015/08/28 16:31:43, robertphillips wrote: > '!=' instead of '!'...'==' ? Done. https://codereview.chromium.org/1307023004/diff/20001/src/core/SkBitmap.cpp#n... src/core/SkBitmap.cpp:1159: // require a non-empty ctable On 2015/08/28 16:31:43, robertphillips wrote: > '<=' instead of '!'...'>' ? Done.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1307023004/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307023004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307023004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) 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
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1307023004/#ps60001 (title: "doh")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307023004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307023004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/b236d1a37bd8ee936587faa0a55128a1f901cb5a
Message was sent while issue was closed.
halcanary@google.com changed reviewers: + halcanary@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1307023004/diff/60001/src/core/SkColorTable.cpp File src/core/SkColorTable.cpp (right): https://codereview.chromium.org/1307023004/diff/60001/src/core/SkColorTable.c... src/core/SkColorTable.cpp:39: SkASSERT(count > 0 && count <= 255); why not 256?
Message was sent while issue was closed.
On 2015/08/28 18:28:03, Hal Canary wrote: > https://codereview.chromium.org/1307023004/diff/60001/src/core/SkColorTable.cpp > File src/core/SkColorTable.cpp (right): > > https://codereview.chromium.org/1307023004/diff/60001/src/core/SkColorTable.c... > src/core/SkColorTable.cpp:39: SkASSERT(count > 0 && count <= 255); > why not 256? DOH! you're right |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
