Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221823003:
Add support to Document::gotoNamedDest method. (Closed)
DescriptionAdd support to Document::gotoNamedDest method.
Patch implements the Document's API gotoNamedDest, which is
part of the PDF specification [1], page 129, with the following
(short) description:
"Use this method to go to a named destination within the
PDF document".
[1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf
"Named destination" is a common concept in the PDF world.
It can be used together with PDF's Links, Annotations, Bookmarks
and OpenActions, as well as an action per se, in case "this.gotoNamedDest"
is called directly.
Note that the implementation makes use of the existing hook
CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling
out the embedder to actually handle it.
In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction
which only handles for now the "page" property of the "destination".
Other properties, including zoom level, and scroll position
are ignored for the moment.
BUG=pdfium:492
Committed: https://pdfium.googlesource.com/pdfium/+/1c836753bb86b3eb0e130f1d92868a273bb26ab8
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add support to Document::gotoNamedDest method. #
Total comments: 2
Patch Set 3 : Add support to Document::gotoNamedDest method. #
Messages
Total messages: 25 (6 generated)
Description was changed from ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) descriptiton: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with both PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per si, in case "this.gotoNamedDest" is called directly. Note that the implements makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scrollt are ignored for the moment. BUG=492 ========== to ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) descriptiton: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with both PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per si, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=492 ==========
tonikitoo@igalia.com changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org
PTAL
https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... File fpdfsdk/javascript/Document.cpp (right): https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... fpdfsdk/javascript/Document.cpp:1487: CFX_ByteString utf16Name = wideName.UTF8Encode(); What about CFX_ByteString utf16Name = params[0].ToString(); ? https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... fpdfsdk/javascript/Document.cpp:1489: CPDF_NameTree name_tree(pDocument, "Dests"); nit: name_tree should probably be nameTree https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... fpdfsdk/javascript/Document.cpp:1500: float* pPosAry = nullptr; Can float* pPosAry be a std::unique_ptr and then just use .get() below? https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... fpdfsdk/javascript/Document.cpp:1507: } nit: no {}'s https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... fpdfsdk/javascript/Document.cpp:1512: pApp->FFI_DoGoToAction(nPageIndex, nFitType, pPosAry, sizeOfAry); No need to store nFitType, just use dest.GetZoomMode() here. Same with nPageIndex. https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... fpdfsdk/javascript/Document.cpp:1513: delete[] pPosAry; delete won't be needed after moving to a unique_ptr https://codereview.chromium.org/2221823003/diff/1/testing/resources/javascrip... File testing/resources/javascript/document_methods.in (right): https://codereview.chromium.org/2221823003/diff/1/testing/resources/javascrip... testing/resources/javascript/document_methods.in:225: // TODO: test success cases. Typically the format is TODO(username). It doesn't mean username will fix the TODO, just they're the person with the most knowledge of what the TODO is about.
On 2016/08/08 19:54:52, tonikitoo1 wrote: > PTAL I worry about calling back to the embedder while JS is still on the stack. Does this already happen at present? Can we get a new action as a result of this which would then trigger more JS on top of itself? Thanks.
On 2016/08/08 20:05:30, dsinclair wrote: > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > File fpdfsdk/javascript/Document.cpp (right): > > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > fpdfsdk/javascript/Document.cpp:1487: CFX_ByteString utf16Name = > wideName.UTF8Encode(); > What about CFX_ByteString utf16Name = params[0].ToString(); ? I tried it, but it seems like this method is not available: ../../fpdfsdk/javascript/Document.cpp:1488:40: error: no member named 'ToString' in 'CJS_Value' CFX_ByteString utf16Name = params[0].ToString(); ~~~~~~~~~ ^ > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > fpdfsdk/javascript/Document.cpp:1489: CPDF_NameTree name_tree(pDocument, > "Dests"); > nit: name_tree should probably be nameTree Done. > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > fpdfsdk/javascript/Document.cpp:1500: float* pPosAry = nullptr; > Can float* pPosAry be a std::unique_ptr and then just use .get() below? Done. > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > fpdfsdk/javascript/Document.cpp:1507: } > nit: no {}'s Done. > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > fpdfsdk/javascript/Document.cpp:1512: pApp->FFI_DoGoToAction(nPageIndex, > nFitType, pPosAry, sizeOfAry); > No need to store nFitType, just use dest.GetZoomMode() here. Same with > nPageIndex. Done. > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document... > fpdfsdk/javascript/Document.cpp:1513: delete[] pPosAry; > delete won't be needed after moving to a unique_ptr Done. > https://codereview.chromium.org/2221823003/diff/1/testing/resources/javascrip... > File testing/resources/javascript/document_methods.in (right): > > https://codereview.chromium.org/2221823003/diff/1/testing/resources/javascrip... > testing/resources/javascript/document_methods.in:225: // TODO: test success > cases. > Typically the format is TODO(username). It doesn't mean username will fix the > TODO, just they're the person with the most knowledge of what the TODO is about. Done. Added myself here, as I am willing to take the challenge and keep improving this test coverage.
On 2016/08/08 20:05:43, Tom Sepez wrote:
> On 2016/08/08 19:54:52, tonikitoo1 wrote:
> > PTAL
>
> I worry about calling back to the embedder while JS is still on the stack.
Does
> this already happen at present? Can we get a new action as a result of this
> which would then trigger more JS on top of itself? Thanks.
Hi Tom. This is a good point. For the record, the answer to your question here
is yes, existing methods do call the embedder while JS is still on the stack:
- Document::mailForm
- Document::submitForm
- Document::pageNum
- Document::print
- Document::gotoNamedDest <- being added here.
On the embedder side (Chromium), this method basically calls sends IPC message
out to the Browser process:
971 void OutOfProcessInstance::ScrollToPage(int page) {
972 if (engine_->GetNumberOfPages() == 0)
973 return;
974
975 pp::VarDictionary message;
976 message.Set(kType, kJSGoToPageType);
977 message.Set(kJSPageNumber, pp::Var(page));
978 PostMessage(message);
> Hi Tom. This is a good point. For the record, the answer to your question here
> is yes, existing methods do call the embedder while JS is still on the stack:
>
> - Document::mailForm
> - Document::submitForm
> - Document::pageNum
> - Document::print
> - Document::gotoNamedDest <- being added here.
Sorry, I wasn't entirely clear. Yes they all call back into the embedder, but
they just get info or take an action
and return control back to JS. What I worry about is that despite the current
implementation, we could get
JS -> embedder -> JS, since the FFI_DoGoToAction sounds like it might be capable
of triggering another action.
>
> On the embedder side (Chromium), this method basically calls sends IPC message
> out to the Browser process:
>
> 971 void (int page) {
> 972 if (engine_->GetNumberOfPages() == 0)
> 973 return;
> 974
> 975 pp::VarDictionary message;
> 976 message.Set(kType, kJSGoToPageType);
> 977 message.Set(kJSPageNumber, pp::Var(page));
> 978 PostMessage(message);
Description was changed from ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) descriptiton: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with both PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per si, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=492 ========== to ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) description: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with both PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per se, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=492 ==========
Alternatively, Maybe the thing to do is just to document the restriction in the public/ header file.
Ok, there appears to be some precedent for this based on setting page number, etc. so I think we can take the patch.
https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Docu... File fpdfsdk/javascript/Document.cpp (right): https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Docu... fpdfsdk/javascript/Document.cpp:1487: CFX_ByteString utf16Name = wideName.UTF8Encode(); nit: naming? Odd that the result of UTF8Encode would be called utf16. https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Docu... fpdfsdk/javascript/Document.cpp:1501: scrollPositionArray = nit: generally the technique to use here is: scrollPositionArray.reset(new float[arrayObject->GetCount()]);
On 2016/08/08 21:35:33, Tom Sepez wrote:
> > Hi Tom. This is a good point. For the record, the answer to your question
here
> > is yes, existing methods do call the embedder while JS is still on the
stack:
> >
> > - Document::mailForm
> > - Document::submitForm
> > - Document::pageNum
> > - Document::print
> > - Document::gotoNamedDest <- being added here.
>
>
> Sorry, I wasn't entirely clear. Yes they all call back into the embedder, but
> they just get info or take an action
> and return control back to JS. What I worry about is that despite the current
> implementation, we could get
> JS -> embedder -> JS, since the FFI_DoGoToAction sounds like it might be
capable
> of triggering another action.
>
I see. Out of the methods listed above, two of them prevent themselves against
JS re-entrance:
- Document::mailForm
- Document::submitForm
They do so by wrapping the embedding hook call with BeginBlock/EndBlock calls,
e.g.
pRuntime->BeginBlock();
pEnv->callTheEmbedded(...);
pRuntime->EndBlock();
.. And this is how it prevents JS re-entrance:
void app::RunJsScript(CJS_Runtime* pRuntime, const CFX_WideString& wsScript) {
if (!pRuntime->IsBlocking()) {
(..)
}
}
@tsepez: Would you prefer to wrap our call to the "embedder" (e.g. chromium)
call with BeginBlock/EndBlock here?
> @tsepez: Would you prefer to wrap our call to the "embedder" (e.g. chromium) > call with BeginBlock/EndBlock here? I think calling BeginBlock() is a good idea here.
On 2016/08/08 22:14:57, Tom Sepez wrote: > > @tsepez: Would you prefer to wrap our call to the "embedder" (e.g. chromium) > > call with BeginBlock/EndBlock here? > I think calling BeginBlock() is a good idea here. Done. > https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Docu... > File fpdfsdk/javascript/Document.cpp (right): > > https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Docu... > fpdfsdk/javascript/Document.cpp:1487: CFX_ByteString utf16Name = > wideName.UTF8Encode(); > nit: naming? Odd that the result of UTF8Encode would be called utf16. Done. > https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Docu... > fpdfsdk/javascript/Document.cpp:1501: scrollPositionArray = > nit: generally the technique to use here is: > > scrollPositionArray.reset(new float[arrayObject->GetCount()]); Done.
lgtm
BUG= need to say pdfium:492, otherwise it refers to Chromium by default. Please note I already fixed a few typos in the CL description.
Description was changed from ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) description: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with both PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per se, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=492 ========== to ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) description: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per se, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=pdfium:492 ==========
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...
Message was sent while issue was closed.
Description was changed from ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) description: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per se, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=pdfium:492 ========== to ========== Add support to Document::gotoNamedDest method. Patch implements the Document's API gotoNamedDest, which is part of the PDF specification [1], page 129, with the following (short) description: "Use this method to go to a named destination within the PDF document". [1] http://partners.adobe.com/public/developer/en/acrobat/sdk/5186AcroJS.pdf "Named destination" is a common concept in the PDF world. It can be used together with PDF's Links, Annotations, Bookmarks and OpenActions, as well as an action per se, in case "this.gotoNamedDest" is called directly. Note that the implementation makes use of the existing hook CPDFDoc_Environment::FFI_DoGoToAction, which ends up calling out the embedder to actually handle it. In case of Chromium, for instance, it calls PDFiumEngine::Form_DoGoToAction which only handles for now the "page" property of the "destination". Other properties, including zoom level, and scroll position are ignored for the moment. BUG=pdfium:492 Committed: https://pdfium.googlesource.com/pdfium/+/1c836753bb86b3eb0e130f1d92868a273bb2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/1c836753bb86b3eb0e130f1d92868a273bb2...
Message was sent while issue was closed.
On 2016/08/08 22:37:45, Lei Zhang wrote: > BUG= need to say pdfium:492, otherwise it refers to Chromium by default. Please > note I already fixed a few typos in the CL description. Thanks for the reviews! Actually, in https://codereview.chromium.org/2219183002/ I wrote the commit message with "BUG=492", and system was able to bug-mail https://bugs.chromium.org/p/pdfium/issues/detail?id=492#c5 .
Message was sent while issue was closed.
On 2016/08/08 23:43:00, tonikitoo1 wrote: > On 2016/08/08 22:37:45, Lei Zhang wrote: > > BUG= need to say pdfium:492, otherwise it refers to Chromium by default. > Please > > note I already fixed a few typos in the CL description. > > Thanks for the reviews! > > Actually, in https://codereview.chromium.org/2219183002/ I wrote the commit > message with "BUG=492", and system was able to bug-mail > https://bugs.chromium.org/p/pdfium/issues/detail?id=492#c5 . Yes, but clicking the link in https://codereview.chromium.org/2219183002/ takes you to the wrong bug. Better to add the prefix for PDFium bugs. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
