Implement a slide-in pane for bookmarks.

Implement a slide-in pane for bookmarks.

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

[Alexandre Carlton, 2015-01-27 07:08:55]

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

[Alexandre Carlton, 2015-01-27 07:09:53]

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

[Alexandre Carlton, 2015-01-27 07:09:53]

thestig@chromium.org: Please review changes in 

chrome/browser/resources/component_extension_resources.grd

Thank you.

[Lei Zhang, 2015-01-27 07:29:57]

On 2015/01/27 07:09:53, Alexandre Carlton wrote:
> mailto:thestig@chromium.org: Please review changes in 
> 
> chrome/browser/resources/component_extension_resources.grd
> 
> Thank you.

Will do after the rest of the CL gets reviewed.

[raymes, 2015-01-27 09:10:55]

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js
(right):

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:6:
bookmarks: null,
Let's add documentation for all of the functions and members

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:26: var
titleNode = document.createElement('div');
titleNode seems a bit confusing to me because I don't think it contains a title
at all? Doesn't it contain the children? "node" seems to be the title?

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:36:
return wrapperNode;
I think the structure should be something like this:
<div> <- wrapper
  <paper-item> <- title
  <div> <- child 1
  <div> <- child 2
  ...
</div>

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/index-material.css (right):

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/index-material.css:60: z-index: 3;
The bookmarks should be behind the toolbar so the shadow the toolbar lies over
the bookmarks pane

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

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:445: this.bookmarksPane.bookmarks =
message.data.bookmarks;
Do we use "this.bookmarks_"? I think we just used it in the test. Could we
return this.bookmarksPane.bookmarks inside the getter (in the test CL) and get
rid of this.bookmarks_?

[Alexandre Carlton, 2015-01-28 00:22:47]

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js
(right):

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:6:
bookmarks: null,
On 2015/01/27 09:10:54, raymes wrote:
> Let's add documentation for all of the functions and members

Done.

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:26: var
titleNode = document.createElement('div');
On 2015/01/27 09:10:54, raymes wrote:
> titleNode seems a bit confusing to me because I don't think it contains a
title
> at all? Doesn't it contain the children? "node" seems to be the title?

Fixed; node is now titleNode, and titleNode has been renamed to subNode.

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:36:
return wrapperNode;
On 2015/01/27 09:10:54, raymes wrote:
> I think the structure should be something like this:
> <div> <- wrapper
>   <paper-item> <- title
>   <div> <- child 1
>   <div> <- child 2
>   ...
> </div>
> 

For reference, the structure currently is:
<div>
  <paper-item> <- title
  <div class="sub-bookmark">
    <div> <- child 2
      <paper-item> <- child 1 title
      <div class="sub-bookmark">...</div>
    </div>
    <div> <- child 2
      <paper-item> <- child 2 title
      <div class="sub-bookmark">...</div>
    </div>
  </div>
  ...
</div>
I have wrapped each parent and its children inside a div as it is easier to
return and append to another element.

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/index-material.css (right):

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/index-material.css:60: z-index: 3;
On 2015/01/27 09:10:54, raymes wrote:
> The bookmarks should be behind the toolbar so the shadow the toolbar lies over
> the bookmarks pane

As discussed, the internal paper-dialog within pdf-pane.html resets its z-index
to 12 on every toggle-in. Currently the only way to force it underneath is to
raise the z-index of #toolbar-shadow to 13 (an ugly hack).

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

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:445: this.bookmarksPane.bookmarks =
message.data.bookmarks;
On 2015/01/27 09:10:54, raymes wrote:
> Do we use "this.bookmarks_"? I think we just used it in the test. Could we
> return this.bookmarksPane.bookmarks inside the getter (in the test CL) and get
> rid of this.bookmarks_?

Done.

[raymes, 2015-01-28 01:10:20]

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/index-material.css (right):

https://codereview.chromium.org/865033002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/index-material.css:60: z-index: 3;
What can we do about this? Will you investigate and fix it in a separate CL?

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js
(right):

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:12: //
Inserts bookmark elements into the pane
Take a look at the jsdoc in pdf.js and let's follow the same style

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:32: var
subNode = document.createElement('div');
nit: can we call this childBookmarksNode

