|
|
DescriptionCleanup CPDF_DeviceCS and friends.
Committed: https://pdfium.googlesource.com/pdfium/+/28c888b8ee6d1da93cffaa8c14833ddc4986cc79
Patch Set 1 #
Total comments: 6
Patch Set 2 : nit #
Messages
Total messages: 21 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by thestig@chromium.org 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...
thestig@chromium.org changed reviewers: + tsepez@chromium.org
I started looking at bug 650230 and bug 603518 and figured I'd clean up the code first.
https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... File core/fpdfapi/page/fpdf_page_colors.cpp (left): https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... core/fpdfapi/page/fpdf_page_colors.cpp:115: ASSERT(m_Family == PDFCS_PATTERN); Looks like we lost this case? Also we hit asserts in a few other places now if this is the case. https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... File core/fpdfapi/page/fpdf_page_colors.cpp (right): https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... core/fpdfapi/page/fpdf_page_colors.cpp:25: if (family == PDFCS_DEVICERGB) You wouldn't want to make PDFCS_* enums while we're at it? https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... core/fpdfapi/page/fpdf_page_colors.cpp:89: G = B = R; nit: one per line.
https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... File core/fpdfapi/page/fpdf_page_colors.cpp (left): https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... core/fpdfapi/page/fpdf_page_colors.cpp:115: ASSERT(m_Family == PDFCS_PATTERN); On 2016/11/08 20:29:25, Tom Sepez wrote: > Looks like we lost this case? Also we hit asserts in a few other places now if > this is the case. This is never reached. There are only 3 instantiations of CPDF_DeviceCS in core/fpdfapi/page/cpdf_pagemodule.h and they are the other 3 cases. https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... File core/fpdfapi/page/fpdf_page_colors.cpp (right): https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... core/fpdfapi/page/fpdf_page_colors.cpp:25: if (family == PDFCS_DEVICERGB) On 2016/11/08 20:29:25, Tom Sepez wrote: > You wouldn't want to make PDFCS_* enums while we're at it? No, not today. https://codereview.chromium.org/2482273002/diff/40001/core/fpdfapi/page/fpdf_... core/fpdfapi/page/fpdf_page_colors.cpp:89: G = B = R; On 2016/11/08 20:29:25, Tom Sepez wrote: > nit: one per line. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Did you intend to upload another patch set?
On 2016/11/08 21:55:00, Tom Sepez wrote: > Did you intend to upload another patch set? Whoops, forgot to upload one for the nit.
The CQ bit was checked by thestig@chromium.org 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...
lgtm
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 thestig@chromium.org
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 ========== Cleanup CPDF_DeviceCS and friends. ========== to ========== Cleanup CPDF_DeviceCS and friends. Committed: https://pdfium.googlesource.com/pdfium/+/28c888b8ee6d1da93cffaa8c14833ddc4986... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://pdfium.googlesource.com/pdfium/+/28c888b8ee6d1da93cffaa8c14833ddc4986... |