Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
872683008:
Small cleanup on BMPImageReader.cpp (Closed)
DescriptionSmall cleanup on BMPImageReader.cpp
'm_headerOffset + m_infoHeader.biSize' is used often in BMPImageReader.cpp.
It is even recalulated several time in a same function. This patch cleans up
this kind of repeated code.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189471
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 6
Patch Set 3 : Updated patch #Messages
Total messages: 21 (7 generated)
shivamidow@gmail.com changed reviewers: + eae@chromium.org, noel@google.com
PTAL.
Seems ok to me. Peter might have suggestions here.
noel@chromium.org changed reviewers: + pkasting@google.com
Any other comments on this?
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Sorry, I didn't see this. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:178: const size_t headerAndInfoHeaderSize = sizeOfHeaderAndInfoHeader(); Nit: Call this |headerEnd|. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:426: static const size_t SIZEOF_BITMASKS = 12; Nit: While here, change to: const size_t bitmasksSize = 12; https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:428: const size_t headerInfoHeaderAndBitmasksSize = headerAndInfoHeaderSize + SIZEOF_BITMASKS; Nit: Call these |headerEnd| and |bitmasksEnd|. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:537: size_t headerInfoHeaderAndTableSize = headerAndInfoHeaderSize + tableSizeInBytes; Nit: Make all these const. Call the new variables |headerEnd| and |tableEnd|. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... File Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.h:267: inline size_t sizeOfHeaderAndInfoHeader() const Don't add this. Do it inline in the places you call it.
https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:178: const size_t headerAndInfoHeaderSize = sizeOfHeaderAndInfoHeader(); On 2015/02/03 23:56:43, Peter Kasting wrote: > Nit: Call this |headerEnd|. Done. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:426: static const size_t SIZEOF_BITMASKS = 12; On 2015/02/03 23:56:43, Peter Kasting wrote: > Nit: While here, change to: > > const size_t bitmasksSize = 12; Done. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:428: const size_t headerInfoHeaderAndBitmasksSize = headerAndInfoHeaderSize + SIZEOF_BITMASKS; On 2015/02/03 23:56:43, Peter Kasting wrote: > Nit: Call these |headerEnd| and |bitmasksEnd|. Done. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.cpp:537: size_t headerInfoHeaderAndTableSize = headerAndInfoHeaderSize + tableSizeInBytes; On 2015/02/03 23:56:43, Peter Kasting wrote: > Nit: Make all these const. Call the new variables |headerEnd| and |tableEnd|. Done. https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... File Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/872683008/diff/1/Source/platform/image-decode... Source/platform/image-decoders/bmp/BMPImageReader.h:267: inline size_t sizeOfHeaderAndInfoHeader() const On 2015/02/03 23:56:43, Peter Kasting wrote: > Don't add this. Do it inline in the places you call it. Done.
LGTM https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... Source/platform/image-decoders/bmp/BMPImageReader.cpp:427: const size_t bitmasksSize = 12; Tiny nit: Swap this line with the next https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... Source/platform/image-decoders/bmp/BMPImageReader.cpp:536: const size_t tableSizeInBytes = m_infoHeader.biClrUsed * (m_isOS21x ? 3 : 4); Tiny nit: Swap this line with the next https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... Source/platform/image-decoders/bmp/BMPImageReader.cpp:539: Nit: Eliminate this newline and move the comment below to above the lines above
Thanks! https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... Source/platform/image-decoders/bmp/BMPImageReader.cpp:427: const size_t bitmasksSize = 12; On 2015/02/04 04:59:02, Peter Kasting wrote: > Tiny nit: Swap this line with the next Done. https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... Source/platform/image-decoders/bmp/BMPImageReader.cpp:536: const size_t tableSizeInBytes = m_infoHeader.biClrUsed * (m_isOS21x ? 3 : 4); On 2015/02/04 04:59:01, Peter Kasting wrote: > Tiny nit: Swap this line with the next Done. https://codereview.chromium.org/872683008/diff/20001/Source/platform/image-de... Source/platform/image-decoders/bmp/BMPImageReader.cpp:539: On 2015/02/04 04:59:01, Peter Kasting wrote: > Nit: Eliminate this newline and move the comment below to above the lines above Done.
New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org
The CQ bit was checked by shivamidow@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/872683008/40001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41244)
On 2015/02/04 08:31:40, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41244) failures: http/tests/navigation/anchor-basic.html Really? mac_blink_rel seems broken. :/
On 2015/02/04 08:35:06, changseok wrote: > On 2015/02/04 08:31:40, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > mac_blink_rel on tryserver.blink > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41244) > > failures: > http/tests/navigation/anchor-basic.html > > Really? mac_blink_rel seems broken. :/ O.K The failed test is marked as an expected crash. https://codereview.chromium.org/895913005 Let me retry commit
The CQ bit was checked by shivamidow@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/872683008/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189471 |
