Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495333002:
Add --pages option in pdfium_test so it renders only some of the pages. (Closed)
DescriptionAdd --pages option in pdfium_test so it renders only some of the pages.
The --pages option is set up to be 0-indexed.
--pages=<number> will render the specific page.
--pages=<number>-<number> will render the range of pages given.
Committed: https://pdfium.googlesource.com/pdfium/+/e3c731526c0464822758a2a8b3a9ca9527ce9272
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use else #
Total comments: 2
Patch Set 3 : Rename firstP, lastP #
Total comments: 4
Messages
Total messages: 19 (9 generated)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add --pages option in pdfium_test so it renders only some of the pages. The --pages option is set up to be 0-indexed. --pages=<number> will render the specific page. --pages=<number>-<number> will render the range of pages given. ========== to ========== Add --pages option in pdfium_test so it renders only some of the pages. The --pages option is set up to be 0-indexed. --pages=<number> will render the specific page. --pages=<number>-<number> will render the range of pages given. ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org, weili@chromium.org
PTAL I've been needing this option for a while now :) Hopefully some of you might find it useful too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc#newc... samples/pdfium_test.cc:80: int firstP; size_t? https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc#newc... samples/pdfium_test.cc:486: } Doesn't this need an else? https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc#newc... samples/pdfium_test.cc:801: int lastPage = options.pages ? options.lastP + 1 : page_count; Why +1?
https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc#newc... samples/pdfium_test.cc:80: int firstP; On 2016/11/14 16:35:27, dsinclair wrote: > size_t? RenderPage receives an int param. I don't think changing that is needed. https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc#newc... samples/pdfium_test.cc:486: } On 2016/11/14 16:35:27, dsinclair wrote: > Doesn't this need an else? Done. https://codereview.chromium.org/2495333002/diff/1/samples/pdfium_test.cc#newc... samples/pdfium_test.cc:801: int lastPage = options.pages ? options.lastP + 1 : page_count; On 2016/11/14 16:35:27, dsinclair wrote: > Why +1? To make the range inclusive. See the "i < lastPage" below.
lgtm w/ nit. https://codereview.chromium.org/2495333002/diff/20001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2495333002/diff/20001/samples/pdfium_test.cc#... samples/pdfium_test.cc:81: int lastP; Can we make these firstPage and lastPage?
https://codereview.chromium.org/2495333002/diff/20001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2495333002/diff/20001/samples/pdfium_test.cc#... samples/pdfium_test.cc:81: int lastP; On 2016/11/14 16:45:38, dsinclair wrote: > Can we make these firstPage and lastPage? Done.
The CQ bit was checked by npm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org Link to the patchset: https://codereview.chromium.org/2495333002/#ps40001 (title: "Rename firstP, lastP")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add --pages option in pdfium_test so it renders only some of the pages. The --pages option is set up to be 0-indexed. --pages=<number> will render the specific page. --pages=<number>-<number> will render the range of pages given. ========== to ========== Add --pages option in pdfium_test so it renders only some of the pages. The --pages option is set up to be 0-indexed. --pages=<number> will render the specific page. --pages=<number>-<number> will render the range of pages given. Committed: https://pdfium.googlesource.com/pdfium/+/e3c731526c0464822758a2a8b3a9ca9527ce... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/e3c731526c0464822758a2a8b3a9ca9527ce...
Message was sent while issue was closed.
https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:80: int firstPage; first_page, last_page https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:803: int firstPage = options.pages ? options.firstPage : 0; If the user passes in --pages=1-2, they actually get pages 2-3. Intentional?
Message was sent while issue was closed.
https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:80: int firstPage; On 2016/11/14 19:09:23, Lei Zhang (not reviewing code) wrote: > first_page, last_page Hmm I missed the format https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc#... samples/pdfium_test.cc:803: int firstPage = options.pages ? options.firstPage : 0; On 2016/11/14 19:09:23, Lei Zhang (not reviewing code) wrote: > If the user passes in --pages=1-2, they actually get pages 2-3. Intentional? This is because it is 0-indexed. This is consistent with the naming of the png files you get when you use "--png". Maybe we should make all of it be 1-indexed since this is more natural considering how pdf viewers usually number the pages?
Message was sent while issue was closed.
On 2016/11/14 20:14:15, npm wrote: > https://codereview.chromium.org/2495333002/diff/40001/samples/pdfium_test.cc#... > samples/pdfium_test.cc:803: int firstPage = options.pages ? options.firstPage : > 0; > On 2016/11/14 19:09:23, Lei Zhang (not reviewing code) wrote: > > If the user passes in --pages=1-2, they actually get pages 2-3. Intentional? > > This is because it is 0-indexed. This is consistent with the naming > of the png files you get when you use "--png". Maybe we should make > all of it be 1-indexed since this is more natural considering how pdf > viewers usually number the pages? Let's not rename all the expectation files in the entire corpus. Instead just make kUsageString print out "page numbers are 0-based" and add the same comment for |first_page| / |last_page|. |
