Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1960193003:
pdfium_test: print out some annotations
|
Created:
4 years, 7 months ago by hal.canary Modified:
4 years, 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. |
Descriptionpdfium_test: print out some annotations
Also, add FPDFDest_GetLocationInPage to public API.
Motivation: to be used for testing PDFs produced by Chromium/SkPDF.
Patch Set 1 : 2016-05-10 (Tuesday) 09:00:37 EDT #Patch Set 2 : 2016-05-13 (Friday) 16:05:57 EDT #
Total comments: 12
Patch Set 3 : 2016-05-20 (Friday) 20:39:39 EDT #
Total comments: 5
Messages
Total messages: 32 (13 generated)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1960193003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1960193003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
Patchset #1 (id:1) has been deleted
halcanary@google.com changed reviewers: + jam@chromium.org, jam@google.com
PTAL
I don't work much on pdfium now, probably best to send this to some of the folks who work daily on it
Description was changed from ========== pdfium_test: print out some annotations Used for testing PDFs produced by Chromium/SkPDF. Also, add FPDFDest_GetLocationInPage to public API. ========== to ========== pdfium_test: print out some annotations Also, add FPDFDest_GetLocationInPage to public API. Motivation: to be used for testing PDFs produced by Chromium/SkPDF. ==========
halcanary@google.com changed reviewers: + dsinclair@chromium.org, dsinclair@google.com - jam@chromium.org, jam@google.com
PTAL, when you can.
https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp File fpdfsdk/fpdfdoc.cpp (right): https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp#new... fpdfsdk/fpdfdoc.cpp:227: return FALSE; nit: blank line after non-{}'d statement. https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp#new... fpdfsdk/fpdfdoc.cpp:231: } nit: no {}'s on single line bodies (and below) https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp#new... fpdfsdk/fpdfdoc.cpp:240: } const CPDF_Number* objX = ToNumber(dest->GetDirectObjectAt(2)); const CPDF_Number* objY = ToNumber(dest->getDirectObjectAt(3)); if (!objX || !objY) return FALSE; https://codereview.chromium.org/1960193003/diff/40001/public/fpdf_doc.h File public/fpdf_doc.h (right): https://codereview.chromium.org/1960193003/diff/40001/public/fpdf_doc.h#newco... public/fpdf_doc.h:181: // Returns TRUE in success. nit: s/in/on https://codereview.chromium.org/1960193003/diff/40001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1960193003/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:350: } else if (cur_arg == "--show_annotations") { show-annotations would better match send-events I think. https://codereview.chromium.org/1960193003/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:791: " --show_annotations - print information about some\n" some or all? (and if some, can you put what those are here?)
dsinclair@chromium.org changed reviewers: - dsinclair@google.com
https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp File fpdfsdk/fpdfdoc.cpp (right): https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp#new... fpdfsdk/fpdfdoc.cpp:227: return FALSE; On 2016/05/16 13:56:44, dsinclair wrote: > nit: blank line after non-{}'d statement. Done. https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp#new... fpdfsdk/fpdfdoc.cpp:231: } On 2016/05/16 13:56:44, dsinclair wrote: > nit: no {}'s on single line bodies (and below) Done. https://codereview.chromium.org/1960193003/diff/40001/fpdfsdk/fpdfdoc.cpp#new... fpdfsdk/fpdfdoc.cpp:240: } On 2016/05/16 13:56:44, dsinclair wrote: > const CPDF_Number* objX = ToNumber(dest->GetDirectObjectAt(2)); > const CPDF_Number* objY = ToNumber(dest->getDirectObjectAt(3)); > if (!objX || !objY) > return FALSE; Done. https://codereview.chromium.org/1960193003/diff/40001/public/fpdf_doc.h File public/fpdf_doc.h (right): https://codereview.chromium.org/1960193003/diff/40001/public/fpdf_doc.h#newco... public/fpdf_doc.h:181: // Returns TRUE in success. On 2016/05/16 13:56:44, dsinclair wrote: > nit: s/in/on Done. https://codereview.chromium.org/1960193003/diff/40001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1960193003/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:350: } else if (cur_arg == "--show_annotations") { On 2016/05/16 13:56:45, dsinclair wrote: > show-annotations would better match send-events I think. Done. https://codereview.chromium.org/1960193003/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:791: " --show_annotations - print information about some\n" On 2016/05/16 13:56:45, dsinclair wrote: > some or all? (and if some, can you put what those are here?) How's this: --show-annotations - print information about URL and internal link annotations in the document.
dsinclair@chromium.org changed reviewers: + thestig@chromium.org
lgtm. thestig@ FYI as to the new api.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1960193003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1960193003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_xfa_64_rel_gn on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_64_rel_...)
https://codereview.chromium.org/1960193003/diff/60001/public/fpdf_doc.h File public/fpdf_doc.h (right): https://codereview.chromium.org/1960193003/diff/60001/public/fpdf_doc.h#newco... public/fpdf_doc.h:175: // destination is in [page /XYZ x y zoom] syntax. What about the zoom value? https://codereview.chromium.org/1960193003/diff/60001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1960193003/diff/60001/samples/pdfium_test.cc#... samples/pdfium_test.cc:494: FPDF_PAGE page = FPDF_LoadPage(doc, page_index); Why not put the ShowAnnotations() call inside RenderPage() after it has already loaded the page and triggered the onload form actions? https://codereview.chromium.org/1960193003/diff/60001/samples/pdfium_test.cc#... samples/pdfium_test.cc:508: auto len = FPDFAction_GetURIPath(doc, action, nullptr, 0); write out unsigned int instead of using auto? https://codereview.chromium.org/1960193003/diff/60001/samples/pdfium_test.cc#... samples/pdfium_test.cc:511: printf(" <%g,%g,%g,%g> uri: \"%.*s\"\n", rect.left, rect.top, I see 5 conversion specifiers and 6 variables. https://codereview.chromium.org/1960193003/diff/60001/samples/pdfium_test.cc#... samples/pdfium_test.cc:512: rect.right, rect.bottom, (int)buffer.size(), &buffer[0]); Borrow PRIuS from base/format_macros.h for size_t if needed?
On 2016/05/23 13:59:36, dsinclair wrote: > lgtm. > > thestig@ FYI as to the new api. I was hoping we didn't need any new APIs until I finished the C++ conversion, but no worries. It would be good to add tests for the new APIs. There's some existing PDFs with dests in testing/resources.
On 2016/05/23 16:56:38, Lei Zhang wrote: > On 2016/05/23 13:59:36, dsinclair wrote: > > lgtm. > > > > thestig@ FYI as to the new api. > > I was hoping we didn't need any new APIs until I finished the C++ conversion, > but no worries. It would be good to add tests for the new APIs. There's some > existing PDFs with dests in testing/resources. Do you have an idea of when the API conversion will be done?
On 2016/05/24 00:26:42, dsinclair wrote: > Do you have an idea of when the API conversion will be done? Depends on how hard I work? I can always try harder? :) Hal, do you need this right away?
On 2016/05/24 00:27:50, Lei Zhang wrote: > On 2016/05/24 00:26:42, dsinclair wrote: > > Do you have an idea of when the API conversion will be done? > > Depends on how hard I work? I can always try harder? :) > > Hal, do you need this right away? This is only one part of a whole chain of things that must be done to test annotations. I need to automate the process of creating input PDFs, too.
dsinclair@chromium.org changed reviewers: - dsinclair@chromium.org
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
Hal, are you still interested in this CL? If so, want to sync it up to master and take a look at the comments thestig@ left on the CL? If not, let me know and I can see about getting it updated and landed.
On 2016/11/01 21:11:57, dsinclair wrote: > Hal, are you still interested in this CL? If so, want to sync it up to master > and take a look at the comments thestig@ left on the CL? > > If not, let me know and I can see about getting it updated and landed. Why don't you take the ball and run with it?
On 2016/11/03 18:51:15, Hal Canary wrote: > On 2016/11/01 21:11:57, dsinclair wrote: > > Hal, are you still interested in this CL? If so, want to sync it up to master > > and take a look at the comments thestig@ left on the CL? > > > > If not, let me know and I can see about getting it updated and landed. > > Why don't you take the ball and run with it? The GetLocationInPage part of this is landed. I skipped the pdfium_test changes for now, let me know if you need them and I can look into making a second CL.
On 2016/11/09 20:29:10, dsinclair wrote: > On 2016/11/03 18:51:15, Hal Canary wrote: > > On 2016/11/01 21:11:57, dsinclair wrote: > > > Hal, are you still interested in this CL? If so, want to sync it up to > master > > > and take a look at the comments thestig@ left on the CL? > > > > > > If not, let me know and I can see about getting it updated and landed. > > > > Why don't you take the ball and run with it? > > > The GetLocationInPage part of this is landed. I skipped the pdfium_test changes > for now, let me know if you need them and I can look into making a second CL. I'll get to it later. Thanks!
dsinclair@chromium.org changed reviewers: - dsinclair@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
