Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
814573004:
Fix for Multipage selection by dragging mouse in OOP case in PDF. (Closed)
DescriptionFix for Multipage selection by dragging mouse in OOP case in PDF.
Code was not present for handling drag by mouse.
Now we are passing message when their is selection happening and based on mousemove event handler we are scrolling page when mouse points are outside the viewport.
When we move mouse outside the viewport and does not move then mousemove events will not come then we are scrolling based on timer.
BUG=446831
Committed: https://crrev.com/423f09c6d2be8c916637cdda28ed0ee4e12490e0
Cr-Commit-Position: refs/heads/master@{#316204}
Patch Set 1 #Patch Set 2 : Approach of handling drag in js file. #Patch Set 3 : Fixing indentation. #Patch Set 4 : Addressing nit. #
Total comments: 17
Patch Set 5 : Changes as per review comments. #Patch Set 6 : Changes for scrolling based on Timer when no mouse move event comes. #
Total comments: 14
Patch Set 7 : Changes as per review comments. #Patch Set 8 : Addressing nit. #Patch Set 9 : Addressing nit. #Patch Set 10 : Addressing nit. #
Total comments: 34
Patch Set 11 : Changes as per review comments. #Patch Set 12 : Refactoring code by moving scroll drag functionality in separate file. #
Total comments: 23
Patch Set 13 : Changes as per review comments. #
Total comments: 8
Patch Set 14 : Addressing nit. #
Total comments: 12
Patch Set 15 : Changes as per review comments. #
Total comments: 1
Patch Set 16 : Addressing nit. #Patch Set 17 : Changes as per review comments. #
Total comments: 6
Patch Set 18 : Changes as per review comments. #
Total comments: 2
Patch Set 19 : Addressing nit. #
Messages
Total messages: 54 (9 generated)
deepak.m1@samsung.com changed reviewers: + gene@chromium.org, jam@chromium.org
PTAL.
PTAL at my changes. Thanks
PTAL at my changes. Thanks
@gene PTAL and share your thoughts. Thanks
@gene PTAL and share your thoughts. Thanks
deepak.m1@samsung.com changed reviewers: + raymes@chromium.org
PTAL. Thanks
raymes@chromium.org changed reviewers: + sammc@chromium.org
sammc@ is currently looking into this. We might use a slightly different approach because the old way of doing scroll while selecting isn't very usable.
On 2015/01/22 05:07:39, raymes wrote: > sammc@ is currently looking into this. We might use a slightly different > approach because the old way of doing scroll while selecting isn't very usable. I understand, Please consider my request to add me in issue loop for my understanding. Thanks.
Here is the very early start of a CL: https://codereview.chromium.org/865533002/
On 2015/01/22 05:33:51, raymes wrote: > Here is the very early start of a CL: https://codereview.chromium.org/865533002/ Thanks for reference, I will start working using CL: https://codereview.chromium.org/865533002/ as base and will upload changes by tomorrow. Thanks
I have made changes using https://codereview.chromium.org/865533002/ as base. It is working as expected. PTAL and share your thoughts. Thanks
PTAL
https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:441: case 'setIsSelecting': { Match the other cases: no {}. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:443: break; 2 space indent. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:448: this.mousemoveCallback_, false); Line up with the opening ( or break before 'mousemove'. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:465: isPointInsideRect_: function(point, rect) { This doesn't need to be a method. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:466: var x1 = Math.min(rect.x1, rect.x2); Why are these here? https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:489: x: event.pageX / this.viewport_.zoom, Check whether screenX or screenY is between 0 and this.viewport_.size.width or this.viewport_.size.height, respectively. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:498: position.y -= (Viewport.SCROLL_INCREMENT / this.viewport_.zoom); This will only scroll when the mouse moves, and always at a fixed speed. What this should do is set the scroll velocity based on how far the mouse is outside the viewport and use a timer to increment the position by the velocity.
https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:442: if (message.data.isSelecting == this.isSelecting_) Does this ever happen?
Thanks for review. PTAL. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:441: case 'setIsSelecting': { On 2015/01/28 02:43:51, Sam McNally wrote: > Match the other cases: no {}. Done. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:442: if (message.data.isSelecting == this.isSelecting_) On 2015/01/28 03:33:08, raymes wrote: > Does this ever happen? This is not needed as we are sending message when selecting_ is getting changed. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:443: break; On 2015/01/28 02:43:51, Sam McNally wrote: > 2 space indent. Done. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:448: this.mousemoveCallback_, false); On 2015/01/28 02:43:51, Sam McNally wrote: > Line up with the opening ( or break before 'mousemove'. If I bring this.mousemoveCallback_ to start of '(' before 'mousemove' then it goes more than 80 chars. Is the rule of breaking line will go 4 spaces more than the parent line does not works for js. does not found much at http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml. Please correct me if I missed something. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:465: isPointInsideRect_: function(point, rect) { On 2015/01/28 02:43:51, Sam McNally wrote: > This doesn't need to be a method. Done. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:466: var x1 = Math.min(rect.x1, rect.x2); On 2015/01/28 02:43:51, Sam McNally wrote: > Why are these here? Done. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:489: x: event.pageX / this.viewport_.zoom, On 2015/01/28 02:43:51, Sam McNally wrote: > Check whether screenX or screenY is between 0 and this.viewport_.size.width or > this.viewport_.size.height, respectively. Done. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:498: position.y -= (Viewport.SCROLL_INCREMENT / this.viewport_.zoom); On 2015/01/28 02:43:51, Sam McNally wrote: > This will only scroll when the mouse moves, and always at a fixed speed. > > What this should do is set the scroll velocity based on how far the mouse is > outside the viewport and use a timer to increment the position by the velocity. yes, Currently it is scrolling when their is mouse move and at the fixed speed. Locally I tried to increase speed based on how distance mouse is outside the view port. it works well. **In adobe reader scrolling happen with same speed irrespective of mouse position from viewport. But when I tried to use this.timerVal_= setInterval(this.fun.bind(this),timeinms); then when I keep mouse still then based on timer it is calling function and updating scrolling. But 2 issue happen: 1) While scrolling with timer selecting is not updating. 2) while clearing interval by if (this.timerVal_ != null) { clearInterval(this.timerVal_); this.timerVal_ = null; } It is calling clearInterval() but still calls are going to the 'fun' function after every timeinms. can you please suggest clue to address these. Thanks.
On 2015/01/28 11:59:27, Deepak wrote: > Thanks for review. > PTAL. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > File chrome/browser/resources/pdf/pdf.js (right): > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:441: case 'setIsSelecting': { > On 2015/01/28 02:43:51, Sam McNally wrote: > > Match the other cases: no {}. > > Done. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:442: if (message.data.isSelecting == > this.isSelecting_) > On 2015/01/28 03:33:08, raymes wrote: > > Does this ever happen? > > This is not needed as we are sending message when selecting_ is getting changed. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:443: break; > On 2015/01/28 02:43:51, Sam McNally wrote: > > 2 space indent. > > Done. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:448: this.mousemoveCallback_, false); > On 2015/01/28 02:43:51, Sam McNally wrote: > > Line up with the opening ( or break before 'mousemove'. > > If I bring this.mousemoveCallback_ to start of '(' before 'mousemove' then it > goes more than 80 chars. > Is the rule of breaking line will go 4 spaces more than the parent line does not > works for js. > does not found much at > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml. > Please correct me if I missed something. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:465: isPointInsideRect_: function(point, > rect) { > On 2015/01/28 02:43:51, Sam McNally wrote: > > This doesn't need to be a method. > > Done. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:466: var x1 = Math.min(rect.x1, rect.x2); > On 2015/01/28 02:43:51, Sam McNally wrote: > > Why are these here? > > Done. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:489: x: event.pageX / this.viewport_.zoom, > On 2015/01/28 02:43:51, Sam McNally wrote: > > Check whether screenX or screenY is between 0 and this.viewport_.size.width or > > this.viewport_.size.height, respectively. > > Done. > > https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... > chrome/browser/resources/pdf/pdf.js:498: position.y -= > (Viewport.SCROLL_INCREMENT / this.viewport_.zoom); > On 2015/01/28 02:43:51, Sam McNally wrote: > > This will only scroll when the mouse moves, and always at a fixed speed. > > > > What this should do is set the scroll velocity based on how far the mouse is > > outside the viewport and use a timer to increment the position by the > velocity. > > yes, Currently it is scrolling when their is mouse move and at the fixed speed. > Locally I tried to increase speed based on how distance mouse is outside the > view port. it works well. > **In adobe reader scrolling happen with same speed irrespective of mouse > position from viewport. > > But when I tried to use > this.timerVal_= setInterval(this.fun.bind(this),timeinms); > > then when I keep mouse still then based on timer it is calling function and > updating scrolling. But 2 issue happen: > 1) While scrolling with timer selecting is not updating. > 2) while clearing interval by > if (this.timerVal_ != null) { > clearInterval(this.timerVal_); > this.timerVal_ = null; > } > It is calling clearInterval() but still calls are going to the 'fun' function > after every timeinms. > can you please suggest clue to address these. > Thanks. I have done changes as @sammc suggested That is scrolling based on timer when mouse is out of the viewport and mouse move event does not came. But as the selection happens from pdfium_engine.cc file so all the scroll of page based on timer will get selection when mouse move comes. * I have not added changes for increasing speed of scroll that is based on how far is mouse point from viewport, as I observed that in adobe reader also scroll happen with constant speed and does not increase based on mouse position. or we want different behavior in chrome PDF than adobe Reader. PTAL and share your thoughts.
I have done changes as @sammc suggested That is scrolling based on timer when mouse is out of the viewport and mouse move event does not came. But as the selection happens from pdfium_engine.cc file so all the scroll of page based on timer will get selection when mouse move comes. * I have not added changes for increasing speed of scroll that is based on how far is mouse point from viewport, as I observed that in adobe reader also scroll happen with constant speed and does not increase based on mouse position. or we want different behavior in chrome PDF than adobe Reader. PTAL and share your thoughts.
On 2015/02/02 13:54:10, Deepak wrote: > I have done changes as @sammc suggested > That is scrolling based on timer when mouse is out of the viewport > and mouse move event does not came. > But as the selection happens from pdfium_engine.cc file so all the scroll > of page based on timer will get selection when mouse move comes. I think it's reasonable to address this in a follow up CL. > > * I have not added changes for increasing speed of scroll that is based on > how far is mouse point from viewport, as I observed that in adobe reader also > scroll happen with constant speed and does not increase based on mouse position. > or we want different behavior in chrome PDF than adobe Reader. > > PTAL and share your thoughts. I think we should match Chrome's drag scrolling behavior. https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/60001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:448: this.mousemoveCallback_, false); On 2015/01/28 11:59:26, Deepak wrote: > On 2015/01/28 02:43:51, Sam McNally wrote: > > Line up with the opening ( or break before 'mousemove'. > > If I bring this.mousemoveCallback_ to start of '(' before 'mousemove' then it > goes more than 80 chars. > Is the rule of breaking line will go 4 spaces more than the parent line does not > works for js. > does not found much at > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml. > Please correct me if I missed something. See code formatting > function arguments. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:458: this.isSelecting_ = message.data.isSelecting; this.isSelecting_ isn't used anywhere else. Remove it. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:460: this.mousemoveCallback_ = this.selectionDragListener_.bind(this); 2 space indent. Also, only set mousemoveCallback_ once. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:464: this.plugin_.removeEventListener('mousemove', Ditto. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:478: window.clearTimeout(this.timerId_); Use clearInterval here. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:507: this.timerId_ = window.setTimeout(this.dragScrollPage_.bind(this), 50); Remove. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:535: if (this.viewport_.documentHasScrollbars().vertical) { This won't work for diagonal scrolls. Instead, calculate and store a scroll velocity using Math.min(Math.max(-event.offsetX, event.offsetX - this.plugin_.offsetWidth, 0), 100) * Math.sign(event.offsetX); and similarly for y. Then in dragScrollPage_, add the velocity to the viewport position. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:538: this.dragScrollPage_(); Instead of calling dragScrollPage_ here, after calculating the new velocity, call setInterval if the velocity is non-zero and there isn't already a timer running. Also call clearTimer_() if the velocity is zero.
Thanks for review. PTAL. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:458: this.isSelecting_ = message.data.isSelecting; On 2015/02/04 02:56:01, Sam McNally wrote: > this.isSelecting_ isn't used anywhere else. Remove it. Done. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:460: this.mousemoveCallback_ = this.selectionDragListener_.bind(this); On 2015/02/04 02:56:01, Sam McNally wrote: > 2 space indent. > > Also, only set mousemoveCallback_ once. Done. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:464: this.plugin_.removeEventListener('mousemove', On 2015/02/04 02:56:02, Sam McNally wrote: > Ditto. Done. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:478: window.clearTimeout(this.timerId_); On 2015/02/04 02:56:01, Sam McNally wrote: > Use clearInterval here. Done. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:507: this.timerId_ = window.setTimeout(this.dragScrollPage_.bind(this), 50); On 2015/02/04 02:56:01, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:535: if (this.viewport_.documentHasScrollbars().vertical) { On 2015/02/04 02:56:01, Sam McNally wrote: > This won't work for diagonal scrolls. Instead, calculate and store a scroll > velocity using > Math.min(Math.max(-event.offsetX, > event.offsetX - this.plugin_.offsetWidth, 0), > 100) * Math.sign(event.offsetX); > and similarly for y. Then in dragScrollPage_, add the velocity to the viewport > position. Done. https://codereview.chromium.org/814573004/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:538: this.dragScrollPage_(); On 2015/02/04 02:56:01, Sam McNally wrote: > Instead of calling dragScrollPage_ here, after calculating the new velocity, > call setInterval if the velocity is non-zero and there isn't already a timer > running. Also call clearTimer_() if the velocity is zero. Done.
Thanks! https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:638: }, Can we move all this stuff into a new class in a separate file (called ViewportScroller or something). We can pass in the viewport and the plugin and the window.
https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:61: * The timer interval for drag scroll when we have selection and mouse is The period of time to wait between updating the viewport position by the scroll velocity. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:68: * The maximum scroll velocity when we have selection and drag mouse out of The maximum drag scroll velocity. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:540: this.mousemoveCallback_ = this.selectionDragListener_.bind(this); I meant only do this once and then reuse the same callback in the future. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:541: this.plugin_.addEventListener('mousemove', Break after the (. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:546: this.plugin_.removeEventListener('mousemove', Ditto. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:548: this.mousemoveCallback_ = null; Delete. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:557: * Start the timer to call dragScrollPage_ function at regular interval till until the timer is stopped. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:560: setTimerInterval_: function() { startDragScrollTimer_ https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:568: * Clear the timer if it is active and reset timerId_. Stops the drag scroll timer if it is active. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:570: clearTimerInterval_: function() { stopDragScrollTimer_ https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:579: * Handles scrolling in the page when we drag mouse out of the viewport and Scrolls the viewport by the current scroll velocity. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:591: * Calculate velocity of scroll in the x and y direction when we drag mouse Calculate the velocity to scroll while dragging using the distance of the cursor outside the viewport. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:597: var x = Math.min(Math.max(-event.offsetX, event.offsetX - Please clang-format this. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:608: * Handles mousemove events. Scrolls page when mouse move happen reacting to This doesn't do any scrolling itself. It just updates the velocity and maybe starts or stops the timer. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:614: var position = this.viewport_.position; These bounds checks aren't necessary. Remove them. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:634: this.setTimerInterval_(); Please make setTimerInterval_ idempotent and simplify these ifs to a single if/else using the velocity.
@Sam Thanks for review. PTAL. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:61: * The timer interval for drag scroll when we have selection and mouse is On 2015/02/06 03:43:06, Sam McNally wrote: > The period of time to wait between updating the viewport position by the scroll > velocity. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:68: * The maximum scroll velocity when we have selection and drag mouse out of On 2015/02/06 03:43:07, Sam McNally wrote: > The maximum drag scroll velocity. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:540: this.mousemoveCallback_ = this.selectionDragListener_.bind(this); On 2015/02/06 03:43:07, Sam McNally wrote: > I meant only do this once and then reuse the same callback in the future. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:541: this.plugin_.addEventListener('mousemove', On 2015/02/06 03:43:07, Sam McNally wrote: > Break after the (. ok, I will run clang format for this. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:546: this.plugin_.removeEventListener('mousemove', On 2015/02/06 03:43:07, Sam McNally wrote: > Ditto. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:548: this.mousemoveCallback_ = null; On 2015/02/06 03:43:07, Sam McNally wrote: > Delete. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:557: * Start the timer to call dragScrollPage_ function at regular interval till On 2015/02/06 03:43:07, Sam McNally wrote: > until the timer is stopped. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:560: setTimerInterval_: function() { On 2015/02/06 03:43:07, Sam McNally wrote: > startDragScrollTimer_ Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:568: * Clear the timer if it is active and reset timerId_. On 2015/02/06 03:43:06, Sam McNally wrote: > Stops the drag scroll timer if it is active. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:570: clearTimerInterval_: function() { On 2015/02/06 03:43:07, Sam McNally wrote: > stopDragScrollTimer_ Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:579: * Handles scrolling in the page when we drag mouse out of the viewport and On 2015/02/06 03:43:07, Sam McNally wrote: > Scrolls the viewport by the current scroll velocity. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:591: * Calculate velocity of scroll in the x and y direction when we drag mouse On 2015/02/06 03:43:07, Sam McNally wrote: > Calculate the velocity to scroll while dragging using the distance of the cursor > outside the viewport. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:597: var x = Math.min(Math.max(-event.offsetX, event.offsetX - On 2015/02/06 03:43:07, Sam McNally wrote: > Please clang-format this. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:608: * Handles mousemove events. Scrolls page when mouse move happen reacting to On 2015/02/06 03:43:07, Sam McNally wrote: > This doesn't do any scrolling itself. It just updates the velocity and maybe > starts or stops the timer. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:614: var position = this.viewport_.position; On 2015/02/06 03:43:06, Sam McNally wrote: > These bounds checks aren't necessary. Remove them. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:634: this.setTimerInterval_(); On 2015/02/06 03:43:07, Sam McNally wrote: > Please make setTimerInterval_ idempotent and simplify these ifs to a single > if/else using the velocity. Done. https://codereview.chromium.org/814573004/diff/180001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:638: }, On 2015/02/06 03:11:07, raymes wrote: > Can we move all this stuff into a new class in a separate file (called > ViewportScroller or something). We can pass in the viewport and the plugin and > the window. ya, sure , I will definitely move this code in separate file. As this issue is blocker, Is it ok, if do this once I got lgtm for these changes from Sam.?
@raymes Have moved related functionality in separate file. @Sam Changes done as suggested. Thanks, PTAL.
@raymes Have moved related functionality in separate file. @Sam Changes done as suggested. Thanks, PTAL.
https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... File chrome/browser/resources/pdf/viewportscroller.js (right): https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. Let's call this viewport_scroller.js. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:7: Remove. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:12: ViewportScroller.DRAG_TIMER_INTERVAL = 100; I think we can make these private. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:15: * The maximum drag scroll velocity when we have selection and drag mouse out The maximum drag scroll velocity. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:21: * Creates a new ViewportScroller for scrolling viewport when have selection Creates a new ViewportScroller. A ViewportScroller scrolls the page in response to drag selection with the mouse. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:40: * Start the timer to call dragScrollPage_ function at regular interval until Start scrolling the page by |scrollVelocity_| every |DRAG_TIMER_INTERVAL|. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:44: this.timerId_ = this.window_.setInterval( if (this.timerId === null) https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:97: selectionDragListener_: function(event) { onMousemove_ https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:106: * @private Remove. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:107: * Function to add and remove callback function when mousemove event comes. Sets whether to scroll the viewport when the mouse is outside the viewport. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:110: viewportScroll: function(isSelecting) { How about calling this setEnableScrolling?
Thanks for Review. Changes done as suggested. PTAL. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... File chrome/browser/resources/pdf/viewportscroller.js (right): https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/02/11 06:59:13, Sam McNally wrote: > Let's call this viewport_scroller.js. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:7: On 2015/02/11 06:59:13, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:12: ViewportScroller.DRAG_TIMER_INTERVAL = 100; On 2015/02/11 06:59:13, Sam McNally wrote: > I think we can make these private. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:15: * The maximum drag scroll velocity when we have selection and drag mouse out On 2015/02/11 06:59:13, Sam McNally wrote: > The maximum drag scroll velocity. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:21: * Creates a new ViewportScroller for scrolling viewport when have selection On 2015/02/11 06:59:13, Sam McNally wrote: > Creates a new ViewportScroller. > > A ViewportScroller scrolls the page in response to drag selection with the > mouse. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:40: * Start the timer to call dragScrollPage_ function at regular interval until On 2015/02/11 06:59:12, Sam McNally wrote: > Start scrolling the page by |scrollVelocity_| every |DRAG_TIMER_INTERVAL|. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:44: this.timerId_ = this.window_.setInterval( On 2015/02/11 06:59:13, Sam McNally wrote: > if (this.timerId === null) Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:44: this.timerId_ = this.window_.setInterval( On 2015/02/11 06:59:13, Sam McNally wrote: > if (this.timerId === null) Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:97: selectionDragListener_: function(event) { On 2015/02/11 06:59:13, Sam McNally wrote: > onMousemove_ Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:106: * @private On 2015/02/11 06:59:13, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:107: * Function to add and remove callback function when mousemove event comes. On 2015/02/11 06:59:12, Sam McNally wrote: > Sets whether to scroll the viewport when the mouse is outside the viewport. Done. https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... chrome/browser/resources/pdf/viewportscroller.js:110: viewportScroll: function(isSelecting) { On 2015/02/11 06:59:13, Sam McNally wrote: > How about calling this setEnableScrolling? Done.
lgtm please wait for sammc@ https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:10: * @param {Object} plugin The plugin object of PDF. nit "The PDF plugin element." https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:11: * @param {Object} window The window object for PDF. nit: "The window containing the viewer."
LGTM with a couple nits. https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:22: this.DRAG_TIMER_INTERVAL = 100; I meant add a trailing underscore, not turn them into instance members. https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:64: * cursor outside the viewport. @param for event
New patchsets have been uploaded after l-g-t-m from raymes@chromium.org,sammc@chromium.org
Thanks @Raymes and @Sam for review. All nit addressed. Thanks https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:10: * @param {Object} plugin The plugin object of PDF. On 2015/02/12 06:22:31, raymes wrote: > nit "The PDF plugin element." Done. https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:11: * @param {Object} window The window object for PDF. On 2015/02/12 06:22:31, raymes wrote: > nit: "The window containing the viewer." Done. https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:22: this.DRAG_TIMER_INTERVAL = 100; On 2015/02/12 06:27:44, Sam McNally wrote: > I meant add a trailing underscore, not turn them into instance members. Done. https://codereview.chromium.org/814573004/diff/240001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:64: * cursor outside the viewport. On 2015/02/12 06:27:45, Sam McNally wrote: > @param for event Done.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/814573004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
deepak.m1@samsung.com changed reviewers: + bauerb@chromium.org
++Adding @bauerb for component_extension_resources.grd file approval. Thanks
deepak.m1@samsung.com changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:4: 'use strict'; ? https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:10: ViewportScroller.DRAG_TIMER_INTERVAL_ = 100; Please add a unit. https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:16: ViewportScroller.MAX_DRAG_SCROLL_VELOCITY_ = 100; Please add a unit here as well. https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:65: position.y += this.scrollVelocity_.y; If the timer doesn't fire after exactly the |DRAG_TIMER_INTERVAL|, the scrolling might feel a little choppy. Can we adjust this by measuring how much time actually passed and adjusting the offset accordingly?
PTAL https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:4: On 2015/02/12 09:24:18, Bernhard Bauer wrote: > 'use strict'; ? actualy @Sam asked me to remove this earlier. Anyways adding it again. https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:10: ViewportScroller.DRAG_TIMER_INTERVAL_ = 100; On 2015/02/12 09:24:18, Bernhard Bauer wrote: > Please add a unit. Done. https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:16: ViewportScroller.MAX_DRAG_SCROLL_VELOCITY_ = 100; On 2015/02/12 09:24:18, Bernhard Bauer wrote: > Please add a unit here as well. Done. https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:65: position.y += this.scrollVelocity_.y; On 2015/02/12 09:24:18, Bernhard Bauer wrote: > If the timer doesn't fire after exactly the |DRAG_TIMER_INTERVAL|, the scrolling > might feel a little choppy. Can we adjust this by measuring how much time > actually passed and adjusting the offset accordingly? I am sorry, I don't understood this comment completely. Can you please elaborate a little more, or give some reference of usage. Thanks
https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:4: On 2015/02/12 10:13:49, Deepak wrote: > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > 'use strict'; ? > > actualy @Sam asked me to remove this earlier. > Anyways adding it again. You mean in https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... ? I thought that was only refering to the extra empty line (there were two of them). Sam, is that correct? :) https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:10: ViewportScroller.DRAG_TIMER_INTERVAL_ = 100; On 2015/02/12 10:13:49, Deepak wrote: > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > Please add a unit. > > Done. Please also add that to the variable name as a prefix (...INTERVAL_MS_). There is a rule about that somewhere, but I can't find it at the moment. https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:65: position.y += this.scrollVelocity_.y; On 2015/02/12 10:13:49, Deepak wrote: > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > If the timer doesn't fire after exactly the |DRAG_TIMER_INTERVAL|, the > scrolling > > might feel a little choppy. Can we adjust this by measuring how much time > > actually passed and adjusting the offset accordingly? > > I am sorry, I don't understood this comment completely. > Can you please elaborate a little more, or give some reference of usage. > Thanks If we assume we are scrolling at maximum velocity, we want to scroll 100 pixels every 100ms. If the timer fires after 150ms though (because of timer coalescing, or the system being busy), we will move 100 pixels in 150ms, which appears slower. Then if the next timer fires again after 100ms, we will be faster again, so the overall experience might be choppy. If we adjust the scrolling distance to (100 pixels * 150ms / 100ms) = 150 pixels, the perceived speed will be always the same, even if the timer fires a bit earlier or later. https://codereview.chromium.org/814573004/diff/280001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/280001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:16: * The maximum drag scroll velocity in pixels. Pixels isn't really a unit of velocity, pixels per <time unit> is. Or you declare this as a the maximum drag distance per DRAG_TIMER_INTERVAL.
Thanks for review. Changes done as suggested. PTAL. On 2015/02/12 10:31:51, Bernhard Bauer wrote: > https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... > File chrome/browser/resources/pdf/viewport_scroller.js (right): > > https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... > chrome/browser/resources/pdf/viewport_scroller.js:4: > On 2015/02/12 10:13:49, Deepak wrote: > > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > > 'use strict'; ? > > > > actualy @Sam asked me to remove this earlier. > > Anyways adding it again. > > You mean in > https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... > ? I thought that was only refering to the extra empty line (there were two of > them). > > Sam, is that correct? :) > > https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... > chrome/browser/resources/pdf/viewport_scroller.js:10: > ViewportScroller.DRAG_TIMER_INTERVAL_ = 100; > On 2015/02/12 10:13:49, Deepak wrote: > > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > > Please add a unit. > > > > Done. > > Please also add that to the variable name as a prefix (...INTERVAL_MS_). There > is a rule about that somewhere, but I can't find it at the moment. Done. > https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... > chrome/browser/resources/pdf/viewport_scroller.js:65: position.y += > this.scrollVelocity_.y; > On 2015/02/12 10:13:49, Deepak wrote: > > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > > If the timer doesn't fire after exactly the |DRAG_TIMER_INTERVAL|, the > > scrolling > > > might feel a little choppy. Can we adjust this by measuring how much time > > > actually passed and adjusting the offset accordingly? > > > > I am sorry, I don't understood this comment completely. > > Can you please elaborate a little more, or give some reference of usage. > > Thanks > > If we assume we are scrolling at maximum velocity, we want to scroll 100 pixels > every 100ms. If the timer fires after 150ms though (because of timer coalescing, > or the system being busy), we will move 100 pixels in 150ms, which appears > slower. Then if the next timer fires again after 100ms, we will be faster again, > so the overall experience might be choppy. If we adjust the scrolling distance > to (100 pixels * 150ms / 100ms) = 150 pixels, the perceived speed will be always > the same, even if the timer fires a bit earlier or later. Done. > https://codereview.chromium.org/814573004/diff/280001/chrome/browser/resource... > File chrome/browser/resources/pdf/viewport_scroller.js (right): > > https://codereview.chromium.org/814573004/diff/280001/chrome/browser/resource... > chrome/browser/resources/pdf/viewport_scroller.js:16: * The maximum drag scroll > velocity in pixels. > Pixels isn't really a unit of velocity, pixels per <time unit> is. Or you > declare this as a the maximum drag distance per DRAG_TIMER_INTERVAL. Done.
https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:35: this.startTime_ = 0; I would probably call this |lastFrameTime_|. https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:50: this.startTime_ = new Date().getTime(); You can use Date.now() for this. https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:72: this.newTime_ = new Date().getTime(); This can be a local variable.
https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:4: On 2015/02/12 10:31:51, Bernhard Bauer wrote: > On 2015/02/12 10:13:49, Deepak wrote: > > On 2015/02/12 09:24:18, Bernhard Bauer wrote: > > > 'use strict'; ? > > > > actualy @Sam asked me to remove this earlier. > > Anyways adding it again. > > You mean in > https://codereview.chromium.org/814573004/diff/220001/chrome/browser/resource... > ? I thought that was only refering to the extra empty line (there were two of > them). > > Sam, is that correct? :) Yes, just the extra empty line.
Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:35: this.startTime_ = 0; On 2015/02/12 14:25:44, Bernhard Bauer wrote: > I would probably call this |lastFrameTime_|. Done. https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:50: this.startTime_ = new Date().getTime(); On 2015/02/12 14:25:44, Bernhard Bauer wrote: > You can use Date.now() for this. Done. https://codereview.chromium.org/814573004/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:72: this.newTime_ = new Date().getTime(); On 2015/02/12 14:25:44, Bernhard Bauer wrote: > This can be a local variable. Done.
https://codereview.chromium.org/814573004/diff/340001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/340001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:71: var currentFrameTime_ = Date.now(); And local variables don't end with underscores.
Ah! sorry, I missed that. Changes done. PTAL https://codereview.chromium.org/814573004/diff/340001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport_scroller.js (right): https://codereview.chromium.org/814573004/diff/340001/chrome/browser/resource... chrome/browser/resources/pdf/viewport_scroller.js:71: var currentFrameTime_ = Date.now(); On 2015/02/13 10:06:22, Bernhard Bauer wrote: > And local variables don't end with underscores. Done.
Thanks, LGTM.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/814573004/360001
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/423f09c6d2be8c916637cdda28ed0ee4e12490e0 Cr-Commit-Position: refs/heads/master@{#316204} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
