Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137663008:
Implement viewporting for the out of process PDF plugin. (Closed)
|
Created:
6 years, 10 months ago by raymes Modified:
6 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement viewporting for the out of process PDF plugin.
BUG=303491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251993
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : #Patch Set 4 : #
Total comments: 42
Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/index.in.html (right): https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/index.in.html:31: <viewer-button id="btn-ftp" src="button_fit_page.png" latchable=true></viewer-button> btn-ftp -> fit-to-page-button https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/index.in.html:32: <viewer-button id="btn-ftw" src="button_fit_width.png" latchable=true></viewer-button> btn-ftw -> fit-to-width-button https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:15: var gSizer; Maybe instead of saying that it represents the size, describe how that works. Eg, it's a sibling of the plugin element that is used to stretch the size of the body (if that is how it works...). An explanation of why we need to resize the sizer, rather than the plugin element itself would be useful, too. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:8: ZOOM_FACTORS = [0.25, 0.333, 0.5, 0.666, 0.75, 0.9, 1.0, add a 'var' here? https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:71: window.addEventListener('scroll', scrollCallback); This could be written as window.addEventListener('scroll', this.updateViewport_.bind(this)); https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:132: var scrollbarWidth = this.scrollbarWidth_; nit: this can be inlined https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:155: * Returns the most visible page on the screen. Maybe rephase as "Returns the page with the most pixels in the current viewport." https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:263: if (ZOOM_FACTORS[i] < this.zoom_) Won't this get the next smallest zoom factor, which means we'll be zooming out? Or do I have it backwards? Maybe a clarifying comment on the ZOOM_FACTORS variable is in order.
https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/index.in.html (right): https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/index.in.html:31: <viewer-button id="btn-ftp" src="button_fit_page.png" latchable=true></viewer-button> On 2014/02/03 03:40:43, koz wrote: > btn-ftp -> fit-to-page-button Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/index.in.html:32: <viewer-button id="btn-ftw" src="button_fit_width.png" latchable=true></viewer-button> On 2014/02/03 03:40:43, koz wrote: > btn-ftw -> fit-to-width-button Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:15: var gSizer; On 2014/02/03 03:40:43, koz wrote: > Maybe instead of saying that it represents the size, describe how that works. > Eg, it's a sibling of the plugin element that is used to stretch the size of the > body (if that is how it works...). An explanation of why we need to resize the > sizer, rather than the plugin element itself would be useful, too. Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:8: ZOOM_FACTORS = [0.25, 0.333, 0.5, 0.666, 0.75, 0.9, 1.0, On 2014/02/03 03:40:43, koz wrote: > add a 'var' here? Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:71: window.addEventListener('scroll', scrollCallback); On 2014/02/03 03:40:43, koz wrote: > This could be written as > > window.addEventListener('scroll', this.updateViewport_.bind(this)); Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:132: var scrollbarWidth = this.scrollbarWidth_; On 2014/02/03 03:40:43, koz wrote: > nit: this can be inlined Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:155: * Returns the most visible page on the screen. On 2014/02/03 03:40:43, koz wrote: > Maybe rephase as "Returns the page with the most pixels in the current > viewport." Done. https://codereview.chromium.org/137663008/diff/20001/chrome/browser/resources... chrome/browser/resources/pdf/viewport.js:263: if (ZOOM_FACTORS[i] < this.zoom_) You're right, good catch! - the ids were backward :)
lgtm!
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/raymes@chromium.org/137663008/130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+arv for OWNERS
On 2014/02/06 04:30:48, raymes wrote: > +arv for OWNERS ping
https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... File chrome/browser/resources/pdf/index.html (right): https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/index.html:264: <viewer-button id="fit-to-page-button" src="button_fit_page.png" latchable="true"> <viewer-button latchable="true"> => <viewer-button latchable> https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:13: var gPlugin; Why gPlugin instead of plugin? https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:57: gPlugin = document.createElement('object'); Any reason why this is created in script instead of in the html Also, can the style be moved to a style element/css file? https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:79: gPlugin.setAttribute('src', url); plugin.src = https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:86: function() { gViewport.fitToWidth(); }); or $('fit-to-width-button').addEventListener('click', gViewport.fitToWidth.bind(gViewport)); https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:103: e.stopPropagation(); why do you need to stop the event propagation. Please add comment why it is needed https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:104: return false; Please use `event.preventDefault()` https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:14: * @param {dictionary} rect1 the first rect Wrong type. Maybe {{x: number, y: number, width: number, height: number}} or just {Object} https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:29: * Return the width of a scrollbar in pixels. Useless comment https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:35: parentDiv.style.width = '500px'; invalidates layout https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:37: var parentDivWidth = parentDiv.offsetWidth; forces layout https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:38: parentDiv.style.overflow = 'scroll'; invalidates layout again. Maybe restructure so that we do not have to do more than one layout? https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:49: * @param {object} window is the page window invalid type https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:49: * @param {object} window is the page window Badly formatted jsdoc @param {Object} window The page window. or just @param {Object} window or better yet @param {Window} window https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:51: * document in the viewport indent 4 spaces https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:52: * @param {function} fitToPageEnabledFunction returns true if fit-to-page is Function or function(T1, T2) : T3 https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:79: Viewport.prototype.documentNeedsScrollbars_ = function(zoom) { Viewport.prototype = { documentNeedsScrollbars_: function(zoom) { }, ... }; https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:113: var currentScrollPos = var currentScrollPos = [ (this.window_.scrollX + this.window_.innerWidth / 2) / oldZoom, (this.window_.scrollY + this.window_.innerHeight / 2) / oldZoom ]; https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:114: [(this.window_.scrollX + this.window_.innerWidth / 2.0) / oldZoom, 2.0 -> 2 https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:119: (currentScrollPos[0] * newZoom) - this.window_.innerWidth / 2.0, too many parens https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:144: 'x': this.window_.pageXOffset / this.zoom_, No need to quote the property names
Thanks for all the comments! Sorry for my javascript illiteracy. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... File chrome/browser/resources/pdf/index.html (right): https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/index.html:264: <viewer-button id="fit-to-page-button" src="button_fit_page.png" latchable="true"> On 2014/02/13 23:47:41, arv wrote: > <viewer-button latchable="true"> > > => > > <viewer-button latchable> Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:13: var gPlugin; On 2014/02/13 23:47:41, arv wrote: > Why gPlugin instead of plugin? Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:57: gPlugin = document.createElement('object'); The plugin gets loaded when the the object node is added to the DOM so we need to create it dynamically. I moved the style to the html file as well as the sizer element. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:79: gPlugin.setAttribute('src', url); That doesn't work, for some reason. I suspect it's due to some weird plugin-loading related behavior. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:86: function() { gViewport.fitToWidth(); }); On 2014/02/13 23:47:41, arv wrote: > or > > $('fit-to-width-button').addEventListener('click', > gViewport.fitToWidth.bind(gViewport)); Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:103: e.stopPropagation(); It's not needed. Thanks. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:104: return false; On 2014/02/13 23:47:41, arv wrote: > Please use `event.preventDefault()` Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:14: * @param {dictionary} rect1 the first rect On 2014/02/13 23:47:41, arv wrote: > Wrong type. Maybe {{x: number, y: number, width: number, height: number}} or > just {Object} Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:29: * Return the width of a scrollbar in pixels. On 2014/02/13 23:47:41, arv wrote: > Useless comment Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:35: parentDiv.style.width = '500px'; On 2014/02/13 23:47:41, arv wrote: > invalidates layout >> https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:37: var parentDivWidth = parentDiv.offsetWidth; On 2014/02/13 23:47:41, arv wrote: > forces layout >> https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:38: parentDiv.style.overflow = 'scroll'; Could you explain a bit more how to improve the performance? This should only be called once on construction so it might not be too bad regardless. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:49: * @param {object} window is the page window On 2014/02/13 23:47:41, arv wrote: > invalid type Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:49: * @param {object} window is the page window On 2014/02/13 23:47:41, arv wrote: > invalid type Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:51: * document in the viewport On 2014/02/13 23:47:41, arv wrote: > indent 4 spaces Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:52: * @param {function} fitToPageEnabledFunction returns true if fit-to-page is On 2014/02/13 23:47:41, arv wrote: > Function or function(T1, T2) : T3 Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:79: Viewport.prototype.documentNeedsScrollbars_ = function(zoom) { On 2014/02/13 23:47:41, arv wrote: > Viewport.prototype = { > documentNeedsScrollbars_: function(zoom) { > }, > ... > }; Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:113: var currentScrollPos = On 2014/02/13 23:47:41, arv wrote: > var currentScrollPos = [ > (this.window_.scrollX + this.window_.innerWidth / 2) / oldZoom, > (this.window_.scrollY + this.window_.innerHeight / 2) / oldZoom > ]; Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:114: [(this.window_.scrollX + this.window_.innerWidth / 2.0) / oldZoom, On 2014/02/13 23:47:41, arv wrote: > 2.0 -> 2 Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:119: (currentScrollPos[0] * newZoom) - this.window_.innerWidth / 2.0, On 2014/02/13 23:47:41, arv wrote: > too many parens Done. https://codereview.chromium.org/137663008/diff/130001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:144: 'x': this.window_.pageXOffset / this.zoom_, On 2014/02/13 23:47:41, arv wrote: > No need to quote the property names Done.
https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:5: (function() { 'use strict'; https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:10: 1.1, 1.25, 1.5, 1.75, 2.0, 2.5, 3.0, 4.0, 5.0]; x.0 -> x JS only has one number type https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:36: var parentDivWidth = parentDiv.offsetWidth; Isn't this always 500? It seems like you could skip one layout here. https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:52: * enabled JSDoc should be indented too https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:55: Viewport = function(window, Prefer FunctionDeclaration over FunctionExpression. function Viewport(...)
https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:5: (function() { On 2014/02/18 17:37:29, arv wrote: > 'use strict'; Done. https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:10: 1.1, 1.25, 1.5, 1.75, 2.0, 2.5, 3.0, 4.0, 5.0]; On 2014/02/18 17:37:29, arv wrote: > x.0 -> x > > JS only has one number type Done. https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:36: var parentDivWidth = parentDiv.offsetWidth; On 2014/02/18 17:37:29, arv wrote: > Isn't this always 500? > > It seems like you could skip one layout here. You're right, done! https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:52: * enabled On 2014/02/18 17:37:29, arv wrote: > JSDoc should be indented too Done. https://codereview.chromium.org/137663008/diff/420002/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:55: Viewport = function(window, On 2014/02/18 17:37:29, arv wrote: > Prefer FunctionDeclaration over FunctionExpression. > > function Viewport(...) Done.
LGTM https://codereview.chromium.org/137663008/diff/470001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/137663008/diff/470001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:69: }; no semicolon after FunctionDeclaration
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/raymes@chromium.org/137663008/540002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) remoting_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/raymes@chromium.org/137663008/540002
Message was sent while issue was closed.
Change committed as 251993 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
