Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152163004:
Remove unused PackBits methods and fix length checks (Closed)
DescriptionRemove unused PackBits methods and fix length checks
Also a bit of general cleanup.
BUG=chromium:486944
Committed: https://skia.googlesource.com/skia/+/699b852e48da8f71c19fcaa13bb109efd68e5c7d
Patch Set 1 #
Total comments: 6
Patch Set 2 : nits and compile fix #Patch Set 3 : Return 0 on error #
Messages
Total messages: 31 (11 generated)
The CQ bit was checked by jschuh@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/1152163004/1
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-05-23 01:31 UTC
jschuh@chromium.org changed reviewers: + mtklein@chromium.org
Not sure if this is posted to the right place, but PTAL.
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...)
mtklein@chromium.org changed reviewers: + reed@google.com
+reed, to get him to sign off on the removed public API. I'm, as usual, surprised this is public API to begin with. This lgtm except for a couple formatting nits I've marked. https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp File src/core/SkPackBits.cpp (right): https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... src/core/SkPackBits.cpp:40: return 0; We generally put {} around all ifs, even if one line. I'll see if I can mark them all. :) https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... src/core/SkPackBits.cpp:93: break; {} https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... src/core/SkPackBits.cpp:98: break; {}
On 2015/05/22 19:31:22, jschuh wrote: > Not sure if this is posted to the right place, but PTAL. (Yes, posted to the right place.)
The CQ bit was checked by jschuh@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/1152163004/#ps20001 (title: "nits and compile fix")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152163004/20001
Thanks. I'll wait for reed@'s response. https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp File src/core/SkPackBits.cpp (right): https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... src/core/SkPackBits.cpp:40: return 0; On 2015/05/22 19:37:53, mtklein_C wrote: > We generally put {} around all ifs, even if one line. I'll see if I can mark > them all. :) Done. https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... src/core/SkPackBits.cpp:93: break; On 2015/05/22 19:37:53, mtklein_C wrote: > {} Done. https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... src/core/SkPackBits.cpp:98: break; On 2015/05/22 19:37:53, mtklein_C wrote: > {} Done.
On 2015/05/22 20:02:15, jschuh wrote: > Thanks. I'll wait for reed@'s response. > > https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp > File src/core/SkPackBits.cpp (right): > > https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... > src/core/SkPackBits.cpp:40: return 0; > On 2015/05/22 19:37:53, mtklein_C wrote: > > We generally put {} around all ifs, even if one line. I'll see if I can mark > > them all. :) > > Done. > > https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... > src/core/SkPackBits.cpp:93: break; > On 2015/05/22 19:37:53, mtklein_C wrote: > > {} > > Done. > > https://codereview.chromium.org/1152163004/diff/1/src/core/SkPackBits.cpp#new... > src/core/SkPackBits.cpp:98: break; > On 2015/05/22 19:37:53, mtklein_C wrote: > > {} > > Done. sgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The Pack code could allow "snug" dst buffers, but checking as it compresses, rather than always requiring the worst-case dst-size. The Unpack code still returns however many bytes it decided to write, even in the case where dst was too small. This doesn't inform the caller that there was a problem. Perhaps the unpacker should return 0 or some other condition indicating dst was too small. I see that the Test was adjusted to pass in the dst-size, but are there tests that exercise the too-small-dst-size cases?
On 2015/05/26 13:20:59, reed1 wrote: > The Pack code could allow "snug" dst buffers, but checking as it compresses, > rather than always requiring the worst-case dst-size. Yeah, but the Given that the only callers are using fixed-size stack buffers I figured it wasn't worth the trouble. > The Unpack code still returns however many bytes it decided to write, even in > the case where dst was too small. This doesn't inform the caller that there was > a problem. Perhaps the unpacker should return 0 or some other condition > indicating dst was too small. Good point. Done. > I see that the Test was adjusted to pass in the dst-size, but are there tests > that exercise the too-small-dst-size cases? Yep, there's a test for too small of an output buffer for both Pack and Unpack.
The CQ bit was checked by jschuh@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/1152163004/#ps40001 (title: "Return 0 on error")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152163004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
On 2015/05/28 23:06:09, jschuh wrote: > ping ping
lgtm
The CQ bit was checked by jschuh@chromium.org
tks :)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152163004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/699b852e48da8f71c19fcaa13bb109efd68e5c7d |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
