Simplify PDFium by removing code that's not used in the open source repo. (issue 801913002 by jam@chromium.org)

[j...@chromium.org]

Reviewers: bo_xu1, Tom Sepez,


https://codereview.chromium.org/801913002/diff/1/core/src/fxge/ge/fx_ge_linux.cpp
File core/src/fxge/ge/fx_ge_linux.cpp (left):

https://codereview.chromium.org/801913002/diff/1/core/src/fxge/ge/fx_ge_linux.cpp#oldcode11
core/src/fxge/ge/fx_ge_linux.cpp:11: #if (_FXM_PLATFORM_ ==
_FXM_PLATFORM_APPLE_ && (!defined(_FPDFAPI_MINI_)))
this code is basically platform == linux && platform == mac which will
never be hit

https://codereview.chromium.org/801913002/diff/1/fpdfsdk/src/fpdfoom.cpp
File fpdfsdk/src/fpdfoom.cpp (left):

https://codereview.chromium.org/801913002/diff/1/fpdfsdk/src/fpdfoom.cpp#oldcode22
fpdfsdk/src/fpdfoom.cpp:22:
FXMEM_SetOOMHandler(FXMEM_GetDefaultMgr(),OOM_Handler,oomInfo);
this method isn't defined anymore.

Tom: this was added for security reasons (see the caller in chromium
code which exits the process on OOM). I'm guessing we don't need this
anymore now that we don't use a custom memory allocator?

Description:
Simplify PDFium by removing code that's not used in the open source repo.

-remove parameter from FPDF_InitLibrary
-

Please review this at https://codereview.chromium.org/801913002/

Base URL: https://pdfium.googlesource.com/pdfium.git@master

Affected files (+61, -1942 lines):
M BUILD.gn
M core/include/fpdfapi/fpdf_module.h
M core/src/fpdfapi/fpdf_basic_module.cpp
M core/src/fpdfapi/fpdf_font/font_int.h
M core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp
M core/src/fpdfapi/fpdf_font/fpdf_font_utility.cpp
M core/src/fpdfapi/fpdf_page/fpdf_page_image.cpp
M core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp
D core/src/fpdfapi/fpdf_page/fpdf_page_parser_new.cpp
M core/src/fpdfapi/fpdf_page/pageint.h
M core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
M core/src/fpdfapi/fpdf_render/fpdf_render.cpp
M core/src/fpdfapi/fpdf_render/fpdf_render_cache.cpp
M core/src/fpdfapi/fpdf_render/fpdf_render_image.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.cpp
M core/src/fpdftext/fpdf_text_int.cpp
M core/src/fxcodec/libjpeg/fpdfapi_jerror.c
M core/src/fxcodec/libjpeg/jmemsys.h
M core/src/fxge/agg/agg23/fx_agg_driver.cpp
M core/src/fxge/apple/fx_apple_platform.cpp
M core/src/fxge/apple/fx_mac_imp.cpp
M core/src/fxge/dib/fx_dib_engine.cpp
M core/src/fxge/dib/fx_dib_main.cpp
M core/src/fxge/fx_freetype/fxft2.5.01/src/psnames/fxft_psmodule.c
M core/src/fxge/ge/fx_ge_font.cpp
M core/src/fxge/ge/fx_ge_fontmap.cpp
D core/src/fxge/ge/fx_ge_linux.cpp
M core/src/fxge/ge/fx_ge_text.cpp
M core/src/fxge/win32/fx_win32_device.cpp
D fpdfsdk/include/fpdfoom.h
M fpdfsdk/include/fpdfview.h
M fpdfsdk/include/fsdk_define.h
M fpdfsdk/src/fpdfeditpage.cpp
D fpdfsdk/src/fpdfoom.cpp
M fpdfsdk/src/fpdfppo.cpp
M fpdfsdk/src/fpdfview.cpp
M fpdfsdk/src/javascript/Document.cpp
M fpdfsdk/src/javascript/app.cpp
M fpdfsdk/src/jsapi/fxjs_v8.cpp
M fpdfsdk/src/pdfwindow/PWL_FontMap.cpp
M pdfium.gyp
M samples/pdfium_test.cc

