Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196743002:
Add SkColorSpace::Equals() API (Closed)
DescriptionAdd SkColorSpace::Equals() API
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2196743002
Committed: https://skia.googlesource.com/skia/+/abbd6d5e02832d53a939be15b78de592d88fe9ec
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use malloc instead of stack for test #
Total comments: 10
Patch Set 3 : Response to comments #
Total comments: 2
Patch Set 4 : Add comments and test #Patch Set 5 : Fix logic #
Depends on Patchset: Messages
Total messages: 32 (15 generated)
Description was changed from ========== Add SkColorSpace::Equals() API BUG=skia: ========== to ========== Add SkColorSpace::Equals() API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2196743002 ==========
msarett@google.com changed reviewers: + brianosman@google.com, herb@google.com, mtklein@google.com, reed@google.com
https://codereview.chromium.org/2196743002/diff/1/tests/ImageIsOpaqueTest.cpp File tests/ImageIsOpaqueTest.cpp (right): https://codereview.chromium.org/2196743002/diff/1/tests/ImageIsOpaqueTest.cpp... tests/ImageIsOpaqueTest.cpp:22: int32_t storage[2000]; Is this going to annoy Google3?
https://codereview.chromium.org/2196743002/diff/1/tests/ImageIsOpaqueTest.cpp File tests/ImageIsOpaqueTest.cpp (right): https://codereview.chromium.org/2196743002/diff/1/tests/ImageIsOpaqueTest.cpp... tests/ImageIsOpaqueTest.cpp:22: int32_t storage[2000]; On 2016/07/29 15:31:27, msarett wrote: > Is this going to annoy Google3? Probably. This is a test... just malloc it.
https://codereview.chromium.org/2196743002/diff/1/tests/ImageIsOpaqueTest.cpp File tests/ImageIsOpaqueTest.cpp (right): https://codereview.chromium.org/2196743002/diff/1/tests/ImageIsOpaqueTest.cpp... tests/ImageIsOpaqueTest.cpp:22: int32_t storage[2000]; On 2016/07/29 15:32:12, mtklein wrote: > On 2016/07/29 15:31:27, msarett wrote: > > Is this going to annoy Google3? > > Probably. This is a test... just malloc it. SGTM. Done.
https://codereview.chromium.org/2196743002/diff/20001/include/core/SkColorSpa... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2196743002/diff/20001/include/core/SkColorSpa... include/core/SkColorSpace.h:107: static bool Equals(SkColorSpace* src, SkColorSpace* dst); any reason to not write const here? Are SkColorSpace immutable? https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:332: // were a match, we would have already returned. This seems fragile. Unless you've got a way to assert that this assumption is true, why not just check? https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:345: if (srcData->size() != dstData->size()) { Maybe, return srcData->size() == dstData->size() && 0 == memcmp(srcData->data(), dstData->data(), srcData()->size()); https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:361: // If |src| does not have a named gamma, fProfileData should be non-null. Does it make sense to just move this before the fProfileData section, and make this fall through? https://codereview.chromium.org/2196743002/diff/20001/tests/ImageIsOpaqueTest... File tests/ImageIsOpaqueTest.cpp (right): https://codereview.chromium.org/2196743002/diff/20001/tests/ImageIsOpaqueTest... tests/ImageIsOpaqueTest.cpp:24: SkAutoMalloc storage(storageBytes); If we need 4-byte aligned storage, we usually use SkAutoTMalloc<uint32_t> to make that clear. It happens this will be 8 or 16 byte aligned anyway, but that's pretty subtle.
https://codereview.chromium.org/2196743002/diff/20001/include/core/SkColorSpa... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2196743002/diff/20001/include/core/SkColorSpa... include/core/SkColorSpace.h:107: static bool Equals(SkColorSpace* src, SkColorSpace* dst); On 2016/07/29 15:43:48, mtklein wrote: > any reason to not write const here? Are SkColorSpace immutable? const SGTM. FWIW, they are immutable. https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:332: // were a match, we would have already returned. On 2016/07/29 15:43:48, mtklein wrote: > This seems fragile. Unless you've got a way to assert that this assumption is > true, why not just check? True, that also seems nicer to read, sgtm. https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:345: if (srcData->size() != dstData->size()) { On 2016/07/29 15:43:48, mtklein wrote: > Maybe, > > return srcData->size() == dstData->size() > && 0 == memcmp(srcData->data(), dstData->data(), srcData()->size()); > Done. https://codereview.chromium.org/2196743002/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:361: // If |src| does not have a named gamma, fProfileData should be non-null. On 2016/07/29 15:43:48, mtklein wrote: > Does it make sense to just move this before the fProfileData section, and make > this fall through? Can't do that because a kNonStandard_GammaNamed is not the only reason we might save the profile data (we also save it if there is a colorLUT). An earlier version of this CL actually checks for colorLUTs, but I thought this was a bit simpler (and also the old one had bugs). Maybe a comment would be helpful? One thing I like about this function is that if I continue to add "nonstandard" features, I think it should continue to work. fProfileData is a nice catch-all for weird profiles. reed@ actually suggested at one point that I just use serialize() to test equality if we don't hit the first couple fast cases. That also might make this simpler and easier to maintain. https://codereview.chromium.org/2196743002/diff/20001/tests/ImageIsOpaqueTest... File tests/ImageIsOpaqueTest.cpp (right): https://codereview.chromium.org/2196743002/diff/20001/tests/ImageIsOpaqueTest... tests/ImageIsOpaqueTest.cpp:24: SkAutoMalloc storage(storageBytes); On 2016/07/29 15:43:48, mtklein wrote: > If we need 4-byte aligned storage, we usually use SkAutoTMalloc<uint32_t> to > make that clear. It happens this will be 8 or 16 byte aligned anyway, but > that's pretty subtle. Done.
lgtm
https://codereview.chromium.org/2196743002/diff/40001/include/core/SkColorSpa... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2196743002/diff/40001/include/core/SkColorSpa... include/core/SkColorSpace.h:107: static bool Equals(const SkColorSpace* src, const SkColorSpace* dst); wonder if it should take const& for each, since we don't support null for either.
Patchset #4 (id:60001) has been deleted
I've uploaded a version that uses const&. I'm not sure I like it - it just means I now have to add null checks to the call sites. But this version may make more sense looking forward to when legacy mode is no longer supported.
On 2016/08/01 12:39:12, msarett wrote: > I've uploaded a version that uses const&. I'm not sure I > like it - it just means I now have to add null checks to the > call sites. But this version may make more sense looking > forward to when legacy mode is no longer supported. I myself prefer the (const*, const*) with the null checks on the inside.
Good case for using pointers (if callers themselves don't know if they have null or not).
On 2016/08/01 13:34:47, reed1 wrote: > (if callers themselves don't know if they have null or not). exactly, yeah
Patchset #4 (id:80001) has been deleted
On 2016/08/01 13:36:21, mtklein wrote: > On 2016/08/01 13:34:47, reed1 wrote: > > (if callers themselves don't know if they have null or not). > > exactly, yeah Deleted Path Set 4, back to the pointer version.
lgtm w/ suggestion for dox https://codereview.chromium.org/2196743002/diff/40001/include/core/SkColorSpa... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2196743002/diff/40001/include/core/SkColorSpa... include/core/SkColorSpace.h:107: static bool Equals(const SkColorSpace* src, const SkColorSpace* dst); Perhaps add dox to describe what we do when we see a null (e.g. if one is null and one is not, we return false, if both are null, we return true, else we do a deeper compare)
maybe add null variants to unittest...
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/01 15:08:54, reed1 wrote: > maybe add null variants to unittest... Added comments and test
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.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 master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2196743002/#ps120001 (title: "Fix logic")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add SkColorSpace::Equals() API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2196743002 ========== to ========== Add SkColorSpace::Equals() API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2196743002 Committed: https://skia.googlesource.com/skia/+/abbd6d5e02832d53a939be15b78de592d88fe9ec ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://skia.googlesource.com/skia/+/abbd6d5e02832d53a939be15b78de592d88fe9ec |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
