Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265313002:
Lazy generate an "AP" when an Annot's hidden state changes (Closed)
DescriptionLazy generate an "AP" when an Annot's hidden state changes
Now that Document::getAnnot works and annotation instances
can have its properties changed, consider the following
scenario:
- A PDF content has an annotation without AP and
CPVT_GenerateAP is called to generate one.
- However the annotation also has its hidden flag set (/F 2),
and CPVT_GenerateAP bails out earlier, not generating an AP.
- When the PDF's Javascript runs, it acquires an instance of
this annotation object, bounded to JS using Document::getAnnot(),
and set its "hidden" flag to false.
- At this point, the annotation should get drawn, but it does
not because its "AP" was never generated.
CL fixes this scenario by making PDFium able to lazy
generate APs, if needed.
BUG=pdfium:492
Committed: https://pdfium.googlesource.com/pdfium/+/ade4b495433751ac853f2d677b9e1da94d0d6bf7
Patch Set 1 #Patch Set 2 : Some minor clean ups. #
Messages
Total messages: 20 (5 generated)
Description was changed from
==========
Lazy generate an "AP" when an Annot's hidden state changes
When an annotation lacks its own apparence definition
("AP"), PDFium generates one for it in accordance to
its type (see CPVT_GenerateAP class), unless it is flagged
as "hidden" (e.g. /F 2).
Now that Document::getAnnot works and annotation instances can
have its properties changed, consider the following scenario:
- A PDF content has an annotation without AP and CPVT_GenerateAP
is called to generate one.
- However the annotation also has its hidden flag set, and CPVT_GenerateAP
bails out earlier, not generating an AP.
- When the PDF's Javascript runs, it acquires an instance of
this annotation object, bounded to JS using Document::getAnnot(),
and set its "hidden" flag to false.
- At this point, the annotation should get drawn, but it does not
because its "AP" was never generated.
This CL fixes this scenario, making PDFium able to lazy
generate APs, if needed.
BUG=pdfium:492
==========
to
==========
Lazy generate an "AP" when an Annot's hidden state changes
When an annotation lacks its own apparence definition
("AP"), PDFium generates one for it in accordance to
its type (see CPVT_GenerateAP class), unless it is flagged
as "hidden" (e.g. /F 2).
Now that Document::getAnnot works and annotation instances can
have its properties changed, consider the following scenario:
- A PDF content has an annotation without AP and CPVT_GenerateAP
is called to generate one.
- However the annotation also has its hidden flag set, and CPVT_GenerateAP
bails out earlier, not generating an AP.
- When the PDF's Javascript runs, it acquires an instance of
this annotation object, bounded to JS using Document::getAnnot(),
and set its "hidden" flag to false.
- At this point, the annotation should get drawn, but it does not
because its "AP" was never generated.
This CL fixes this scenario, making PDFium able to lazy
generate APs, if needed.
BUG=pdfium:492
==========
tonikitoo@igalia.com changed reviewers: + dsinclair@chromium.org, jaepark@google.com, thestig@chromium.org, tsepez@chromium.org
PTAL
Can we add a test case? Otherwise looks good.
Thanks for looking!
On 2016/08/23 00:12:04, Tom Sepez (Offsite til 8-23) wrote:
> Can we add a test case? Otherwise looks good.
I believe so. Test could be as simple as:
- have a highlight annotation, hidden by default.
/Type /Annot
/Subtype /Highlight
/QuadPoints [475.0 688.0 512.0 688.0 475.0 679.0 512.0 679.0]
/C [0.0 0.0 1.0]
/CA 0.2
/Rect [475.0 681.0 512.0 690.0]
/F 2
/NM (AnnotName)
- have some JS that acquires the annotation object and "un-hides" it:
/JS (
var a = this.getAnnot(0, "AnnotName");
a.hidden = false;
)
That being said, how do you guys produce PDFs to serve as test case for the
issues fixed? Any specific tool or procedure?
On 2016/08/23 00:50:46, tonikitoo1 wrote: > That being said, how do you guys produce PDFs to serve as test case for the > issues fixed? Any specific tool or procedure? Some testing/resources/foo.pdf files are written using testing/resources/foo.in + testing/tools/fixup_pdf_template.py.
On 2016/08/23 00:12:04, Tom Sepez (Offsite til 8-23) wrote: > Can we add a test case? Otherwise looks good. I believe this is not easily testable with our 'pdfium_test' app. Here is the reason: - pdfium_test bails out earlier during the execution of Document::getAnnot: $ <out dir>/pdfium_test --png <my test>.pdf #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) (returns nullptr) In frame #6, pdfium calls out the embedder hook FFI_GetPage, which is not implemented in pdfium_test (note that it is implemented in Chromium's corresponding class, PDFiumEngine). Additionally, I believe this hook is not easily implementable in pdfium_test given the way it works: each page in the PDF document is read, rendered into a PNG file and discarded sequentially, hence pdfium_test does not keep track of each page loaded. Adding an implementation to FFI_GetPage in pdfium_test would likely require it to reload the given page index, which I do not think it right. That being said, I believe the exceptional absence of a test case in this CL is justified by the limitation described above in our pdfium_test app. WDYT?
On 2016/08/23 18:50:16, tonikitoo1 wrote: > On 2016/08/23 00:12:04, Tom Sepez (Offsite til 8-23) wrote: > > Can we add a test case? Otherwise looks good. > > I believe this is not easily testable with our 'pdfium_test' app. Here is the > reason: The next level of testing is with an embedder test. There, you can actually hook FFI_GetPage and such.
On 2016/08/23 18:53:00, Lei Zhang wrote: > On 2016/08/23 18:50:16, tonikitoo1 wrote: > > On 2016/08/23 00:12:04, Tom Sepez (Offsite til 8-23) wrote: > > > Can we add a test case? Otherwise looks good. > > > > I believe this is not easily testable with our 'pdfium_test' app. Here is the > > reason: > > The next level of testing is with an embedder test. There, you can actually hook > FFI_GetPage and such. I think what we need here is a pixel test, since the annotation in case has to show. Hence, my attempt to work it out with 'pdfium_test'. BTW, this is the testing PDF: https://people.igalia.com/agomes/pdfium/samples/bug_492.pdf Loading it with PDFium ToT does not show the highlight annotation. Loading it with the fix applied, does.
Description was changed from
==========
Lazy generate an "AP" when an Annot's hidden state changes
When an annotation lacks its own apparence definition
("AP"), PDFium generates one for it in accordance to
its type (see CPVT_GenerateAP class), unless it is flagged
as "hidden" (e.g. /F 2).
Now that Document::getAnnot works and annotation instances can
have its properties changed, consider the following scenario:
- A PDF content has an annotation without AP and CPVT_GenerateAP
is called to generate one.
- However the annotation also has its hidden flag set, and CPVT_GenerateAP
bails out earlier, not generating an AP.
- When the PDF's Javascript runs, it acquires an instance of
this annotation object, bounded to JS using Document::getAnnot(),
and set its "hidden" flag to false.
- At this point, the annotation should get drawn, but it does not
because its "AP" was never generated.
This CL fixes this scenario, making PDFium able to lazy
generate APs, if needed.
BUG=pdfium:492
==========
to
==========
Lazy generate an "AP" when an Annot's hidden state changes
Now that Document::getAnnot works and annotation instances
can have its properties changed, consider the following
scenario:
- A PDF content has an annotation without AP and
CPVT_GenerateAP is called to generate one.
- However the annotation also has its hidden flag set (/F 2),
and CPVT_GenerateAP bails out earlier, not generating an AP.
- When the PDF's Javascript runs, it acquires an instance of
this annotation object, bounded to JS using Document::getAnnot(),
and set its "hidden" flag to false.
- At this point, the annotation should get drawn, but it does
not because its "AP" was never generated.
CL fixes this scenario by making PDFium able to lazy
generate APs, if needed.
BUG=pdfium:492
==========
> > Loading it with PDFium ToT does not show the highlight annotation. Loading it > with the fix applied, does. By "Loading", do you mean using pdfium_test, or chrome, or some other embedder?
On 2016/08/24 16:25:37, Tom Sepez wrote: > > > > Loading it with PDFium ToT does not show the highlight annotation. Loading it > > with the fix applied, does. > By "Loading", do you mean using pdfium_test, or chrome, or some other embedder? Sorry for not being clear. I meant to say this: " If [1] is loaded in Chromium, with PDFium ToT, the highlight annotation is not shown as it should. On the other hand, when loading [1] in Chromium with PDFium ToT and fix applied, the highlight annotation does show. [1] https://people.igalia.com/agomes/pdfium/samples/bug_492.pdf "
Ah, so there's probably a missing call in pdfium_test to draw the annotations. If you're ambitious, you might want to suggest a change to it, otherwise its probably ok as-is.
On 2016/08/24 16:41:31, Tom Sepez wrote:
> Ah, so there's probably a missing call in pdfium_test to draw the annotations.
> If you're ambitious, you might want to suggest a change to it, otherwise its
> probably ok as-is.
As described in comment #7, pdfium_test does not even unhide the annotation,
because the JS execution bails out earlier:
#0 /JS (
#1 var a = this.getAnnot(0, "Annot-1");
#2 if ( a != null )
#3 a.hidden = false;
#4 )
when line #1 runs, Document::getAnnot is called and tries to get a pointer to
"Page 0" object (see line 1066 below).
This call ends up calling a hook on the client side that 'pdfium_test' does not
implement. So, 'a' ends up null.
1052 FX_BOOL Document::getAnnot(IJS_Context* cc,
1053 const std::vector<CJS_Value>& params,
1054 CJS_Value& vRet,
1055 CFX_WideString& sError) {
(..)
1063 int nPageNo = params[0].ToInt(pRuntime);
1064 CFX_WideString swAnnotName = params[1].ToCFXWideString(pRuntime);
1065
1066 CPDFSDK_PageView* pPageView = m_pDocument->GetPageView(nPageNo); <--- it
fails here.
1067 if (!pPageView)
1068 return FALSE;
In comment #7 I also tried to explain why I believe it does not make sense to
pdfium_test to actually implement this hook:
"
Additionally, I believe this hook is not easily implementable in pdfium_test
given the way it works: each page in the PDF document is read, rendered into a
PNG file and discarded sequentially, hence pdfium_test does not keep track of
each page loaded.
Adding an implementation to FFI_GetPage in pdfium_test would likely require it
to reload the given page index, which I do not think it right.
"
In case of Chromium, the hook is implemented and the JS execution succeeds: the
annotation is un-hidden by JS and redraw.
Ok, LGTM. Thanks for your work.
The CQ bit was checked by tonikitoo@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/24 17:22:57, Tom Sepez wrote: > Ok, LGTM. Thanks for your work. Thanks for the great review Tom/Lei. cq+
Message was sent while issue was closed.
Description was changed from ========== Lazy generate an "AP" when an Annot's hidden state changes Now that Document::getAnnot works and annotation instances can have its properties changed, consider the following scenario: - A PDF content has an annotation without AP and CPVT_GenerateAP is called to generate one. - However the annotation also has its hidden flag set (/F 2), and CPVT_GenerateAP bails out earlier, not generating an AP. - When the PDF's Javascript runs, it acquires an instance of this annotation object, bounded to JS using Document::getAnnot(), and set its "hidden" flag to false. - At this point, the annotation should get drawn, but it does not because its "AP" was never generated. CL fixes this scenario by making PDFium able to lazy generate APs, if needed. BUG=pdfium:492 ========== to ========== Lazy generate an "AP" when an Annot's hidden state changes Now that Document::getAnnot works and annotation instances can have its properties changed, consider the following scenario: - A PDF content has an annotation without AP and CPVT_GenerateAP is called to generate one. - However the annotation also has its hidden flag set (/F 2), and CPVT_GenerateAP bails out earlier, not generating an AP. - When the PDF's Javascript runs, it acquires an instance of this annotation object, bounded to JS using Document::getAnnot(), and set its "hidden" flag to false. - At this point, the annotation should get drawn, but it does not because its "AP" was never generated. CL fixes this scenario by making PDFium able to lazy generate APs, if needed. BUG=pdfium:492 Committed: https://pdfium.googlesource.com/pdfium/+/ade4b495433751ac853f2d677b9e1da94d0d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/ade4b495433751ac853f2d677b9e1da94d0d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