[tse...@chromium.org]

I'm not sure its worth doing this cleanup against the origin/master branch,
when
we hope to replace it with the code currently in origin/xfa. Last time I
put
together such a cleanup, the response was "but this is used in XFA" for a
number
of them. Do you know if that's the case here?

https://codereview.chromium.org/801913002/

[j...@chromium.org]

On 2014/12/12 22:23:04, Tom Sepez wrote:
> I'm not sure its worth doing this cleanup against the origin/master
> branch,
when
> we hope to replace it with the code currently in origin/xfa. Last time I
> put
> together such a cleanup, the response was "but this is used in XFA" for a
number
> of them. Do you know if that's the case here?

I thought that the xfa branch only touched a small number of the existing
files,
and it was mostly new files?

do you have a pointer to your old cl so I can see what was changed in it?

all the code i removed is stuff we wouldn't use in chrome (i.e. different
memory
manager, "mini" mode, non-pdfium code).

https://codereview.chromium.org/801913002/

[tse...@chromium.org]

I'd keep the memory stuff as-is. I'm not sure we want to tie this to a
specific
allocator behaviour.


https://codereview.chromium.org/801913002/diff/1/core/src/fxge/agg/agg23/fx_agg_driver.cpp
File core/src/fxge/agg/agg23/fx_agg_driver.cpp (right):

https://codereview.chromium.org/801913002/diff/1/core/src/fxge/agg/agg23/fx_agg_driver.cpp#newcode217
core/src/fxge/agg/agg23/fx_agg_driver.cpp:217: #if _FXM_PLATFORM_ !=
_FXM_PLATFORM_APPLE_
nit: extra space before !=

