Refactor Material Design PDF Viewer toolbar into a single element

Refactor Material Design PDF Viewer toolbar into a single element

Before, the layout of the Material Design PDF Viewer toolbar was exposed.
Now it has been compacted into one element so that it can be more easily maintained and updated.

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

[Alexandre Carlton, 2015-02-03 02:22:24]

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

[Alexandre Carlton, 2015-02-03 02:22:25]

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:687: this.pageIndicator_.pageLabels =
message.data.pageNumbers;
Just drawing attention to this; what should we do about pageLabels?

[benwells, 2015-02-03 06:31:33]

looks great. lgtm with some formatting nits but obviously wait for the others to
review...

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.html
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.html:16:
<viewer-page-selector id="pageselector" index="{{index}}"
docLength="{{docLength}}" flex></viewer-page-selector>
Can you keep your lines to < 80 characters (the imports are probably fine).

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:7:
filename: '',
I think you can probably remove the blank line above.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:20:
save: function() {
For consistency, please have a blank line between all the functions (or
alternatively, none, but all is probably more readable).

[Sam McNally, 2015-02-03 07:22:46]

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:7:
filename: '',
How about "title"?

Please add comments for these attributes.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:8:
index: '0',
In a toolbar element it isn't obvious what this is referring to. How about
pageIndex?

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:9:
progress: 0,
Similarly, loadProgress?

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:160: this.bookmarksPane_.toggle();
2 space indent.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:481: if (!this.isPrintPreview_ &&
!this.isMaterial_)
Move this inside the if above.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:687: this.pageIndicator_.pageLabels =
message.data.pageNumbers;
On 2015/02/03 02:22:24, Alexandre Carlton wrote:
> Just drawing attention to this; what should we do about pageLabels?

We're keeping pageLabels, but we don't need to deal with them in the new UI
until we figure out what the plan is for print preview.

[Alexandre Carlton, 2015-02-03 22:50:43]

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.html
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.html:16:
<viewer-page-selector id="pageselector" index="{{index}}"
docLength="{{docLength}}" flex></viewer-page-selector>
On 2015/02/03 06:31:33, benwells wrote:
> Can you keep your lines to < 80 characters (the imports are probably fine).

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:7:
filename: '',
On 2015/02/03 06:31:33, benwells wrote:
> I think you can probably remove the blank line above.

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:7:
filename: '',
On 2015/02/03 07:22:45, Sam McNally wrote:
> How about "title"?
> 
> Please add comments for these attributes.

I'm a bit resistant to change it to 'title' since the title of a PDF document
can greatly differ to its filename. raymes@, what do you think?

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:8:
index: '0',
On 2015/02/03 07:22:45, Sam McNally wrote:
> In a toolbar element it isn't obvious what this is referring to. How about
> pageIndex?

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:9:
progress: 0,
On 2015/02/03 07:22:45, Sam McNally wrote:
> Similarly, loadProgress?

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:20:
save: function() {
On 2015/02/03 06:31:33, benwells wrote:
> For consistency, please have a blank line between all the functions (or
> alternatively, none, but all is probably more readable).

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:160: this.bookmarksPane_.toggle();
On 2015/02/03 07:22:45, Sam McNally wrote:
> 2 space indent.

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:481: if (!this.isPrintPreview_ &&
!this.isMaterial_)
On 2015/02/03 07:22:45, Sam McNally wrote:
> Move this inside the if above.

Done.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:687: this.pageIndicator_.pageLabels =
message.data.pageNumbers;
On 2015/02/03 07:22:45, Sam McNally wrote:
> On 2015/02/03 02:22:24, Alexandre Carlton wrote:
> > Just drawing attention to this; what should we do about pageLabels?
> 
> We're keeping pageLabels, but we don't need to deal with them in the new UI
> until we figure out what the plan is for print preview.

Acknowledged.

[raymes, 2015-02-04 00:54:26]

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:6:

nit remove the blank line

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:11:
filename: '',
nit: we can add blank lines after each of the variables

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

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/index-material.html:18: <viewer-pdf-toolbar
id="material-toolbar"></viewer-pdf-toolbar>
nit: can we just call this "viewer-toolbar"?

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:142:
document.body.addEventListener('changePage', function(e) {
nit: should we just attach the event listener to the materialToolbar as well? We
can group this with the other event listeners below

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:159:
this.materialToolbar_.addEventListener('toggle-bookmarks', function() {
We should be consistent with how we name events (we used camel case for
changePage). Polymer seems to use dashes like this, so can we change changePage
to change-page?

[Alexandre Carlton, 2015-02-04 04:28:47]

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:6:

On 2015/02/04 00:54:25, raymes wrote:
> nit remove the blank line

Done.

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:11:
filename: '',
On 2015/02/04 00:54:25, raymes wrote:
> nit: we can add blank lines after each of the variables

Done.

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

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/index-material.html:18: <viewer-pdf-toolbar
id="material-toolbar"></viewer-pdf-toolbar>
On 2015/02/04 00:54:25, raymes wrote:
> nit: can we just call this "viewer-toolbar"?

Until the new Polymer zoom element is finished we will still have viewer-toolbar
at the bottom-right of the screen (necessary for the non-material viewer).

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:142:
document.body.addEventListener('changePage', function(e) {
On 2015/02/04 00:54:25, raymes wrote:
> nit: should we just attach the event listener to the materialToolbar as well?
We
> can group this with the other event listeners below
> 

The viewer-bookmarks-pane also fires 'change-page' as well, which will not be
caught by this.materialToolbar_. Should two event listeners be added for each
element or still have the body listen for the event?

https://codereview.chromium.org/898673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:159:
this.materialToolbar_.addEventListener('toggle-bookmarks', function() {
On 2015/02/04 00:54:25, raymes wrote:
> We should be consistent with how we name events (we used camel case for
> changePage). Polymer seems to use dashes like this, so can we change
changePage
> to change-page?

Done.

[Sam McNally, 2015-02-08 23:30:47]

LGTM

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:7:
filename: '',
On 2015/02/03 22:50:42, Alexandre Carlton wrote:
> On 2015/02/03 07:22:45, Sam McNally wrote:
> > How about "title"?
> > 
> > Please add comments for these attributes.
> 
> I'm a bit resistant to change it to 'title' since the title of a PDF document
> can greatly differ to its filename. raymes@, what do you think?

Let's call this title. It doesn't necessarily mean the PDF's title, but rather
whatever the user of this element decides the title is, which happens to be the
filename right now.

[Alexandre Carlton, 2015-02-09 03:34:22]

New patchsets have been uploaded after l-g-t-m from sammc@chromium.org

[Alexandre Carlton, 2015-02-09 03:44:04]

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

[Alexandre Carlton, 2015-02-09 03:44:05]

estade@chromium.org: Please review changes in 

chrome/browser/resources/component_extension_resources.grd

Thank you.

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js
(right):

https://codereview.chromium.org/898673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/viewer-pdf-toolbar/viewer-pdf-toolbar.js:7:
filename: '',
On 2015/02/08 23:30:47, Sam McNally wrote:
> On 2015/02/03 22:50:42, Alexandre Carlton wrote:
> > On 2015/02/03 07:22:45, Sam McNally wrote:
> > > How about "title"?
> > > 
> > > Please add comments for these attributes.
> > 
> > I'm a bit resistant to change it to 'title' since the title of a PDF
document
> > can greatly differ to its filename. raymes@, what do you think?
> 
> Let's call this title. It doesn't necessarily mean the PDF's title, but rather
> whatever the user of this element decides the title is, which happens to be
the
> filename right now.

The 'title' attribute is special when dealing with HTML elements, so I've named
it docTitle.
Reference:
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/title

[raymes, 2015-02-09 04:04:24]

lgtm

[Alexandre Carlton, 2015-02-09 04:06:42]

New patchsets have been uploaded after l-g-t-m from raymes@chromium.org

[Evan Stade, 2015-02-10 00:30:57]

feel free to tbr me for this kind of mechanical change

lgtm

[Alexandre Carlton, 2015-02-10 01:53:44]

New patchsets have been uploaded after l-g-t-m from estade@chromium.org

[Alexandre Carlton, 2015-02-10 01:54:17]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-02-10 01:54:56]

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

[commit-bot: I haz the power, 2015-02-10 02:59:01]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-02-10 02:59:30]

Patchset 14 (id:??) landed as
https://crrev.com/cb0a54c38338f9d62605857662e4940eae89480f
Cr-Commit-Position: refs/heads/master@{#315484}