Chromium Code Reviews
Help | Chromium Project | Sign in
(22)

Issue 2277063003: Extend pdfium_test capability so that more Javascript can be executed (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months ago by tonikitoo
Modified:
7 months, 1 week ago
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

Extend pdfium_test capability so that more Javascript can be executed In [1], the lack of support of pdfium_test to some application level hooks was felt. More specifically, the lack of implementation of the hook FFI_GetPage, called when 'this.getAnnot()' is executed in an Acrobar JS context, makes it non-trivial to JS texts that manipulate PDF annotations. [1] https://codereview.chromium.org/2265313002/ Here is the failing call stack in pdfium_test: 0 ::RenderPdf (samples/pdfium_test.cc) 1 ::FORM_DoDocumentOpenAction (fpdfsdk/fpdfformfill.cpp) 2 CPDFSDK_Document::ProcOpenAction (fpdfsdk/fsdk_mgr.cpp) 3 CPDFSDK_ActionHandler::DoAction_DocOpen (fpdfsdk/fsdk_actionhandler.cpp) <----v8----> 4 Document::getAnnot (fpdfsdk/javascript/Document.cpp) 5 CPDFSDK_Document::GetPageView (fpdfsdk/fsdk_mgr.cpp) 6 CPDFDoc_Environment::FFI_GetPage (fpdfsdk/include/fsdk_mgr.h) (frame 6 returns nullptr, and getAnnot call in frame 4 bails) CL extends pdfium_test app with a FFI_GetPage hook implementation. Basically what FFI_GetPage does is returning a FPDF_PAGE instance. In case of pdfium_test, FPDF_PAGE instances were only created on demand when the page was going to get rendered, and then discarded. Since FFI_GetPage can be called by JS before pages are rendered, CL moved the page creation code into a helper function, and cached the FPDF_PAGE instances created in a map, so it does not recreate them needlessly. BUG=pdfium:492 Committed: https://pdfium.googlesource.com/pdfium/+/3e98158a6c47361ca7d6c2c18d47c9f8f3aabb8a

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -6 lines) Patch
M samples/pdfium_test.cc View 6 chunks +35 lines, -6 lines 5 comments Download
A testing/resources/pixel/bug_492.pdf View Binary file 0 comments Download
A testing/resources/pixel/bug_492.pdf.0.png View Binary file 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 16 (4 generated)
tonikitoo
I did not get happy with us not being able to add a test in ...
8 months ago (2016-08-25 19:29:16 UTC) #3
dsinclair
https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc#newcode59 samples/pdfium_test.cc:59: std::map<int, FPDF_PAGE> g_loadedPages; It may actually be closer to ...
8 months ago (2016-08-25 19:36:49 UTC) #4
tonikitoo
https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc#newcode59 samples/pdfium_test.cc:59: std::map<int, FPDF_PAGE> g_loadedPages; On 2016/08/25 19:36:49, dsinclair wrote: > ...
8 months ago (2016-08-25 19:48:59 UTC) #5
dsinclair
On 2016/08/25 19:48:59, tonikitoo1 wrote: > Chromium keeps track of the pages in src/pdf/pdfium/pdfium_engine.h > ...
8 months ago (2016-08-25 20:05:18 UTC) #6
tonikitoo
On 2016/08/25 20:05:18, dsinclair wrote: > On 2016/08/25 19:48:59, tonikitoo1 wrote: > > Chromium keeps ...
8 months ago (2016-08-25 21:44:39 UTC) #7
tonikitoo
On 2016/08/25 20:05:18, dsinclair wrote: > On 2016/08/25 19:48:59, tonikitoo1 wrote: > > Chromium keeps ...
8 months ago (2016-08-26 13:37:36 UTC) #8
dsinclair
lgtm Thanks for the explanation, it sounds like what you've got here makes the most ...
8 months ago (2016-08-26 15:16:03 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2277063003/1
8 months ago (2016-08-26 15:16:15 UTC) #11
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://pdfium.googlesource.com/pdfium/+/3e98158a6c47361ca7d6c2c18d47c9f8f3aabb8a
8 months ago (2016-08-26 15:37:16 UTC) #13
Lei Zhang
It would be nice if the sample program can set a good example for those ...
8 months ago (2016-08-27 01:03:23 UTC) #14
tonikitoo
On 2016/08/27 01:03:23, Lei Zhang wrote: > It would be nice if the sample program ...
8 months ago (2016-08-27 04:57:33 UTC) #15
tonikitoo
7 months, 1 week ago (2016-09-12 15:04:40 UTC) #16
Message was sent while issue was closed.
On 2016/08/27 04:57:33, tonikitoo wrote:
> On 2016/08/27 01:03:23, Lei Zhang wrote:
> > It would be nice if the sample program can set a good example for those that
> are
> > learning PDFium code.
> > 
> > https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc
> > File samples/pdfium_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc#newc...
> > samples/pdfium_test.cc:59: std::map<int, FPDF_PAGE> g_loadedPages;
> > On 2016/08/25 19:36:49, dsinclair wrote:
> > > It may actually be closer to what Chromium is doing by not keeping the map
> and
> > > creating the pages on the fly.
> > > 
> > > Chromium will discard pages when they aren't in view in order to save
> memory.
> > > This can at times trigger bugs in PDFium as we execute JS that access
pages
> > that
> > > are no longer loaded.
> > > 
> > > (Correct me if I'm wrong Lei)
> > 
> > Form_GetPage() does reload the page as shown. The Chrome PDF Viewer does
have
> > the choice to unload a page if it like to. The actual implementation is a
bit
> > more complex because we need to avoid bonehead maneuvers like unloading
while
> > unloading.
> > 
> >
>
https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc#newc...
> > samples/pdfium_test.cc:59: std::map<int, FPDF_PAGE> g_loadedPages;
> > I believe this actually adds a static initializer, which is not ideal.
> > 
> >
>
https://codereview.chromium.org/2277063003/diff/1/samples/pdfium_test.cc#newc...
> > samples/pdfium_test.cc:63: FPDF_FORMHANDLE g_formHandle;
> > It would have been nice to use GetPageForIndex()'s |param| argument to pass
> this
> > around, instead of having a global. Maybe make the param a struct to pass in
> > |g_loadedPages| as well.
> 
> Thanks Lei. I will get your comments addressed in a follow up CL.

For the sake of completeness/history: https://codereview.chromium.org/2330043002
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46