https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc#newcode381
samples/pdfium_test.cc:381: FPDF_InitLibrary();
OK for now, but interestingly, we're probably going to have to modify
this function to accept some js-specific arguments now that JS can't
find its own code anymore ( see
https://code.google.com/p/chromium/issues/detail?id=439793 ).

https://codereview.chromium.org/801913002/

[j...@chromium.org]

On 2014/12/12 22:36:12, Tom Sepez wrote:
> I'd keep the memory stuff as-is. I'm not sure we want to tie this to a
specific
> allocator behaviour.

which memory stuff are you talking about? not the
USE_MSDOS_MEMMGR/USE_MAC_MEMMGR i presume which aren't used?

https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc#newcode381
samples/pdfium_test.cc:381: FPDF_InitLibrary();

On 2014/12/12 22:36:12, Tom Sepez wrote:
> OK for now, but interestingly, we're probably going to have to modify
this
> function to accept some js-specific arguments now that JS can't find
its own
> code anymore ( see
https://code.google.com/p/chromium/issues/detail?id=439793 ).

i'm curious, why do that instead of initializing v8 directly from
src/pdf? i.e. third_party/pdfium already doesn't initialize v8 itself,
and depends on the wrapper code to do so.

https://codereview.chromium.org/801913002/

[tse...@chromium.org]

On 2014/12/12 22:55:56, jam wrote:
> On 2014/12/12 22:36:12, Tom Sepez wrote:
> > I'd keep the memory stuff as-is. I'm not sure we want to tie this to a
> specific
> > allocator behaviour.

> which memory stuff are you talking about? not the
> USE_MSDOS_MEMMGR/USE_MAC_MEMMGR i presume which aren't used?

No, just the OOM stuff you asked about.

> https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc
> File samples/pdfium_test.cc (right):


https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc#newcode381
> samples/pdfium_test.cc:381: FPDF_InitLibrary();
> On 2014/12/12 22:36:12, Tom Sepez wrote:
> > OK for now, but interestingly, we're probably going to have to modify
> this
> > function to accept some js-specific arguments now that JS can't find
> its own
> > code anymore ( see
https://code.google.com/p/chromium/issues/detail?id=439793
> ).

> i'm curious, why do that instead of initializing v8 directly from src/pdf?
i.e.
> third_party/pdfium already doesn't initialize v8 itself, and depends on
> the
> wrapper code to do so.

That sounds like a better approach, however what's going on here?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium/fpdfsdk/src/javascript/JS_Runtime.cpp&q=v8::%20file:third_party/pdfium&sq=package:chromium&type=cs&l=42

in XFA we have more of the same (which still needs resolution).


https://codereview.chromium.org/801913002/

[John Abd-El-Malek]

On Fri, Dec 12, 2014 at 3:09 PM, <tse...@chromium.org> wrote:

On 2014/12/12 22:55:56, jam wrote:

On 2014/12/12 22:36:12, Tom Sepez wrote:
> I'd keep the memory stuff as-is.  I'm not sure we want to tie this to a
specific
> allocator behaviour.

which memory stuff are you talking about? not the
USE_MSDOS_MEMMGR/USE_MAC_MEMMGR i presume which aren't used?

No, just the OOM stuff you asked about.

Why would we keep that method? it does nothing.

 

https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/801913002/diff/1/samples/pdfium_test.cc#newcode381

samples/pdfium_test.cc:381: FPDF_InitLibrary();
On 2014/12/12 22:36:12, Tom Sepez wrote:
> OK for now, but interestingly, we're probably going to have to modify this
> function to accept some js-specific arguments now that JS can't find its own
> code anymore ( see

https://code.google.com/p/chromium/issues/detail?id=439793

).

i'm curious, why do that instead of initializing v8 directly from src/pdf?

i.e.

third_party/pdfium already doesn't initialize v8 itself, and depends on the
wrapper code to do so.

That sounds like a better approach, however what's going on here?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium/fpdfsdk/src/javascript/JS_Runtime.cpp&q=v8::%20file:third_party/pdfium&sq=package:chromium&type=cs&l=42

in XFA we have more of the same (which still needs resolution).

ah, I wasn't aware of this code. I was thinking of the v8::V8::InitializeICU() code that we ifdef out for foxit_chrome_build.

it seems that either way, we want to avoid passing in path names to third_party/pdfium (which this change does). historically we've done things like this by having embedder apis to get data (i.e. like the fonts on linux). so i'm still not sure why we would keep the existing FPDF_InitLibrary?

https://codereview.chromium.org/801913002/

[Thomas Sepez]

ory stuff are you talking about? not the
USE_MSDOS_MEMMGR/USE_MAC_MEMMGR i presume which aren't used?

No, just the OOM stuff you asked about.

Why would we keep that method? it does nothing.

 

Ok, kill it.  It was a placeholder in case things changed, but we can do that should it ever arise.

That sounds like a better approach, however what's going on here?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium/fpdfsdk/src/javascript/JS_Runtime.cpp&q=v8::%20file:third_party/pdfium&sq=package:chromium&type=cs&l=42

in XFA we have more of the same (which still needs resolution).

ah, I wasn't aware of this code. I was thinking of the v8::V8::InitializeICU() code that we ifdef out for foxit_chrome_build.

it seems that either way, we want to avoid passing in path names to third_party/pdfium (which this change does). historically we've done things like this by having embedder apis to get data (i.e. like the fonts on linux). so i'm still not sure why we would keep the existing FPDF_InitLibrary?

I think they have that code so they can defer initializaton until they know they need JS. Agreed that a callback approach would be better.

[tse...@chromium.org]

I think its OK to land this as-is and take care of any other issues as a
follow-one. LGTM.


https://codereview.chromium.org/801913002/

[j...@chromium.org]

Committed patchset #1 (id:1) manually as
217ecf3b39f8d5c01260684848a8886c8ed2bf89 (presubmit successful).

https://codereview.chromium.org/801913002/