Implement basic toolbar with Material Design and loading progress.

Implement basic toolbar with Material Design and loading progress.

Currently the toolbar only offers Save and Print functionality, but in other CLs  Bookmarks, Zoom and Goto Page features will be implemented.

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

[Alexandre Carlton, 2015-01-09 03:11:15]

Stripped out everything but the bare essentials.

[raymes, 2015-01-09 04:15:08]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2015-01-09 04:15:10]

Thanks!

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:14: <link rel="import"
href="chrome://resources/polymer/core-icons/core-icons.html">
Do we need this?

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:20: <!-- Font Roboto is not
included in chrome://resources, so this is a temporary workaround. -->
Can we add it to chrome://resources? It looks like we may just need to remove an
if-def:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:24: <paper-shadow
id="toolbar-shadow" z="1">
nit: only add ids to things that need to be referenced in JS (I don't think this
does)

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:25: <core-toolbar
id="pdf-toolbar">
maybe just id="toolbar"

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/index.css (left):

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index.css:24: }
Is this meant to be deleted?

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/index.css (right):

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index.css:30: }
Since this file is quite small and I think the material css file will end up
quite different, can we create a separate one (index-material.css)?

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/manifest-common.json.unflattened (left):

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/manifest-common.json.unflattened:34: ]
Is this meant to be removed?

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:51: this.isMaterial = pageLoaded ===
'index-material.html';
I think this can just be:
this.isMaterial_ = location.pathname.substring(1) == 'index-material.html'

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:85: }
We should set the plugin size here too, but it doesn't need to be inside
this.isMaterial.

this.plugin_.style.height = (window.innerHeight - this.toolbarHeight_) + 'px';
this.plugin_.style.width = window.innerWidth;

In the case of the non-material version, this.toolbarHeight should be 0

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:500: }
Same here, we can move this outside the if-block. We should set width and
height.

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:45: this.toolbarHeight_ = (toolbar) ?
toolbar.clientHeight : 0;
Can we pass the toolbar height into the viewport. Don't call it toolbarHeight
though, maybe call it yPos or something.

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:140: this.toolbarHeight_ + 'px';
We shouldn't need to adjust the width

[Alexandre Carlton, 2015-01-12 06:33:57]

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:14: <link rel="import"
href="chrome://resources/polymer/core-icons/core-icons.html">
On 2015/01/09 04:15:09, raymes wrote:
> Do we need this?

Removed.

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:20: <!-- Font Roboto is not
included in chrome://resources, so this is a temporary workaround. -->
On 2015/01/09 04:15:09, raymes wrote:
> Can we add it to chrome://resources? It looks like we may just need to remove
an
> if-def:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

See Issue 846833002.

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:24: <paper-shadow
id="toolbar-shadow" z="1">
On 2015/01/09 04:15:09, raymes wrote:
> nit: only add ids to things that need to be referenced in JS (I don't think
this
> does)

We need to set the position and width of this particular paper-shadow element
(as done in index.css); we might introduce a bug if we reference paper-shadow
instead.

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:25: <core-toolbar
id="pdf-toolbar">
On 2015/01/09 04:15:09, raymes wrote:
> maybe just id="toolbar"

The viewer-toolbar currently has an id of "toolbar" and a lot of code depends on
this; when it is removed in the next CL, the core-toolbar's id will be changed
to "toolbar".

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
File chrome/browser/resources/pdf/index.css (right):

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/index.css:30: }
On 2015/01/09 04:15:09, raymes wrote:
> Since this file is quite small and I think the material css file will end up
> quite different, can we create a separate one (index-material.css)?

Done.

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:51: this.isMaterial = pageLoaded ===
'index-material.html';
On 2015/01/09 04:15:10, raymes wrote:
> I think this can just be:
> this.isMaterial_ = location.pathname.substring(1) == 'index-material.html'

Done.

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:45: this.toolbarHeight_ = (toolbar) ?
toolbar.clientHeight : 0;
On 2015/01/09 04:15:10, raymes wrote:
> Can we pass the toolbar height into the viewport. Don't call it toolbarHeight
> though, maybe call it yPos or something.

Done.

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:140: this.toolbarHeight_ + 'px';
On 2015/01/09 04:15:10, raymes wrote:
> We shouldn't need to adjust the width

Done.

[raymes, 2015-01-13 00:26:18]

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:500: }
I think we need to keep something here.

https://codereview.chromium.org/806633003/diff/230001/chrome/browser/resource...
File chrome/browser/resources/pdf/viewport.js (right):

https://codereview.chromium.org/806633003/diff/230001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:26: * @param {number} yPos the offset
from the toolbar
the offset of the viewport from the top of the window

[Alexandre Carlton, 2015-01-13 03:57:27]

Progress now fills the entire toolbar.

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

https://codereview.chromium.org/806633003/diff/180001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:500: }
On 2015/01/09 04:15:10, raymes wrote:
> Same here, we can move this outside the if-block. We should set width and
> height.

Done.

https://codereview.chromium.org/806633003/diff/230001/chrome/browser/resource...
File chrome/browser/resources/pdf/viewport.js (right):

