Implement Page Selection in PDF Toolbar

Implement Page Selection in PDF Toolbar

BUG=110020
Committed: https://crrev.com/06c4b8c9dc292f0e23a4bb128d87165079557e3e
Cr-Commit-Position: refs/heads/master@{#313841}

[Alexandre Carlton, 2015-01-20 07:08:28]

alexandrec@chromium.org changed reviewers:
+ raymes@chromium.org, sammc@chromium.org

[Alexandre Carlton, 2015-01-20 07:08:28]

First version of page indicator using Material Design.

[Sam McNally, 2015-01-21 04:03:38]

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js
(right):

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js:6:
pageNo: '1',
Can you add comments for these fields. In particular, the relationship between
pageNo, chosenPageNo and index.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js:10:
pageLabels: null,
Is this used?

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js:15:
var page = parseInt(this.chosenPageNo) - 1;
What happens if it isn't a number?

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/index-material.html (right):

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:6: <link rel="import"
href="elements/pdf-page-indicator/pdf-page-indicator.html">
Maybe we should call it viewer-page-indicator to match the other custom
elements.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:300: goToNextPage_: function() {
Are these used?

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:463: this.pageIndicator_.docLength =
Indentation.

[Alexandre Carlton, 2015-01-21 05:20:35]

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js
(right):

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js:6:
pageNo: '1',
On 2015/01/21 04:03:37, Sam McNally wrote:
> Can you add comments for these fields. In particular, the relationship between
> pageNo, chosenPageNo and index.

Done.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js:10:
pageLabels: null,
On 2015/01/21 04:03:37, Sam McNally wrote:
> Is this used?

Yes, for Print Preview.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-page-indicator/pdf-page-indicator.js:15:
var page = parseInt(this.chosenPageNo) - 1;
On 2015/01/21 04:03:37, Sam McNally wrote:
> What happens if it isn't a number?

Done.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/index-material.html (right):

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:6: <link rel="import"
href="elements/pdf-page-indicator/pdf-page-indicator.html">
On 2015/01/21 04:03:38, Sam McNally wrote:
> Maybe we should call it viewer-page-indicator to match the other custom
> elements.

As discussed it is now page-selector.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:300: goToNextPage_: function() {
On 2015/01/21 04:03:38, Sam McNally wrote:
> Are these used?

Removed.

https://codereview.chromium.org/861673002/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:463: this.pageIndicator_.docLength =
On 2015/01/21 04:03:38, Sam McNally wrote:
> Indentation.

Done.

[Alexandre Carlton, 2015-01-21 06:40:30]

alexandrec@chromium.org changed reviewers:
+ estade@chromium.org

[Alexandre Carlton, 2015-01-21 06:40:31]

estade@chromium.org: Please review changes in 

chrome/browser/resources/component_extension_resources.grd
ui/webui/resources/polymer_resources.grdp

Thank you.

[Evan Stade, 2015-01-21 20:43:49]

lgtm

[Sam McNally, 2015-01-22 06:22:04]

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:6: pageNo:
'1', // The current entry in the input field.
Prefer putting comments on their own lines. For same-line comments, leave two
spaces before the comment.

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:16: if
(!isNaN(page))
I think we want to fall into the else below if the submitted page is invalid.

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:17:
this.fire('changePage', {page: page - 1});
This isn't always correct if pageLabels is set. This probably isn't worth fixing
now as we haven't decided whether we'll use this for print preview, but we
should leave a TODO about it.

[Alexandre Carlton, 2015-01-22 06:42:59]

alexandrec@chromium.org changed reviewers:
+ benwells@chromium.org

[Alexandre Carlton, 2015-01-22 06:43:00]

Add benwells@ as reviewer.

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:6: pageNo:
'1', // The current entry in the input field.
On 2015/01/22 06:22:04, Sam McNally wrote:
> Prefer putting comments on their own lines. For same-line comments, leave two
> spaces before the comment.

Done.

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:16: if
(!isNaN(page))
On 2015/01/22 06:22:04, Sam McNally wrote:
> I think we want to fall into the else below if the submitted page is invalid.

Done.

https://codereview.chromium.org/861673002/diff/220001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:17:
this.fire('changePage', {page: page - 1});
On 2015/01/22 06:22:04, Sam McNally wrote:
> This isn't always correct if pageLabels is set. This probably isn't worth
fixing
> now as we haven't decided whether we'll use this for print preview, but we
> should leave a TODO about it.

Done.

[raymes, 2015-01-22 22:31:55]

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.css
(right):

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.css:24: }
nit: blank line after this

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:1: <link
rel="import" href="chrome://resources/polymer/polymer/polymer.html">
do you need this? Will it get included in the top level page? Or should it be
included here as well

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:6: pageNo:
'1', // The current entry in the input field.
nit: double space before "//"

[raymes, 2015-01-22 22:37:56]

I had a look and it looks pretty good to me :) I defer to Ben/Sam

[Alexandre Carlton, 2015-01-22 22:50:29]

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.css
(right):

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.css:24: }
On 2015/01/22 22:31:55, raymes wrote:
> nit: blank line after this

