Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476733003:
OOP PDF - Add support for "page" open pdf parameter (Closed)
DescriptionOOP PDF - Add support for "page" open pdf parameter
This patch adds support for "page" feature of open PDF
parameters in OOP PDF.
When page is used as an open PDF parameter, then PDF
should initially be loaded at the page specified by user.
BUG=64309
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290529
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review feedback (removed new message for openPDFParams) #
Total comments: 6
Patch Set 3 : Review feedback (nit fixes) #
Total comments: 4
Patch Set 4 : Review feedback (hasParams -> paramIndex) #
Messages
Total messages: 18 (0 generated)
PTAL at the changes to support "page" feature for OOP PDF in pdf.js. [1] This doesn't have support for "nameddest". If it's acceptable, then I'm planning to add it separately once we have identified correct way to add features through this review. [2] I've not yet removed GetInitialPage() from out_of_process_instance.cc. I'll remove it in next iteration of this patch when I incorporate review comments. PTAL. Thanks!
https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:306: }, I would suggest just inlining this function. viewport_.goToPage should already check value > 0 so you don't need that check. https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:322: // FIXME: Send message for GetNamedDestinationPage(); Are you going to add support for this soon? If not we might as well just remove these lines in this CL. Also, when you do go to implement this, I would suggest sending a single message from the plugin to the JS when the document is loaded which contains all of the named pages, and then storing it as a member variable in this object. https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:420: this.handleOpenPDFParams_(); Rather than calling this here, we can get rid of the new message you added and just call this function inside updateProgress() (if progress == 100%) https://codereview.chromium.org/476733003/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/476733003/diff/1/pdf/out_of_process_instance.... pdf/out_of_process_instance.cc:1081: PostMessage(message); I don't think we need to add this new message type. https://codereview.chromium.org/476733003/diff/1/pdf/out_of_process_instance.... pdf/out_of_process_instance.cc:1309: int OutOfProcessInstance::GetInitialPage(const std::string& url) { I think we can get rid of this entire function from this file, hooray!
Thanks for reviewing this! I've made changes as per review comments. PTAL. Thanks! https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:306: }, On 2014/08/15 01:07:18, raymes wrote: > I would suggest just inlining this function. viewport_.goToPage should already > check value > 0 so you don't need that check. Done. https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:322: // FIXME: Send message for GetNamedDestinationPage(); On 2014/08/15 01:07:18, raymes wrote: > Are you going to add support for this soon? If not we might as well just remove > these lines in this CL. > > Also, when you do go to implement this, I would suggest sending a single message > from the plugin to the JS when the document is loaded which contains all of the > named pages, and then storing it as a member variable in this object. Yes, I'm planning to add support for this soon in next CL. I've removed these lines for now. Based on your suggestion, I think we also need to add an interface in pdfium engine that provides all named destinations. Currently, we only have GetNamedDestinationPage() which returns page index for a particular destination. Please let me know if it makes sense, and I'll check how to support this. https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:420: this.handleOpenPDFParams_(); On 2014/08/15 01:07:17, raymes wrote: > Rather than calling this here, we can get rid of the new message you added and > just call this function inside updateProgress() (if progress == 100%) Done. https://codereview.chromium.org/476733003/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/476733003/diff/1/pdf/out_of_process_instance.... pdf/out_of_process_instance.cc:1081: PostMessage(message); On 2014/08/15 01:07:18, raymes wrote: > I don't think we need to add this new message type. Done. https://codereview.chromium.org/476733003/diff/1/pdf/out_of_process_instance.... pdf/out_of_process_instance.cc:1309: int OutOfProcessInstance::GetInitialPage(const std::string& url) { On 2014/08/15 01:07:18, raymes wrote: > I think we can get rid of this entire function from this file, hooray! Done. https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:312: this.handleOpenPDFParams_(); Is there a way to check whether we are in PrintPreview mode or not? This call should be protected by such a check.
https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:312: this.handleOpenPDFParams_(); Maybe I can introduce isPrintPreview() here? Similar to OutOfProcessInstance::IsPrintPreview() https://code.google.com/p/chromium/codesearch#chromium/src/pdf/out_of_process...
lgtm https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:322: // FIXME: Send message for GetNamedDestinationPage(); I had a look at the API that pdfium exposes and it doesn't look like we can get a list of named destinations :(. So we might need to do what you were proposing and add a new message which has a reply with the page number. https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:268: * Handle open PDF parameters. nit: Maybe we can elaborate on the comment saying that all these parameters are contained in the URL and include a link to the spec? https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:312: this.handleOpenPDFParams_(); I think it should be ok not to do any check here. The reason is that the URL should never contain these parameters in print preview mode. Let's leave out the check for now :)
PTAL. Thanks! https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:322: // FIXME: Send message for GetNamedDestinationPage(); On 2014/08/18 00:39:34, raymes wrote: > I had a look at the API that pdfium exposes and it doesn't look like we can get > a list of named destinations :(. So we might need to do what you were proposing > and add a new message which has a reply with the page number. I'll check it further. https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:268: * Handle open PDF parameters. On 2014/08/18 00:39:34, raymes wrote: > nit: Maybe we can elaborate on the comment saying that all these parameters are > contained in the URL and include a link to the spec? Done. https://codereview.chromium.org/476733003/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:312: this.handleOpenPDFParams_(); On 2014/08/18 00:39:34, raymes wrote: > I think it should be ok not to do any check here. The reason is that the URL > should never contain these parameters in print preview mode. Let's leave out the > check for now :) Acknowledged.
https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:274: var hasParams = originalUrl.search('#'); hasParams -> paramIndex? https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:291: this.viewport_.goToPage(paramsDictionary['page'] - 1); The current instance.cc code skips this if the page number is out of range.
On Tue, Aug 19, 2014 at 9:53 AM, <thestig@chromium.org> wrote: > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > File chrome/browser/resources/pdf/pdf.js (right): > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:274: var hasParams = > originalUrl.search('#'); > hasParams -> paramIndex? > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:291: > this.viewport_.goToPage(paramsDictionary['page'] - 1); > The current instance.cc code skips this if the page number is out of > range. Viewport.goToPage should handle that case. > > https://codereview.chromium.org/476733003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/19 03:44:22, raymes wrote: > On Tue, Aug 19, 2014 at 9:53 AM, <mailto:thestig@chromium.org> wrote: > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > > chrome/browser/resources/pdf/pdf.js:291: > > this.viewport_.goToPage(paramsDictionary['page'] - 1); > > The current instance.cc code skips this if the page number is out of > > range. > > Viewport.goToPage should handle that case. Viewport.goToPage will clip the page to 0 or N and go to that page. If you don't call goToPage, then on a reload, you'll stay on the page you were viewing, right? We should check what Acrobat does on a reload.
Oh sorry, I see the distinction. I think clipping it makes more sense haha! But doing what acrobat does is fine too :) On Tue, Aug 19, 2014 at 2:46 PM, <thestig@chromium.org> wrote: > On 2014/08/19 03:44:22, raymes wrote: > >> On Tue, Aug 19, 2014 at 9:53 AM, <mailto:thestig@chromium.org> wrote: > > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... >> >> > chrome/browser/resources/pdf/pdf.js:291: >> > this.viewport_.goToPage(paramsDictionary['page'] - 1); >> > The current instance.cc code skips this if the page number is out of >> > range. > > >> Viewport.goToPage should handle that case. > > > Viewport.goToPage will clip the page to 0 or N and go to that page. If you > don't > call goToPage, then on a reload, you'll stay on the page you were viewing, > right? We should check what Acrobat does on a reload. > > https://codereview.chromium.org/476733003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
FWIW, here's the behavior that I observed - [*] Firefox loads first page whenever an out of range page number is specified in parameter. Tested on Firefox 25.01 and 30.0. [*] Adobe (_not_ latest) shows an error dialog box saying there was an error in opening the page because of bad parameter.
PTAL. Thanks! https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:274: var hasParams = originalUrl.search('#'); On 2014/08/18 23:53:32, Lei Zhang wrote: > hasParams -> paramIndex? Done. https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:291: this.viewport_.goToPage(paramsDictionary['page'] - 1); On 2014/08/18 23:53:32, Lei Zhang wrote: > The current instance.cc code skips this if the page number is out of range. Please let me know how do you think this should be handled - [1] Ignore #page when page number is out of range (same as Firefox and instance.cc). [2] Maintain current behavior and clip the page (based on viewport_.goToPage). I'm inclining towards [1], but don't have a strong preference.
I'm ok either way. If thestig@ has a preference let's go with that or I'll leave it to you to decide :) On Tue, Aug 19, 2014 at 3:28 PM, <n.bansal@samsung.com> wrote: > PTAL. Thanks! > > > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > File chrome/browser/resources/pdf/pdf.js (right): > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:274: var hasParams = > originalUrl.search('#'); > On 2014/08/18 23:53:32, Lei Zhang wrote: >> >> hasParams -> paramIndex? > > > Done. > > > https://codereview.chromium.org/476733003/diff/40001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:291: > this.viewport_.goToPage(paramsDictionary['page'] - 1); > On 2014/08/18 23:53:32, Lei Zhang wrote: >> >> The current instance.cc code skips this if the page number is out of > > range. > > Please let me know how do you think this should be handled - > > [1] Ignore #page when page number is out of range (same as Firefox and > instance.cc). > [2] Maintain current behavior and clip the page (based on > viewport_.goToPage). > > I'm inclining towards [1], but don't have a strong preference. > > https://codereview.chromium.org/476733003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/19 05:17:27, Nikhil wrote: > FWIW, here's the behavior that I observed - > [*] Firefox loads first page whenever an out of range page number is specified > in parameter. Tested on Firefox 25.01 and 30.0. > [*] Adobe (_not_ latest) shows an error dialog box saying there was an error in > opening the page because of bad parameter. In Adobe Acrobat 11 inside IE 11, it does show the error dialog, but it also seems to clip the page number (i.e. going to page=3 on a 2 page PDF shows an error dialog but goes to page 2) as currently implemented, so LGTM!
Awesome! Let's take "page" parameter OOP then :) I'm going to submit this now. Thank you for reviewing this!
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/476733003/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as 290529 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
