Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892403003:
Simplify image decoder selection. (Closed)
DescriptionSimplify image decoder selection.
Simplify the image decoder selection by removing SniffResult.
This avoids two levels of condition check to select image decoders.
BUG=None
Review-Url: https://codereview.chromium.org/2892403003
Cr-Commit-Position: refs/heads/master@{#475446}
Committed: https://chromium.googlesource.com/chromium/src/+/aa263e32cadf984c2c77ea31a45d7bf39f14d8fc
Patch Set 1 #
Total comments: 3
Patch Set 2 : y #
Total comments: 5
Patch Set 3 : y #Patch Set 4 : y #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== Simplify image decoder selection. Simplify the image decoder selection by removing SniffResult. This avoids two levels of condition check to select image decoders. BUG=None ========== to ========== Simplify image decoder selection. Simplify the image decoder selection by removing SniffResult. This avoids two levels of condition check to select image decoders. BUG=None R=SamsungPeerReview ==========
nagarajan.n@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:88: const char* contents = DetermineImageType is Dchecking the length, but this CL is not checking anything. DCHECK_GE(length, kLongestSignatureLength);
On 2017/05/22 09:57:30, Shanmuga Pandi wrote: > https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): > > https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:88: const > char* contents = > DetermineImageType is Dchecking the length, but this CL is not checking > anything. > DCHECK_GE(length, kLongestSignatureLength); Looks like the DCHECK is redundant as we are passing KLongestSignatureLength itself. Changes look fine but, let the module owner give opinion whether to go with this change or to keep the helper function as it is.
https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:88: const char* contents = On 2017/05/22 09:57:30, Shanmuga Pandi wrote: > DetermineImageType is Dchecking the length, but this CL is not checking > anything. > DCHECK_GE(length, kLongestSignatureLength); Passing and Dchecking the same with kLongestSignatureLength can be avoided. Ignore my previous comment.
https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2892403003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:88: const char* contents = On 2017/05/22 09:57:30, Shanmuga Pandi wrote: > DetermineImageType is Dchecking the length, but this CL is not checking > anything. > DCHECK_GE(length, kLongestSignatureLength); It's not required. This condition already is there in this function and the length we are passing is kLongestSignatureLength
Description was changed from ========== Simplify image decoder selection. Simplify the image decoder selection by removing SniffResult. This avoids two levels of condition check to select image decoders. BUG=None R=SamsungPeerReview ========== to ========== Simplify image decoder selection. Simplify the image decoder selection by removing SniffResult. This avoids two levels of condition check to select image decoders. BUG=None ==========
nagarajan.n@samsung.com changed reviewers: + scroggo@chromium.org
nagarajan.n@samsung.com changed reviewers: + noel@chromium.org
nagarajan.n@samsung.com changed reviewers: + pkasting@chromium.org
Digging into the history a bit here: https://codereview.chromium.org/2173873003 introduced the SniffResult, to distinguish between an invalid image and not enough data. In theory, a client could call determineImageType and handle kInvalid different from kInsufficient, but only one client did. https://codereview.chromium.org/2257513002/ moved the SniffResult to private, and essentially accomplished the same goal by calling ImageDecoder::hasSufficientDataToSniffImageType. So I agree that there's no point to the SniffResult anymore. LGTM
LGTM - with nits https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:66: // This needs to be updated if we ever add a Matches*Signature() which requires Remove this comment. It is a redundant statement of the obvious. https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:75: // We need at least kLongestSignatureLength bytes to run the signature // At least kLongestSignatureLength bytes are needed to sniff the signature.
I will update the patch based on review comments https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:66: // This needs to be updated if we ever add a Matches*Signature() which requires On 2017/05/27 03:51:09, noel gordon wrote: > Remove this comment. It is a redundant statement of the obvious. No need to remove this comment. We need to update the length when we add new signature in future https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:75: // We need at least kLongestSignatureLength bytes to run the signature On 2017/05/27 03:51:09, noel gordon wrote: > // At least kLongestSignatureLength bytes are needed to sniff the signature. I will update the patch based on the above comment
https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:66: // This needs to be updated if we ever add a Matches*Signature() which requires On 2017/05/29 05:39:56, naga wrote: > On 2017/05/27 03:51:09, noel gordon wrote: > > Remove this comment. It is a redundant statement of the obvious. > > No need to remove this comment. We need to update the length when we add new > signature in future Are you planning on adding a new image decoding signature to the Web?
On 2017/05/29 05:43:51, noel gordon wrote: > https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): > > https://codereview.chromium.org/2892403003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:66: // This > needs to be updated if we ever add a Matches*Signature() which requires > On 2017/05/29 05:39:56, naga wrote: > > On 2017/05/27 03:51:09, noel gordon wrote: > > > Remove this comment. It is a redundant statement of the obvious. > > > > No need to remove this comment. We need to update the length when we add new > > signature in future > > Are you planning on adding a new image decoding signature to the Web? No. But future someone may add. If you feel not required than I will remove it
On 2017/05/29 05:46:58, naga wrote: > On 2017/05/29 05:43:51, noel gordon wrote: > > > > Are you planning on adding a new image decoding signature to the Web? > > No. But future someone may add. If you feel not required than I will remove it. Not really required. Any new image format for the Web (that happens rarely) would require layout tests. Those tests would sort out any data sniffing problems.
On 2017/05/29 06:48:39, noel gordon wrote: > On 2017/05/29 05:46:58, naga wrote: > > On 2017/05/29 05:43:51, noel gordon wrote: > > > > > > Are you planning on adding a new image decoding signature to the Web? > > > > No. But future someone may add. If you feel not required than I will remove > it. > > Not really required. Any new image format for the Web (that happens rarely) > would require layout tests. Those tests would sort out any data sniffing > problems. Ok.
The CQ bit was checked by nagarajan.n@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, noel@chromium.org Link to the patchset: https://codereview.chromium.org/2892403003/#ps60001 (title: "y")
The CQ bit was unchecked by nagarajan.n@samsung.com
The CQ bit was checked by nagarajan.n@samsung.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496119841543330,
"parent_rev": "1bf5669767a63f7e1d851c953e949f9e740aaf64", "commit_rev":
"aa263e32cadf984c2c77ea31a45d7bf39f14d8fc"}
Message was sent while issue was closed.
Description was changed from ========== Simplify image decoder selection. Simplify the image decoder selection by removing SniffResult. This avoids two levels of condition check to select image decoders. BUG=None ========== to ========== Simplify image decoder selection. Simplify the image decoder selection by removing SniffResult. This avoids two levels of condition check to select image decoders. BUG=None Review-Url: https://codereview.chromium.org/2892403003 Cr-Commit-Position: refs/heads/master@{#475446} Committed: https://chromium.googlesource.com/chromium/src/+/aa263e32cadf984c2c77ea31a45d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/aa263e32cadf984c2c77ea31a45d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