https://codereview.chromium.org/806633003/diff/230001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:26: * @param {number} yPos the offset
from the toolbar
On 2015/01/13 00:26:18, raymes wrote:
> the offset of the viewport from the top of the window

Done.

[Alexandre Carlton, 2015-01-13 03:59:28]

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

[Alexandre Carlton, 2015-01-13 03:59:29]

Add sammc@ as a reviewer.

[raymes, 2015-01-13 04:32:01]

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

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.css:14: font-family: RobotoDraft,
Roboto, 'Helvetica Neue', Helvetica, Arial;
Do we want RobotoDraft there?

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
File chrome/browser/resources/pdf/manifest-common.json.unflattened (left):

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
chrome/browser/resources/pdf/manifest-common.json.unflattened:34: ]
Is this meant to change?

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

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:483: this.plugin_.style.height =
I'm a little afraid this could impact the non-material plugin so let's wrap it
in an if (this.isMaterial_) for now (sorry I think I asked you to remove it
before, but let's just keep it for safety for now

[Alexandre Carlton, 2015-01-13 05:21:44]

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

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.css:14: font-family: RobotoDraft,
Roboto, 'Helvetica Neue', Helvetica, Arial;
On 2015/01/13 04:32:01, raymes wrote:
> Do we want RobotoDraft there?

RobotoDraft is the default for Polymer, so if it's available then I thought it
should be used. I've removed it now.

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
File chrome/browser/resources/pdf/manifest-common.json.unflattened (left):

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
chrome/browser/resources/pdf/manifest-common.json.unflattened:34: ]
On 2015/01/13 04:32:01, raymes wrote:
> Is this meant to change?

I might have accidentally removed this when rebasing. I've included it now.

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

https://codereview.chromium.org/806633003/diff/270001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:483: this.plugin_.style.height =
On 2015/01/13 04:32:01, raymes wrote:
> I'm a little afraid this could impact the non-material plugin so let's wrap it
> in an if (this.isMaterial_) for now (sorry I think I asked you to remove it
> before, but let's just keep it for safety for now

Done.

[raymes, 2015-01-13 05:27:27]

https://codereview.chromium.org/806633003/diff/290001/chrome/browser/resource...
File chrome/browser/resources/pdf/manifest-common.json.unflattened (right):

https://codereview.chromium.org/806633003/diff/290001/chrome/browser/resource...
chrome/browser/resources/pdf/manifest-common.json.unflattened:33: }
There are whitespace changes here still.

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

https://codereview.chromium.org/806633003/diff/290001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:84: this.plugin_.style.height =
(window.innerHeight - this.toolbarHeight_) + 'px';
We'll have to add if (this.isMaterial_) here too.

[Alexandre Carlton, 2015-01-13 06:22:41]

https://codereview.chromium.org/806633003/diff/290001/chrome/browser/resource...
File chrome/browser/resources/pdf/manifest-common.json.unflattened (right):

https://codereview.chromium.org/806633003/diff/290001/chrome/browser/resource...
chrome/browser/resources/pdf/manifest-common.json.unflattened:33: }
On 2015/01/13 05:27:27, raymes wrote:
> There are whitespace changes here still.

Done.

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

https://codereview.chromium.org/806633003/diff/290001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:84: this.plugin_.style.height =
(window.innerHeight - this.toolbarHeight_) + 'px';
On 2015/01/13 05:27:27, raymes wrote:
> We'll have to add if (this.isMaterial_) here too.

Done.

[raymes, 2015-01-13 06:24:50]

lgtm

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

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:62: this.toolbarHeight_ = (this.isMaterial_)
? $('pdf-toolbar').clientHeight : 0;
nit: don't need the () around this.isMaterial_

[Sam McNally, 2015-01-13 07:03:57]

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

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:26: <core-toolbar
id="pdf-toolbar">
It seems a bit weird for the toolbar to be inside the progress bar.

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

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:84: this.plugin_.style.top =
this.toolbarHeight_ + 'px';
top and width can be set in the css file.

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:99: $('title').innerHTML = document.title;
$('title').textContent

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:489: this.plugin_.style.height =
window.innerWidth;
this.plugin_.style.width.
I think it's worth skipping this until the width can change.

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
File chrome/browser/resources/pdf/viewport.js (right):

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:46: var toolbar = $('pdf-toolbar');
Is this used?

[Alexandre Carlton, 2015-01-14 22:42:51]

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

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:26: <core-toolbar
id="pdf-toolbar">
On 2015/01/13 07:03:56, Sam McNally wrote:
> It seems a bit weird for the toolbar to be inside the progress bar.

Done.

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

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:84: this.plugin_.style.top =
this.toolbarHeight_ + 'px';
On 2015/01/13 07:03:56, Sam McNally wrote:
> top and width can be set in the css file.

Done for top; does width need to be set at all?

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
File chrome/browser/resources/pdf/viewport.js (right):

