Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
834703002:
Add APIs for getting bookmarks and named destinations. (Closed)
|
Created:
5 years, 11 months ago by Bo Xu Modified:
5 years, 11 months ago CC:
pdfium-reviews_googlegroups.com, Sam McNally Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd APIs for getting bookmarks and named destinations.
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/476cd69a6f5c5096a3145e0c4d567010f37739c3
Patch Set 1 #Patch Set 2 : #
Total comments: 25
Patch Set 3 : Address minor comments, still need to fix std:string #
Total comments: 3
Patch Set 4 : use C style query mode for destination name #
Total comments: 14
Patch Set 5 : address comments and change API prototype #Patch Set 6 : check pDict and fix indent #
Messages
Total messages: 35 (2 generated)
bo_xu@foxitsoftware.com changed reviewers: + jam@chromium.org, raymes@chromium.org, tsepez@chromium.org
Hi, Raymes, please review these APIs as you requested, thanks!
https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:632: DLLEXPORT FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, std::string name); This is inside an extern C block, so we don't get to use std::string here :(. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp File fpdfsdk/src/fpdfdoc.cpp (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:33: if (document == NULL) return NULL; nit: or just !document. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:37: return (CPDF_Dictionary*)tree.GetFirstChild((CPDF_Dictionary*)bookmark); The cast of the return value doen't match the declared return type above. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:46: return (CPDF_Dictionary*)tree.GetNextSibling((CPDF_Dictionary*)bookmark); ditto. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:52: nit: or just !bookmark https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:53: CPDF_Bookmark Bookmark = (CPDF_Dictionary*)bookmark; nit: maybe pBookmark to avoid starting local variable names with a capital letter. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:55: CFX_ByteString bstr = title.UTF16LE_Encode(FALSE); nit: how about encoded_title instead of bstr (following whatever convention is in effect here (encodedTitle?)). https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:56: unsigned long len = bstr.GetLength(); nit: how about title_len instead of just len https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:57: if (buffer != NULL && buflen >= len + 2) { nit: (buffer && buflen >= len + 2) https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:59: ((FX_BYTE*)buffer)[len] = ((FX_BYTE*)buffer)[len + 1] = 0; nit: two separate statements. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:794: CPDF_Document* pDoc = (CPDF_Document*)document; nit: blank line here if you want to use one-line ifs. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:821: CPDF_Document* pDoc = (CPDF_Document*)document; nit: blank line. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:827: nit: no blank line
Cleaning of current code at https://codereview.chromium.org/828203002/ to assist landing of this patch. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp File fpdfsdk/src/fpdfdoc.cpp (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:37: return (CPDF_Dictionary*)tree.GetFirstChild((CPDF_Dictionary*)bookmark); On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > The cast of the return value doen't match the declared return type above. This is an operator in CPDF_Bookmark class. I removed it in another patch. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:46: return (CPDF_Dictionary*)tree.GetNextSibling((CPDF_Dictionary*)bookmark); On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > ditto. ditto
raymes@chromium.org changed reviewers: + alexandrec@chromium.org
We also need a way to get the title of a bookmark. Could you also please add a function that does this (FPDFBookmark_GetTitle)?
On 2015/01/05 00:25:55, Alexandre Carlton wrote: > We also need a way to get the title of a bookmark. Could you also please add a > function that does this (FPDFBookmark_GetTitle)? Sorry, I just saw the function that does this.
On 2015/01/01 06:40:38, Bo Xu wrote: > Cleaning of current code at https://codereview.chromium.org/828203002/ to assist > landing of this patch. > > https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp > File fpdfsdk/src/fpdfdoc.cpp (right): > > https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... > fpdfsdk/src/fpdfdoc.cpp:37: return > (CPDF_Dictionary*)tree.GetFirstChild((CPDF_Dictionary*)bookmark); > On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > > The cast of the return value doen't match the declared return type above. > > This is an operator in CPDF_Bookmark class. I removed it in another patch. > > https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... > fpdfsdk/src/fpdfdoc.cpp:46: return > (CPDF_Dictionary*)tree.GetNextSibling((CPDF_Dictionary*)bookmark); > On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > > ditto. > > ditto Thanks Bo! How do you plan to address Tom's comment about using std::string in the API? I think most of the other comments are more minor.
> > Thanks Bo! How do you plan to address Tom's comment about using std::string in > the API? I think most of the other comments are more minor. Actually we have a new struct FPDF_BSTR in the xfa branch: https://pdfium.googlesource.com/pdfium/+/87e9598a159d17a3b45821635b7ad43a18dd.... That's used a lot in the xfa branch and the original patch was using this one. But I feel using FPDF_BSTR over std:string does not really offer any benefit and should be get rid of in long run. But we may want to use it for now considering "extern C"?
> But I feel using FPDF_BSTR over std:string does not really offer any benefit and > should be get rid of in long run. But we may want to use it for now considering > "extern C"? Oddly, only part of the pdfview.h is under extern C. That's probably an issue in itself. The FPDF_BSTR would be appropriate if it moved under extern C.
On 2015/01/05 18:20:30, Tom Sepez (vacated ho ho ho) wrote: > > But I feel using FPDF_BSTR over std:string does not really offer any benefit > and > > should be get rid of in long run. But we may want to use it for now > considering > > "extern C"? > Oddly, only part of the pdfview.h is under extern C. That's probably an issue > in itself. The FPDF_BSTR would be appropriate if it moved under extern C. FPDF_BSTR is hard to use, it should be initialized with ::FPDF_BStr_Init before calling this API and released with ::FPDF_BStr_Clear.
https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:632: DLLEXPORT FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, std::string name); This seems like it is just an in-parameter, is that correct? If so, can we just use a char*? As an aside I'm a bit confused about this API since it has both an index and a name. What happens if I specify both? How would I go about iterating through all of the named destinations in the document? Thanks!!
https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:632: DLLEXPORT FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, std::string name); On 2015/01/05 22:38:09, raymes wrote: > This seems like it is just an in-parameter, is that correct? If so, can we just > use a char*? > > As an aside I'm a bit confused about this API since it has both an index and a > name. What happens if I specify both? How would I go about iterating through all > of the named destinations in the document? > > Thanks!! Ohh, my mistake, this should be passed by reference. Originally it passes FPDF_BSTR*. It intends to get the name of a particular index. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp File fpdfsdk/src/fpdfdoc.cpp (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:33: if (document == NULL) return NULL; On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: or just !document. Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:52: On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: or just !bookmark Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:53: CPDF_Bookmark Bookmark = (CPDF_Dictionary*)bookmark; On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: maybe pBookmark to avoid starting local variable names with a capital > letter. Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:57: if (buffer != NULL && buflen >= len + 2) { On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: (buffer && buflen >= len + 2) Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:59: ((FX_BYTE*)buffer)[len] = ((FX_BYTE*)buffer)[len + 1] = 0; On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: two separate statements. Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:794: CPDF_Document* pDoc = (CPDF_Document*)document; On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: blank line here if you want to use one-line ifs. Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:821: CPDF_Document* pDoc = (CPDF_Document*)document; On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: blank line. Done. https://codereview.chromium.org/834703002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:827: On 2015/01/01 01:10:08, Tom Sepez (vacated ho ho ho) wrote: > nit: no blank line Done.
https://codereview.chromium.org/834703002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:643: DLLEXPORT FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, std::string name); @Tom, @raymes, should we have something like: FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, char* name, int& len); and ask the application to manage the string buffer?
https://codereview.chromium.org/834703002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:643: DLLEXPORT FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, std::string name); On 2015/01/06 18:15:56, Bo Xu wrote: > @Tom, @raymes, should we have something like: > > FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, > char* name, int& len); > > and ask the application to manage the string buffer? Yes. But the problem is that the application doesn't know how big to make the buffer. There are a few ways to deal with this. 1. There's a fixed constant like MAX_NAMELEN somewhere that limits the length of possible dest names, it must always be enforced upon creation of these dests, and we require a buffer that big here. 2. We pass a length on input, and return an error if its too small. 3. We have a "query" mode where if |name| is null, we return the required space for the next call in |len|. Pick one of these. Thanks.
https://codereview.chromium.org/834703002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:643: DLLEXPORT FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, std::string name); On 2015/01/06 18:25:44, Tom Sepez wrote: > On 2015/01/06 18:15:56, Bo Xu wrote: > > @Tom, @raymes, should we have something like: > > > > FPDF_DEST STDCALL FPDF_GetNamedDest(FPDF_DOCUMENT document, int index, > > char* name, int& len); > > > > and ask the application to manage the string buffer? > Yes. But the problem is that the application doesn't know how big to make the > buffer. There are a few ways to deal with this. > > 1. There's a fixed constant like MAX_NAMELEN somewhere that limits the length of > possible dest names, it must always be enforced upon creation of these dests, > and we require a buffer that big here. > > 2. We pass a length on input, and return an error if its too small. > > 3. We have a "query" mode where if |name| is null, we return the required space > for the next call in |len|. > > Pick one of these. Thanks. > I use the query mode in patch 4. Can you take a look?
https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:857: CFX_WideString wsName = PDF_DecodeText(bsName); need to check that len was sufficient here to get the whole value.
On 2015/01/07 16:56:24, Tom Sepez wrote: > https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp > File fpdfsdk/src/fpdfview.cpp (right): > > https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp... > fpdfsdk/src/fpdfview.cpp:857: CFX_WideString wsName = PDF_DecodeText(bsName); > need to check that len was sufficient here to get the whole value. For that matter, Is there some sort of error indication?
On 2015/01/07 16:57:58, Tom Sepez wrote: > On 2015/01/07 16:56:24, Tom Sepez wrote: > > https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp > > File fpdfsdk/src/fpdfview.cpp (right): > > > > > https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp... > > fpdfsdk/src/fpdfview.cpp:857: CFX_WideString wsName = PDF_DecodeText(bsName); > > need to check that len was sufficient here to get the whole value. > > For that matter, Is there some sort of error indication? Maybe return |len| of -1 to indicate insufficient buffer?
Some comments below, but once these are addressed I'm ok with this and defer to tsepez https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:11: #include <string> nit: we don't need this now https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp File fpdfsdk/src/fpdfdoc.cpp (right): https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:32: if (!document) should you check if pDict is null here too (as in the function below)? https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:36: CPDF_Bookmark bookmark = CPDF_Bookmark((CPDF_Dictionary*)pDict); nit: indentation https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:46: CPDF_Bookmark bookmark = CPDF_Bookmark((CPDF_Dictionary*)pDict); nit: indentation (here and the next line) https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:52: if (!NULL) I think you want pDict here https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:57: unsigned long len = encodedTitle.GetLength(); nit: indentation
I have taken code of https://codereview.chromium.org/834703002/ and made build. And then I tried to access all the nameDests of http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf. at load time. I am getting total nameDest as 32, But FPDF_GetNamedDest() does not returns proper names. I am getting names as: [0110/040135:INFO:out_of_process_instance.cc(1336)] Deepak:32 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :0 page:A,3,18 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :1 page:A,3,32 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :2 page:A,3,17 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :3 page:B,3,31 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :4 page:B,3,20 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :5 page:B,3,21 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :6 page:C,3,7 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :7 page:C,3,16 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :8 page:C,3,30 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :9 page:C,3,6 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :10 page:D,3,5 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :11 page:D,3,15 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :12 page:E,3,10 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :13 page:F,3,3 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :14 page:G,3,2 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :15 page:G,3,29 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :16 page:H,3,14 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :17 page:I,3,28 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :18 page:I,3,27 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :19 page:I,3,9 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :20 page:J,3,4 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :21 page:K,3,13 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :22 page:M,3,8 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :23 page:N,3,19 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :24 page:N,3,25 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :25 page:R,3,26 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :26 page:S,3,12 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :27 page:S,3,24 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :28 page:T,3,23 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :29 page:T,3,11 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :30 page:U,3,0 [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :31 page:U,3,22 But I think we want to get the all the names instead of handles, so that using names we can get nameDest and then page index. at the time of documnet load. as when we select "Go back to the first page" from page 34 from above link then we get #US as the name, and from this we fetch nameDest by FPDF_GetNamedDestByName() and then from nameDest we get page index by FPDFDest_GetPageIndex(). So we should get all the names properly. We want all the names and thier corresponding page values get computed at document load time. Please correct me if I misunderstand something.
On 2015/01/09 10:35:23, Deepak wrote: > I have taken code of https://codereview.chromium.org/834703002/ and made build. > And then I tried to access all the nameDests of > http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf. > at load time. > I am getting total nameDest as 32, But FPDF_GetNamedDest() does not returns > proper names. > I am getting names as: > > [0110/040135:INFO:out_of_process_instance.cc(1336)] Deepak:32 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :0 page:A,3,18 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :1 page:A,3,32 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :2 page:A,3,17 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :3 page:B,3,31 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :4 page:B,3,20 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :5 page:B,3,21 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :6 page:C,3,7 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :7 page:C,3,16 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :8 page:C,3,30 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :9 page:C,3,6 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :10 page:D,3,5 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :11 page:D,3,15 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :12 page:E,3,10 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :13 page:F,3,3 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :14 page:G,3,2 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :15 page:G,3,29 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :16 page:H,3,14 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :17 page:I,3,28 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :18 page:I,3,27 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :19 page:I,3,9 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :20 page:J,3,4 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :21 page:K,3,13 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :22 page:M,3,8 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :23 page:N,3,19 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :24 page:N,3,25 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :25 page:R,3,26 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :26 page:S,3,12 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :27 page:S,3,24 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :28 page:T,3,23 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :29 page:T,3,11 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :30 page:U,3,0 > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :31 page:U,3,22 > > But I think we want to get the all the names instead of handles, so that using > names we can get nameDest and then page index. > at the time of documnet load. > as when we select "Go back to the first page" from page 34 from above link then > we get #US as the name, and from this we > fetch nameDest by FPDF_GetNamedDestByName() and then from nameDest we get page > index by FPDFDest_GetPageIndex(). > So we should get all the names properly. > We want all the names and thier corresponding page values get computed at > document load time. > Please correct me if I misunderstand something. Sorry I forget to tell meaning of logs: Deepak :0 page:A,3,18 0: index A: name I am getting 3: length 18: page number corresponding to this nameDest. As above we are getting same name for nameDest.so It segregation in ambiguish.
On 2015/01/09 10:39:23, Deepak wrote: > On 2015/01/09 10:35:23, Deepak wrote: > > I have taken code of https://codereview.chromium.org/834703002/ and made > build. > > And then I tried to access all the nameDests of > > http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf. > > at load time. > > I am getting total nameDest as 32, But FPDF_GetNamedDest() does not returns > > proper names. > > I am getting names as: > > > > [0110/040135:INFO:out_of_process_instance.cc(1336)] Deepak:32 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :0 page:A,3,18 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :1 page:A,3,32 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :2 page:A,3,17 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :3 page:B,3,31 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :4 page:B,3,20 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :5 page:B,3,21 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :6 page:C,3,7 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :7 page:C,3,16 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :8 page:C,3,30 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :9 page:C,3,6 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :10 page:D,3,5 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :11 page:D,3,15 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :12 page:E,3,10 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :13 page:F,3,3 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :14 page:G,3,2 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :15 page:G,3,29 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :16 page:H,3,14 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :17 page:I,3,28 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :18 page:I,3,27 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :19 page:I,3,9 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :20 page:J,3,4 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :21 page:K,3,13 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :22 page:M,3,8 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :23 page:N,3,19 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :24 page:N,3,25 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :25 page:R,3,26 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :26 page:S,3,12 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :27 page:S,3,24 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :28 page:T,3,23 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :29 page:T,3,11 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :30 page:U,3,0 > > [0110/040135:INFO:pdfium_engine.cc(2352)] Deepak :31 page:U,3,22 > > > > But I think we want to get the all the names instead of handles, so that using > > names we can get nameDest and then page index. > > at the time of documnet load. > > as when we select "Go back to the first page" from page 34 from above link > then > > we get #US as the name, and from this we > > fetch nameDest by FPDF_GetNamedDestByName() and then from nameDest we get page > > index by FPDFDest_GetPageIndex(). > > So we should get all the names properly. > > We want all the names and thier corresponding page values get computed at > > document load time. > > Please correct me if I misunderstand something. > > Sorry I forget to tell meaning of logs: > Deepak :0 page:A,3,18 > 0: index > A: name I am getting > 3: length > 18: page number corresponding to this nameDest. > > As above we are getting same name for nameDest.so It segregation in ambiguish. The name should be wchar_t* not char*, it's converted from utf16. I have updated the code correspondingly. Thanks!
Hi all, please take a look patch 6, thanks. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:11: #include <string> On 2015/01/08 03:55:44, raymes wrote: > nit: we don't need this now Done. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp File fpdfsdk/src/fpdfdoc.cpp (right): https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:32: if (!document) On 2015/01/08 03:55:44, raymes wrote: > should you check if pDict is null here too (as in the function below)? Done. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:36: CPDF_Bookmark bookmark = CPDF_Bookmark((CPDF_Dictionary*)pDict); On 2015/01/08 03:55:44, raymes wrote: > nit: indentation Done. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:46: CPDF_Bookmark bookmark = CPDF_Bookmark((CPDF_Dictionary*)pDict); On 2015/01/08 03:55:44, raymes wrote: > nit: indentation (here and the next line) Done. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:52: if (!NULL) On 2015/01/08 03:55:44, raymes wrote: > I think you want pDict here Done. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#... fpdfsdk/src/fpdfdoc.cpp:57: unsigned long len = encodedTitle.GetLength(); On 2015/01/08 03:55:44, raymes wrote: > nit: indentation Done. https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/834703002/diff/60001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:857: CFX_WideString wsName = PDF_DecodeText(bsName); On 2015/01/07 16:56:24, Tom Sepez wrote: > need to check that len was sufficient here to get the whole value. Return -1 if length not sufficient.
Bo, can you take a look at #c23 and decide whether the code is functioning properly?
On 2015/01/09 19:36:52, Tom Sepez wrote: > Bo, can you take a look at #c23 and decide whether the code is functioning > properly? Yep, I have verified locally, it worked. Previous char* is used which was wrong. The returned wchar buffer "47 00 48 00 ... " was truncated to the first char only
On 2015/01/09 19:39:55, Bo Xu wrote: > On 2015/01/09 19:36:52, Tom Sepez wrote: > > Bo, can you take a look at #c23 and decide whether the code is functioning > > properly? > > Yep, I have verified locally, it worked. Previous char* is used which was wrong. > The returned wchar buffer "47 00 48 00 ... " was truncated to the first char > only Great. Thanks. LGTM.
On 2015/01/09 20:48:03, Tom Sepez wrote:
> On 2015/01/09 19:39:55, Bo Xu wrote:
> > On 2015/01/09 19:36:52, Tom Sepez wrote:
> > > Bo, can you take a look at #c23 and decide whether the code is functioning
> > > properly?
> >
> > Yep, I have verified locally, it worked. Previous char* is used which was
> wrong.
> > The returned wchar buffer "47 00 48 00 ... " was truncated to the first char
> > only
>
> Great. Thanks. LGTM.
I am sorry but still, I am not able to get the proper data even afetr taking new
code.
The I am fetching data is as:
--
wchar_t* name = NULL;
unsigned long len = 0;
FPDF_DEST dest = NULL;
dest = FPDF_GetNamedDest(doc_,i,(void*)name,len);
if (dest) {
name = new wchar_t[len];
FPDF_DEST dest = FPDF_GetNamedDest(doc_,i,(void*)name,len);
if (dest) {
std::string temp;
bool rv = base::WideToUTF8(name,len,&temp);
LOG(INFO)<<"Deepak :"<<rv<<","<<temp<<","<<len;
}
delete []name;
}
--
Is I am doing something wrong for getting std::string data from 'name'
as I am getting
0,�������,8.
Can you please call this API from external code and print received value in that
external code.
Please correct me if I am missing something.
Thanks
On 2015/01/10 09:50:15, Deepak wrote:
> On 2015/01/09 20:48:03, Tom Sepez wrote:
> > On 2015/01/09 19:39:55, Bo Xu wrote:
> > > On 2015/01/09 19:36:52, Tom Sepez wrote:
> > > > Bo, can you take a look at #c23 and decide whether the code is
functioning
> > > > properly?
> > >
> > > Yep, I have verified locally, it worked. Previous char* is used which was
> > wrong.
> > > The returned wchar buffer "47 00 48 00 ... " was truncated to the first
char
> > > only
> >
> > Great. Thanks. LGTM.
>
> I am sorry but still, I am not able to get the proper data even afetr taking
new
> code.
> The I am fetching data is as:
> --
> wchar_t* name = NULL;
> unsigned long len = 0;
> FPDF_DEST dest = NULL;
> dest = FPDF_GetNamedDest(doc_,i,(void*)name,len);
> if (dest) {
> name = new wchar_t[len];
> FPDF_DEST dest = FPDF_GetNamedDest(doc_,i,(void*)name,len);
> if (dest) {
> std::string temp;
> bool rv = base::WideToUTF8(name,len,&temp);
> LOG(INFO)<<"Deepak :"<<rv<<","<<temp<<","<<len;
> }
> delete []name;
> }
> --
> Is I am doing something wrong for getting std::string data from 'name'
> as I am getting
> 0,�������,8.
>
> Can you please call this API from external code and print received value in
that
> external code.
> Please correct me if I am missing something.
> Thanks
Sorry to bothering you ..
I found the way to fetch data properly.
Thanks.
Please roll in these changes. My changes are depends on these, as I am consumer of these chnages :) Thanks
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 476cd69a6f5c5096a3145e0c4d567010f37739c3 (presubmit successful).
Message was sent while issue was closed.
On 2015/01/11 06:38:03, Deepak wrote: > Please roll in these changes. > My changes are depends on these, as I am consumer of these chnages :) > Thanks CL committed :)
Message was sent while issue was closed.
On 2015/01/11 06:53:31, Bo Xu wrote: > On 2015/01/11 06:38:03, Deepak wrote: > > Please roll in these changes. > > My changes are depends on these, as I am consumer of these chnages :) > > Thanks > > CL committed :) Can we get this merged to XFA branch? Thanks.
Message was sent while issue was closed.
On 2015/01/16 02:19:48, Tom Sepez wrote: > On 2015/01/11 06:53:31, Bo Xu wrote: > > On 2015/01/11 06:38:03, Deepak wrote: > > > Please roll in these changes. > > > My changes are depends on these, as I am consumer of these chnages :) > > > Thanks > > > > CL committed :) > > Can we get this merged to XFA branch? Thanks. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