Done.

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:1: <link
rel="import" href="chrome://resources/polymer/polymer/polymer.html">
On 2015/01/22 22:31:55, raymes wrote:
> do you need this? Will it get included in the top level page? Or should it be
> included here as well

The tutorial (https://www.polymer-project.org/docs/start/tutorial/step-2.html)
imports polymer.html in the custom element, though it does not do so in
index.html. Where should polymer.html be imported?

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/200001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:6: pageNo:
'1', // The current entry in the input field.
On 2015/01/22 22:31:55, raymes wrote:
> nit: double space before "//"

I've placed the comments on their own lines.

[benwells, 2015-01-27 02:01:41]

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.css
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.css:30:
paper-input::shadow input[is='core-input'] {
Why just ::shadow paper-inputs? Is there a specific one you want this to apply
to?

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:4:
<polymer-element name="page-selector" attributes="pageNo docLength
chosenPageNo">
Is there no line length limit in html?

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:7:
<paper-input value="{{pageNo}}" committedValue="{{chosenPageNo}}"
id="pageselector"></paper-input>
Same here. Is this valid html style?

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:13:
pageLabels: null,
Could you add more comment for all of these fields?

In particular:
- pageNo and chosenPageNo being 1 based should be pointed out
- index is 0 based should be pointed out
- what are the pageLabels? E.g. what types are they and what are they used for
- what does digitLength do? (actually should this just be a constant, or can it
be set?)

Also I *think* only some of these are exposed. Should they be highlighted in
some way? Should the non-exposed ones have a trailing underscore?

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:19: //
TODO(alexandrec): This is not correct if pageLabels is set.
This TODO confuses me. Is the TODO to handle pageLabels being non-null and
having a label entered in the else statement?

[benwells, 2015-01-27 02:10:56]

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:5:
<template>
Should this template tag and the script tag be indented?

[Alexandre Carlton, 2015-01-27 04:04:54]

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.css
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.css:30:
paper-input::shadow input[is='core-input'] {
On 2015/01/27 02:01:41, benwells wrote:
> Why just ::shadow paper-inputs? Is there a specific one you want this to apply
> to?

This specificity allows me to enforce my styling within the shadow DOM.

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:4:
<polymer-element name="page-selector" attributes="pageNo docLength
chosenPageNo">
On 2015/01/27 02:01:41, benwells wrote:
> Is there no line length limit in html?

The Google style guide doesn't specify anything about line length, and the
linter did not pick anything up. I've set it to 80 characters for now.

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:5:
<template>
On 2015/01/27 02:10:56, benwells wrote:
> Should this template tag and the script tag be indented?

Other viewer-elements (e.g. viewer-button.html) do not indent the <template> and
<script> tags.

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:13:
pageLabels: null,
On 2015/01/27 02:01:41, benwells wrote:
> Could you add more comment for all of these fields?
> 
> In particular:
> - pageNo and chosenPageNo being 1 based should be pointed out
> - index is 0 based should be pointed out
> - what are the pageLabels? E.g. what types are they and what are they used for
> - what does digitLength do? (actually should this just be a constant, or can
it
> be set?)
> 
> Also I *think* only some of these are exposed. Should they be highlighted in
> some way? Should the non-exposed ones have a trailing underscore?

digitLength is used in setting the length of the field; I haven't yet found a
way to make the input field grow otherwise. Is there a better way?

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:19: //
TODO(alexandrec): This is not correct if pageLabels is set.
On 2015/01/27 02:01:41, benwells wrote:
> This TODO confuses me. Is the TODO to handle pageLabels being non-null and
> having a label entered in the else statement?

This is more to do with Print Preview; I've modified the TODO statement.

[benwells, 2015-01-27 04:54:52]

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:5:
<template>
On 2015/01/27 04:04:54, Alexandre Carlton wrote:
> On 2015/01/27 02:10:56, benwells wrote:
> > Should this template tag and the script tag be indented?
> 
> Other viewer-elements (e.g. viewer-button.html) do not indent the <template>
and
> <script> tags.

Ah. Another change of yours I think I just lg'd had them indented.

https://codereview.chromium.org/861673002/diff/300001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/300001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:15: // An
array of the page numbers.
Please explain more how this is used by print preview. Also, it is only
half-used currently. I think it would be better to just ignore it completely and
add a TODO to take it into account if this is ever used in print preview.

[Alexandre Carlton, 2015-01-27 05:34:50]

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.html
(right):

https://codereview.chromium.org/861673002/diff/260001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.html:5:
<template>
On 2015/01/27 04:54:51, benwells wrote:
> On 2015/01/27 04:04:54, Alexandre Carlton wrote:
> > On 2015/01/27 02:10:56, benwells wrote:
> > > Should this template tag and the script tag be indented?
> > 
> > Other viewer-elements (e.g. viewer-button.html) do not indent the <template>
> and
> > <script> tags.
> 
> Ah. Another change of yours I think I just lg'd had them indented.

That was in Issue 864753002, I'll fix that now.

https://codereview.chromium.org/861673002/diff/300001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/page-selector/page-selector.js
(right):

https://codereview.chromium.org/861673002/diff/300001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/page-selector/page-selector.js:15: // An
array of the page numbers.
On 2015/01/27 04:54:51, benwells wrote:
> Please explain more how this is used by print preview. Also, it is only
> half-used currently. I think it would be better to just ignore it completely
and
> add a TODO to take it into account if this is ever used in print preview.

I've removed pageLabels completely; it should not be hard to re-implement should
we need to add in Print Preview functionality.

[benwells, 2015-01-27 05:36:40]

lgtm

[Sam McNally, 2015-01-28 02:51:58]

https://codereview.chromium.org/861673002/diff/320001/chrome/browser/resource...
File chrome/browser/resources/component_extension_resources.grd (right):

https://codereview.chromium.org/861673002/diff/320001/chrome/browser/resource...
chrome/browser/resources/component_extension_resources.grd:193: <include
name="IDR_PDF_PAGE_SELECTOR_CSS"
file="pdf/elements/page-selector/page-selector.css" type="BINDATA" />
These should called viewer-page-selector.

[Alexandre Carlton, 2015-01-28 07:49:15]

https://codereview.chromium.org/861673002/diff/320001/chrome/browser/resource...
File chrome/browser/resources/component_extension_resources.grd (right):

https://codereview.chromium.org/861673002/diff/320001/chrome/browser/resource...
chrome/browser/resources/component_extension_resources.grd:193: <include
name="IDR_PDF_PAGE_SELECTOR_CSS"
file="pdf/elements/page-selector/page-selector.css" type="BINDATA" />
On 2015/01/28 02:51:58, Sam McNally wrote:
> These should called viewer-page-selector.

Done.

[Sam McNally, 2015-01-29 04:20:28]

lgtm

[Alexandre Carlton, 2015-01-29 23:05:09]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-01-29 23:05:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861673002/380001

[commit-bot: I haz the power, 2015-01-30 00:44:50]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-01-30 00:46:11]

Patchset 20 (id:??) landed as
https://crrev.com/06c4b8c9dc292f0e23a4bb128d87165079557e3e
Cr-Commit-Position: refs/heads/master@{#313841}