Reviewers: Lei Zhang CL: https://codereview.chromium.org/1709393002/ Message: PTAL, thanks Description: For CPDF_PageObject and its subclasses, remove m_Type and use GetType() instead. Also, add As<Type> functions to avoid casting all over the places. BUG=397 Base URL: https://pdfium.googlesource.com/pdfium.git@master Affected files (+111, -83 lines): M core/include/fpdfapi/fpdf_pageobj.h M core/src/fpdfapi/fpdf_edit/fpdf_edit_content.cpp M core/src/fpdfapi/fpdf_page/fpdf_page.cpp M core/src/fpdfapi/fpdf_page/fpdf_page_image.cpp M core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp M core/src/fpdfapi/fpdf_render/fpdf_render.cpp M core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp M core/src/fpdfapi/fpdf_render/fpdf_render_text.cpp M core/src/fpdfapi/fpdf_render/render_int.h M core/src/fpdftext/fpdf_text_int.cpp M fpdfsdk/src/fpdf_transformpage.cpp M fpdfsdk/src/fpdfeditpage.cpp M fpdfsdk/src/javascript/Document.cpp
lgtm https://codereview.chromium.org/1709393002/diff/20001/core/src/fpdfapi/fpdf_render/fpdf_render.cpp File core/src/fpdfapi/fpdf_render/fpdf_render.cpp (right): https://codereview.chromium.org/1709393002/diff/20001/core/src/fpdfapi/fpdf_render/fpdf_render.cpp#newcode325 core/src/fpdfapi/fpdf_render/fpdf_render.cpp:325: if (type != CPDF_PageObject::IMAGE) { Maybe just remove the type parameter and this check? The only caller on line 310 above behaves as though we never return NULL here. https://codereview.chromium.org/1709393002/diff/20001/core/src/fpdfapi/fpdf_render/fpdf_render.cpp#newcode379 core/src/fpdfapi/fpdf_render/fpdf_render.cpp:379: bRet = ProcessText((CPDF_TextObject*)pObj, pObj2Device, NULL); More casts to get rid of. https://codereview.chromium.org/1709393002/diff/20001/fpdfsdk/src/fpdfeditpage.cpp File fpdfsdk/src/fpdfeditpage.cpp (right): https://codereview.chromium.org/1709393002/diff/20001/fpdfsdk/src/fpdfeditpage.cpp#newcode145 fpdfsdk/src/fpdfeditpage.cpp:145: CPDF_PathObject* pPathObj = (CPDF_PathObject*)pPageObj; More casts. https://codereview.chromium.org/1709393002/
BTW, I changed the CL description. If one writes BUG=397, the Chromium code review tool assumes Chromium bug 397 by default. https://codereview.chromium.org/1709393002/
thanks! I also propagated the constness along the functions while making the change.
https://codereview.chromium.org/1709393002/diff/20001/core/src/fpdfapi/fpdf_render/fpdf_render.cpp File core/src/fpdfapi/fpdf_render/fpdf_render.cpp (right): https://codereview.chromium.org/1709393002/diff/20001/core/src/fpdfapi/fpdf_render/fpdf_render.cpp#newcode325 core/src/fpdfapi/fpdf_render/fpdf_render.cpp:325
: if (type !=
CPDF_PageObject::IMAGE) {
On 2016/02/19 04:22:31, Lei Zhang wrote:
> Maybe just remove the type parameter and this check? The only caller
on line 310
> above behaves as though we never return NULL here.
I removed it for now. Looks like IPDF_ObjectRenderer is only used by
ImageRender, maybe we could get rid of it all later.
https://codereview.chromium.org/1709393002/diff/20001/core/src/fpdfapi/fpdf_render/fpdf_render.cpp#newcode379
core/src/fpdfapi/fpdf_render/fpdf_render.cpp:379: bRet =
ProcessText((CPDF_TextObject*)pObj, pObj2Device, NULL);
On 2016/02/19 04:22:31, Lei Zhang wrote:
> More casts to get rid of.
Thanks for spotting these. Done.
: CPDF_PathObject* pPathObj = (CPDF_PathObject*)pPageObj; On 2016/02/19 04:22:31, Lei Zhang wrote: > More casts. Done. https://codereview.chromium.org/1709393002/
Committed patchset #2 (id:40001) manually as 7cf13c9c8b9b69b01e5debb5e8dc8b265983dfa8 (presubmit successful). https://codereview.chromium.org/1709393002/