Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289783002:
dedup images/blobs/pictures by ID in old serialization format (Closed)
Descriptiondedup images/blobs/pictures by ID in old serialization format
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2289783002
Committed: https://skia.googlesource.com/skia/+/22b2af1dc42c0b95fe93b9a951313efe438e54cd
Patch Set 1 #
Total comments: 4
Messages
Total messages: 19 (7 generated)
Description was changed from ========== dedup images/blobs/pictures in old serialization format BUG=skia: ========== to ========== dedup images/blobs/pictures in old serialization format BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2289783002 ==========
reed@google.com changed reviewers: + bsalomon@google.com, herb@google.com, mtklein@google.com
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 2016-08-29 20:27 UTC
This actually only adds deduping for textblobs, right? The other two appear to already be deduped. https://codereview.chromium.org/2289783002/diff/1/src/core/SkPictureRecord.cpp File src/core/SkPictureRecord.cpp (right): https://codereview.chromium.org/2289783002/diff/1/src/core/SkPictureRecord.cp... src/core/SkPictureRecord.cpp:875: index = array.count(); Seems like we could return count instead of -1, so this becomes if (index == array.count()) { *array.append() = SkRef(obj); } return index;
lgtm
https://codereview.chromium.org/2289783002/diff/1/include/private/SkTDArray.h File include/private/SkTDArray.h (right): https://codereview.chromium.org/2289783002/diff/1/include/private/SkTDArray.h... include/private/SkTDArray.h:220: template <typename S> int select(S&& selector) const { Select in many other languages usually returns ALL selectable items instead of just one. Just a nit. https://codereview.chromium.org/2289783002/diff/1/include/private/SkTDArray.h... include/private/SkTDArray.h:229: return -1; I agree with Mike. I think that count is a better failure mark than -1.
On 2016/08/29 14:34:46, mtklein wrote: > This actually only adds deduping for textblobs, right? The other two appear to > already be deduped. Good (subtle) point. They are deduped, but by ptr instead of uniqueID. The real motivation here was to dedup based on uniqueID, since we think we're seeing different image-ptrs from android but the same ID. > > https://codereview.chromium.org/2289783002/diff/1/src/core/SkPictureRecord.cpp > File src/core/SkPictureRecord.cpp (right): > > https://codereview.chromium.org/2289783002/diff/1/src/core/SkPictureRecord.cp... > src/core/SkPictureRecord.cpp:875: index = array.count(); > Seems like we could return count instead of -1, so this becomes > > if (index == array.count()) { > *array.append() = SkRef(obj); > } > return index;
Description was changed from ========== dedup images/blobs/pictures in old serialization format BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2289783002 ========== to ========== dedup images/blobs/pictures by ID in old serialization format BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2289783002 ==========
https://codereview.chromium.org/2289783002/diff/1/src/core/SkPictureRecord.cpp File src/core/SkPictureRecord.cpp (right): https://codereview.chromium.org/2289783002/diff/1/src/core/SkPictureRecord.cp... src/core/SkPictureRecord.cpp:875: index = array.count(); On 2016/08/29 14:34:46, mtklein wrote: > Seems like we could return count instead of -1, so this becomes > > if (index == array.count()) { > *array.append() = SkRef(obj); > } > return index; That would make this site a tiny bit simpler, but I was following the previous convention for find() which returns negative on failure. For me this makes callers slightly simpler in general, since they don't need to always query count() to know if it failed. If we decide to return count() instead, I'd vote for also changing find()'s convention (and updating its callers) to keep find and select consistent.
The CQ bit was unchecked by reed@google.com
ah! lgtm
The CQ bit was checked by reed@google.com
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 ========== dedup images/blobs/pictures by ID in old serialization format BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2289783002 ========== to ========== dedup images/blobs/pictures by ID in old serialization format BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2289783002 Committed: https://skia.googlesource.com/skia/+/22b2af1dc42c0b95fe93b9a951313efe438e54cd ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/22b2af1dc42c0b95fe93b9a951313efe438e54cd
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
