Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2221823003: Add support to Document::gotoNamedDest method. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by tonikitoo
Modified:
1 year, 1 month 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

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -0 lines) Patch
M fpdfsdk/javascript/Document.h View 2 chunks +5 lines, -0 lines 0 comments Download
M fpdfsdk/javascript/Document.cpp View 3 chunks +49 lines, -0 lines 0 comments Download
M testing/resources/javascript/document_methods.in View 2 chunks +12 lines, -0 lines 0 comments Download
M testing/resources/javascript/document_methods_expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 25 (6 generated)
tonikitoo
PTAL
1 year, 1 month ago (2016-08-08 19:54:52 UTC) #3
dsinclair
https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document.cpp File fpdfsdk/javascript/Document.cpp (right): https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document.cpp#newcode1487 fpdfsdk/javascript/Document.cpp:1487: CFX_ByteString utf16Name = wideName.UTF8Encode(); What about CFX_ByteString utf16Name = ...
1 year, 1 month ago (2016-08-08 20:05:30 UTC) #4
Tom Sepez
On 2016/08/08 19:54:52, tonikitoo1 wrote: > PTAL I worry about calling back to the embedder ...
1 year, 1 month ago (2016-08-08 20:05:43 UTC) #5
tonikitoo
On 2016/08/08 20:05:30, dsinclair wrote: > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document.cpp > File fpdfsdk/javascript/Document.cpp (right): > > https://codereview.chromium.org/2221823003/diff/1/fpdfsdk/javascript/Document.cpp#newcode1487 > ...
1 year, 1 month ago (2016-08-08 21:25:31 UTC) #6
tonikitoo
On 2016/08/08 20:05:43, Tom Sepez wrote: > On 2016/08/08 19:54:52, tonikitoo1 wrote: > > PTAL ...
1 year, 1 month ago (2016-08-08 21:31:24 UTC) #7
Tom Sepez
> Hi Tom. This is a good point. For the record, the answer to your ...
1 year, 1 month ago (2016-08-08 21:35:33 UTC) #8
Tom Sepez
Alternatively, Maybe the thing to do is just to document the restriction in the public/ ...
1 year, 1 month ago (2016-08-08 21:45:50 UTC) #10
Tom Sepez
Ok, there appears to be some precedent for this based on setting page number, etc. ...
1 year, 1 month ago (2016-08-08 21:54:47 UTC) #11
Tom Sepez
https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Document.cpp File fpdfsdk/javascript/Document.cpp (right): https://codereview.chromium.org/2221823003/diff/20001/fpdfsdk/javascript/Document.cpp#newcode1487 fpdfsdk/javascript/Document.cpp:1487: CFX_ByteString utf16Name = wideName.UTF8Encode(); nit: naming? Odd that the ...
1 year, 1 month ago (2016-08-08 22:05:24 UTC) #12
tonikitoo
On 2016/08/08 21:35:33, Tom Sepez wrote: > > Hi Tom. This is a good point. ...
1 year, 1 month ago (2016-08-08 22:13:01 UTC) #13
Tom Sepez
> @tsepez: Would you prefer to wrap our call to the "embedder" (e.g. chromium) > ...
1 year, 1 month ago (2016-08-08 22:14:57 UTC) #14
tonikitoo
On 2016/08/08 22:14:57, Tom Sepez wrote: > > @tsepez: Would you prefer to wrap our ...
1 year, 1 month ago (2016-08-08 22:25:52 UTC) #15
tonikitoo
1 year, 1 month ago (2016-08-08 22:26:48 UTC) #16
Tom Sepez
lgtm
1 year, 1 month ago (2016-08-08 22:28:53 UTC) #17
Lei Zhang
BUG= need to say pdfium:492, otherwise it refers to Chromium by default. Please note I ...
1 year, 1 month ago (2016-08-08 22:37:45 UTC) #18
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/2221823003/40001
1 year, 1 month ago (2016-08-08 22:48:03 UTC) #21
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/1c836753bb86b3eb0e130f1d92868a273bb26ab8
1 year, 1 month ago (2016-08-08 23:14:09 UTC) #23
tonikitoo
On 2016/08/08 22:37:45, Lei Zhang wrote: > BUG= need to say pdfium:492, otherwise it refers ...
1 year, 1 month ago (2016-08-08 23:43:00 UTC) #24
Lei Zhang
1 year, 1 month ago (2016-08-08 23:51:09 UTC) #25
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.
Sign in to reply to this message.

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