Remove PageObject's m_Type and add As<Type> functions (issue 1709393002 by weili@chromium.org)

0 views
Skip to first unread message

we...@chromium.org

unread,
Feb 18, 2016, 10:38:24 PM2/18/16
to the...@chromium.org, pdfium-...@googlegroups.com
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


the...@chromium.org

unread,
Feb 18, 2016, 11:22:31 PM2/18/16
to we...@chromium.org, pdfium-...@googlegroups.com
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/

the...@chromium.org

unread,
Feb 18, 2016, 11:25:15 PM2/18/16
to we...@chromium.org, pdfium-...@googlegroups.com
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/

we...@chromium.org

unread,
Feb 19, 2016, 2:32:19 PM2/19/16
to the...@chromium.org, pdfium-...@googlegroups.com
thanks! I also propagated the constness along the functions while making the
change.  


: 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/

the...@chromium.org

unread,
Feb 19, 2016, 2:37:49 PM2/19/16
to we...@chromium.org, pdfium-...@googlegroups.com

weili@chromium.org via codereview.chromium.org

unread,
Feb 19, 2016, 2:53:07 PM2/19/16
to the...@chromium.org, we...@chromium.org, pdfium-...@googlegroups.com
Committed patchset #2 (id:40001) manually as
7cf13c9c8b9b69b01e5debb5e8dc8b265983dfa8 (presubmit successful).

https://codereview.chromium.org/1709393002/
Reply all
Reply to author
Forward
0 new messages