[Sam McNally, 2015-01-28 03:04:30]

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

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/component_extension_resources.grd:174: <include
name="IDR_PDF_BOOKMARKS_PANE_CSS"
file="pdf/elements/bookmarks-pane/bookmarks-pane.css" type="BINDATA" />
viewer-bookmarks-pane.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js
(right):

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:21:
makeBookmark_: function(bookmark) {
It feels like a bookmark could be a custom element and this should use polymer
template binding.

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

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:131:
$('bookmarks-button').addEventListener('click', function() {
if (this.isMaterial_) {

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:132: this.bookmarksPane.toggle();
Indentation.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:695: return this.bookmarksPane.bookmarks;
Does this still work if !this.isMaterial_?

[Alexandre Carlton, 2015-01-28 07:16:52]

Created a new Polymer element called 'pdf-bookmark' which is far more malleable
in laying out the bookmarks in the pane.

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

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/component_extension_resources.grd:174: <include
name="IDR_PDF_BOOKMARKS_PANE_CSS"
file="pdf/elements/bookmarks-pane/bookmarks-pane.css" type="BINDATA" />
On 2015/01/28 03:04:30, Sam McNally wrote:
> viewer-bookmarks-pane.

Done.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js
(right):

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:12: //
Inserts bookmark elements into the pane
On 2015/01/28 01:10:20, raymes wrote:
> Take a look at the jsdoc in pdf.js and let's follow the same style

Done.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:32: var
subNode = document.createElement('div');
On 2015/01/28 01:10:20, raymes wrote:
> nit: can we call this childBookmarksNode 

Done.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/bookmarks-pane/bookmarks-pane.js:32: var
subNode = document.createElement('div');
On 2015/01/28 01:10:20, raymes wrote:
> nit: can we call this childBookmarksNode 

Done.

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

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:131:
$('bookmarks-button').addEventListener('click', function() {
On 2015/01/28 03:04:30, Sam McNally wrote:
> if (this.isMaterial_) {

Done.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:132: this.bookmarksPane.toggle();
On 2015/01/28 03:04:30, Sam McNally wrote:
> Indentation.

Done.

https://codereview.chromium.org/865033002/diff/100001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:695: return this.bookmarksPane.bookmarks;
On 2015/01/28 03:04:30, Sam McNally wrote:
> Does this still work if !this.isMaterial_?

Would it be best to return an empty list if !this.isMaterial_ ?

[raymes, 2015-01-29 04:55:07]

Looks good!

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/pdf-bookmark/pdf-bookmark.js (right):

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-bookmark/pdf-bookmark.js:5:
Polymer('pdf-bookmark', {
Sam suggested calling this viewer-bookmark

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-bookmark/pdf-bookmark.js:10: // -
children (an array of bookmarks)
Please use the jsdoc like in pdf.js :)

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

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:700: return this.bookmarksPane.bookmarks;
Actually now that you've written it, I realised this will cause the bookmarks
test to fail. Let's keep this as an instance variable instead (this.bookmarks).
Sorry about that!

[Alexandre Carlton, 2015-01-29 05:39:06]

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/pdf/elements/pdf-bookmark/pdf-bookmark.js (right):

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-bookmark/pdf-bookmark.js:5:
Polymer('pdf-bookmark', {
On 2015/01/29 04:55:07, raymes wrote:
> Sam suggested calling this viewer-bookmark

Done.

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/pdf-bookmark/pdf-bookmark.js:10: // -
children (an array of bookmarks)
On 2015/01/29 04:55:07, raymes wrote:
> Please use the jsdoc like in pdf.js :)

Done.

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

https://codereview.chromium.org/865033002/diff/140001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:700: return this.bookmarksPane.bookmarks;
On 2015/01/29 04:55:07, raymes wrote:
> Actually now that you've written it, I realised this will cause the bookmarks
> test to fail. Let's keep this as an instance variable instead
(this.bookmarks).
> Sorry about that!

Done.

[raymes, 2015-01-30 03:46:43]

lgtm

[Lei Zhang, 2015-01-30 03:55:20]

On 2015/01/27 07:29:57, Lei Zhang wrote:
> On 2015/01/27 07:09:53, Alexandre Carlton wrote:
> > mailto:thestig@chromium.org: Please review changes in 
> > 
> > chrome/browser/resources/component_extension_resources.grd
> > 
> > Thank you.
> 
> Will do after the rest of the CL gets reviewed.

lgtm

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

LGTM

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

https://codereview.chromium.org/865033002/diff/190001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:393: this.toolbar_.initialFadeIn();
Isn't the old toolbar still used for zoom?

[Alexandre Carlton, 2015-02-02 01:41:03]

Committing as benwells@ is currently in London and may not provide feedback for
a week.

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

https://codereview.chromium.org/865033002/diff/190001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:393: this.toolbar_.initialFadeIn();
On 2015/01/30 06:30:04, Sam McNally wrote:
> Isn't the old toolbar still used for zoom?

Done.

[Alexandre Carlton, 2015-02-02 01:42:17]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-02-02 01:45:24]

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

[commit-bot: I haz the power, 2015-02-02 02:50:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-02 02:50:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexandre Carlton, 2015-02-02 03:19:16]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-02-02 03:20:02]

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

[commit-bot: I haz the power, 2015-02-02 06:23:03]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-02-02 06:23:59]

Patchset 13 (id:??) landed as
https://crrev.com/8a94d32d211003d3bc6741c393e9547138d3ade4
Cr-Commit-Position: refs/heads/master@{#314105}

[benwells, 2015-02-02 10:26:43]

lgtm