Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(476)

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

Created:
4 years, 4 months ago by tonikitoo
Modified:
4 years, 4 months 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

Messages

Total messages: 25 (6 generated)
tonikitoo
PTAL
4 years, 4 months 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 = ...
4 years, 4 months 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 ...
4 years, 4 months 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 > ...
4 years, 4 months 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 ...
4 years, 4 months 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 ...
4 years, 4 months 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/ ...
4 years, 4 months 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. ...
4 years, 4 months 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 ...
4 years, 4 months 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. ...
4 years, 4 months 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) > ...
4 years, 4 months 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 ...
4 years, 4 months ago (2016-08-08 22:25:52 UTC) #15
tonikitoo
4 years, 4 months ago (2016-08-08 22:26:48 UTC) #16
Tom Sepez
lgtm
4 years, 4 months 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 ...
4 years, 4 months 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
4 years, 4 months 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
4 years, 4 months 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 ...
4 years, 4 months ago (2016-08-08 23:43:00 UTC) #24
Lei Zhang
4 years, 4 months 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.

Powered by Google App Engine
This is Rietveld 408576698