https://codereview.chromium.org/806633003/diff/310001/chrome/browser/resource...
chrome/browser/resources/pdf/viewport.js:46: var toolbar = $('pdf-toolbar');
On 2015/01/13 07:03:57, Sam McNally wrote:
> Is this used?

Removed.

[Sam McNally, 2015-01-19 00:09:37]

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

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:17: <!-- TODO(alexandrec): When
https://codereview.chromium.org/846833002/ is landed, remove this.-->
I think you can remove this now.

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:27: <span id="left-toolbar"
flex>
Is this containing span necessary?

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:28: <span id="title">PDF
Viewer</span>
Is "PDF Viewer" meant to be seen? If not, leave this empty.

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:31: <span id="middle-toolbar"
flex>
Does this need an id?

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:34: <span id="left-toolbar">
Is this containing span necessary?

[raymes, 2015-01-19 00:11:50]

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

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:28: <span id="title">PDF
Viewer</span>
On 2015/01/19 00:09:36, Sam McNally wrote:
> Is "PDF Viewer" meant to be seen? If not, leave this empty.

+1

[Alexandre Carlton, 2015-01-19 00:40:12]

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

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:17: <!-- TODO(alexandrec): When
https://codereview.chromium.org/846833002/ is landed, remove this.-->
On 2015/01/19 00:09:36, Sam McNally wrote:
> I think you can remove this now.

Done.

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:27: <span id="left-toolbar"
flex>
On 2015/01/19 00:09:36, Sam McNally wrote:
> Is this containing span necessary?

Removed.

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:28: <span id="title">PDF
Viewer</span>
On 2015/01/19 00:09:36, Sam McNally wrote:
> Is "PDF Viewer" meant to be seen? If not, leave this empty.

Done.

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:31: <span id="middle-toolbar"
flex>
On 2015/01/19 00:09:36, Sam McNally wrote:
> Does this need an id?

Removed.

https://codereview.chromium.org/806633003/diff/350001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:34: <span id="left-toolbar">
On 2015/01/19 00:09:36, Sam McNally wrote:
> Is this containing span necessary?

Removed.

[Sam McNally, 2015-01-19 00:42:55]

lgtm

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

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-01-19 02:11:57]

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

[commit-bot: I haz the power, 2015-01-19 02:15:20]

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

[commit-bot: I haz the power, 2015-01-19 02:15:20]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexandre Carlton, 2015-01-19 03:42:45]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-01-19 03:43:03]

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

[commit-bot: I haz the power, 2015-01-19 03:48:58]

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

[commit-bot: I haz the power, 2015-01-19 03:49:00]

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

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

[Alexandre Carlton, 2015-01-19 04:11:44]

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

[Alexandre Carlton, 2015-01-19 04:11:44]

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-20 22:42:57]

I don't see polymner_resources.grdp but component_extension_resources.grd lgtm

[raymes, 2015-01-20 23:00:10]

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

https://codereview.chromium.org/806633003/diff/580001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:18: <link rel="stylesheet"
type="text/css" href="index.css">
Let's remove this line and copy the parts that we need into index-material.css
In particular remove:
the plugin width and height attributes
anything else we don't use

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

https://codereview.chromium.org/806633003/diff/580001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:89: (window.innerHeight -
this.toolbarHeight_) + 'px';
Let's add back the width setting here.

https://codereview.chromium.org/806633003/diff/580001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:561: (window.innerHeight -
this.toolbarHeight_) + 'px';
Let's add back the width setting here.

[Alexandre Carlton, 2015-01-20 23:19:18]

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

https://codereview.chromium.org/806633003/diff/580001/chrome/browser/resource...
chrome/browser/resources/pdf/index-material.html:18: <link rel="stylesheet"
type="text/css" href="index.css">
On 2015/01/20 23:00:10, raymes wrote:
> Let's remove this line and copy the parts that we need into index-material.css
> In particular remove:
> the plugin width and height attributes
> anything else we don't use

Done.

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

https://codereview.chromium.org/806633003/diff/580001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:89: (window.innerHeight -
this.toolbarHeight_) + 'px';
On 2015/01/20 23:00:10, raymes wrote:
> Let's add back the width setting here.

Done.

https://codereview.chromium.org/806633003/diff/580001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:561: (window.innerHeight -
this.toolbarHeight_) + 'px';
On 2015/01/20 23:00:10, raymes wrote:
> Let's add back the width setting here.

Done.

[raymes, 2015-01-21 00:04:27]

lgtm

[Alexandre Carlton, 2015-01-21 00:25:33]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-01-21 00:27:18]

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

[commit-bot: I haz the power, 2015-01-21 01:50:40]

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

[commit-bot: I haz the power, 2015-01-21 01:50:41]

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_...)

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-01-21 03:28:52]

Adjusted viewport_test.js to accommodate the extra yPos argument.

[raymes, 2015-01-21 03:46:42]

lgtm

[Alexandre Carlton, 2015-01-21 04:33:32]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-01-21 04:36:08]

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

[commit-bot: I haz the power, 2015-01-21 11:08:09]

Committed patchset #34 (id:640001)

[commit-bot: I haz the power, 2015-01-21 11:09:50]

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