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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 3 weeks ago by tonikitoo
Modified:
9 months, 3 weeks 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
9 months, 3 weeks 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 = ...
9 months, 3 weeks 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 ...
9 months, 3 weeks 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 > ...
9 months, 3 weeks 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 ...
9 months, 3 weeks 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 ...
9 months, 3 weeks 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/ ...
9 months, 3 weeks 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. ...
9 months, 3 weeks 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 ...
9 months, 3 weeks 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. ...
9 months, 3 weeks 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) > ...
9 months, 3 weeks 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 ...
9 months, 3 weeks ago (2016-08-08 22:25:52 UTC) #15
tonikitoo
9 months, 3 weeks ago (2016-08-08 22:26:48 UTC) #16
Tom Sepez
lgtm
9 months, 3 weeks 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 ...
9 months, 3 weeks 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
9 months, 3 weeks 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
9 months, 3 weeks 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 ...
9 months, 3 weeks ago (2016-08-08 23:43:00 UTC) #24
Lei Zhang
9 months, 3 weeks 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 650457f06