Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400743002:
Improved Pinch-Zoom for PDF. (Closed)
|
Created:
4 years, 2 months ago by Kevin McNee - google account Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImproved Pinch-Zoom for PDF.
Currently, when the PDF viewer is pinch-zoomed, the content is not
rerastered and the viewer's controls are scaled off screen.
We now have the viewer handle the pinch-zoom events, instead of the
browser. We compute scrollbars and scale after a pinch and apply transforms
on the existing bitmap. This allows us to keep the controls fixed and only
scale the content as well as reraster the content when needed.
BUG=605379
patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001)
This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6e1abbfb2450eedddb1ab128be1b31cc93104e41
Cr-Commit-Position: refs/heads/master@{#430973}
Patch Set 1 #
Total comments: 71
Patch Set 2 : Fix presubmit errors, code clean up, and refactor. #
Total comments: 24
Patch Set 3 : Code review changes. #
Total comments: 21
Patch Set 4 : More code review changes. #
Total comments: 12
Patch Set 5 : Clarify names and behaviours. #Patch Set 6 : Use our own touch gesture detection. #
Total comments: 32
Patch Set 7 : Code review changes and GestureDetector tests. #
Total comments: 22
Patch Set 8 : JS code review changes. #
Total comments: 4
Patch Set 9 : Small changes. #
Messages
Total messages: 67 (27 generated)
Description was changed from ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). ========== to ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mcnee@google.com changed reviewers: + bokan@chromium.org, wjmaclean@chromium.org
Hello. I'm taking ownership of this issue. This is a continuation of https://codereview.chromium.org/1901903002/ . The first patch in this new code review is identical to the last patch (6) in the old code review (other than rebasing). I will be adding comments to this review based on any unresolved comments in the last one.
Here are the unresolved comments from the previous code review along with some of my own comments. Here are further unresolved comments that were in the messages: bokan: Please expand on the description. When writing the description, think about the poor soul, who in two years, with no idea about PDF or pinch-zooming, wonders by this code wondering why some variable was added. It should explain why this CL is being made and summarize the major points. E.g. https://codereview.chromium.org/1840113005/. It doesn't have to be long (I tend to have descriptions on the longer side) but should be descriptive enough that someone who doesn't know anything about this can understand at a high level why and what's going on. bokan: And the issues I still saw when trying this patch: -Zoom into the PDF. Once the PDF is rerastered at the new level, it "jumps" a few pixels. ~1cm -Zoom in and out repeatedly (in separate gestures). Occasionally, after a zoom in, there's a 2-3 second delay before the PDF is rastered at the new level -Zooming out is non-ideal since we don't generate the surrounding content when zooming out. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:215: if(!this.didPinch_) { Style: missing space https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:225: if(!this.didPinchEnd_) { Style: missing space https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:227: window.requestAnimationFrame(function() { Comment from previous review: bokan: I don't think we need to rAF the PinchEnd event. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:725: render: doRender, // Render or not Comment from previous review: bokan: I think a better name here would be needsReraster https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:178: updateViewport_: function() { this.viewportChangedCallback_(); }, Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:227: get zoom() { return this.zoom_; }, Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:242: this.pinchCenter_ = { Comments from previous review: wjmaclean: Perhaps add a TODO here: at present the transition between using the window centre and the gesture centre is liable to be a little 'jarring' ... might be nice to think of some way of doing a smooth transition, perhaps a weighted average of the window and gesture centres where the weight transitions smoothly from one to the other over a preset number of frames? bokan: This is the case where we don't have horizontal scrolling so we want to keep the document centered. In that case, I think using the window center is fine (and this feels fine to me). Although we probably don't want to do the centering vertically. alessandroa: I've explained today to David how by having the point in the middle of the view helps us zooming. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:345: get scrollbarWidth() { return this.scrollbarWidth_; }, Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:350: get fittingType() { return this.fittingType_; }, Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:535: fitToPage: function() { this.fitToPageInternal_(true); }, Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:583: this.pinchPanVector_ = this.vectorDelta(ev.center, this.first_pinch_center_in_frame_); Line too long. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:596: this.setPinchZoomInternal_(scaleDelta, this.frameToPluginCoordinate(ev.center)); Line too long. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:624: this.setPinchZoomInternal_(scaleDelta, this.frameToPluginCoordinate(ev.center)); Line too long. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:62: const char kJSPy[] = "py"; Comment from previous review: bokan: more descriptive name please https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:404: if (type == kJSViewportType && Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:434: // Pinch start. Comments from previous review: wjmaclean: Is it worth having an enum like "enum PinchPhase { PinchStart, PinchUpdate, PinchEnd}" to make tracking of current phase more explicit? bokan: +1. In fact, I would argue you should be passing that as an arg into this class rather than doRender and we should figure out the state of doRender in here based on the pinch phase. (And you may want to split up update into PinchInUpdate and PinchOutUpdate since behavior seems to differ based on the two cases). https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:435: if (!do_render && do_render_) { Comment from previous review: bokan: We can also get into this block if we switch from pinch in to pinch out during a gesture, no? bokan: We should replace these with an actual state enum or something of the like. Also, can't we get in here if we switch from pinch-in (do_render_ == true) to pinch-out (!do_render) in one gesture? In that case, it's not really a pinch-start. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:471: // While pinching. Comments from previous review: wjmaclean: See comment about tracking pinch state above ... bokan: The if ... else branch above is also part of "while pinching" right? bokan: I think the above if..else is part of "While Pinching" also. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:481: if (do_render && !do_render_) { Comment from previous review: wjmaclean: PinchEnd state. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:637: paint_manager_.SetSize(view_device_size, device_scale_); Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:102: has_pending_resize_ = true; Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:107: Invalidate(); Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:285: } Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:287: callback_factory_.NewCallback(&PaintManager::OnFlushComplete)); Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:331: manual_callback_pending_ = false; Unnecessary whitespace change. https://codereview.chromium.org/2400743002/diff/1/third_party/hammer/README.c... File third_party/hammer/README.chromium (right): https://codereview.chromium.org/2400743002/diff/1/third_party/hammer/README.c... third_party/hammer/README.chromium:5: Missing 'Security Critical' field. https://codereview.chromium.org/2400743002/diff/1/ui/webui/resources/js/hamme... File ui/webui/resources/js/hammer.js (right): https://codereview.chromium.org/2400743002/diff/1/ui/webui/resources/js/hamme... ui/webui/resources/js/hammer.js:1: // This file serves as a proxy to bring the included js file from /third_party Missing license header.
On 2016/10/06 18:31:17, Kevin McNee wrote: > Hello. I'm taking ownership of this issue. Awesome! Thanks for taking this on!
Thanks for getting this underway again, and for porting over 'state' from the previous review :-) I've take a quick read through the existing comments, and will give a more thorough re-read on the next patch revision. bokan@ - There was a performance / correctness issue that we discussed w.r.t. to the previous patch, and I cannot remember what it was; can you remind me? As I recall it was something that should be addressed as part of landing this patch. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:725: render: doRender, // Render or not On 2016/10/06 21:53:17, Kevin McNee wrote: > Comment from previous review: > bokan: I think a better name here would be needsReraster Agreed, needsReraster is better. Nit: update comment as well. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:242: this.pinchCenter_ = { On 2016/10/06 21:53:17, Kevin McNee wrote: > Comments from previous review: > wjmaclean: Perhaps add a TODO here: at present the transition between using the > window centre and the gesture centre is liable to be a little 'jarring' ... > might be nice to think of some way of doing a smooth transition, perhaps a > weighted average of the window and gesture centres where the weight transitions > smoothly from one to the other over a preset number of frames? I'm fine with making this a todo ("maybedo"?), and using the window centre for now. > bokan: This is the case where we don't have horizontal scrolling so we want to > keep the document centered. In that case, I think using the window center is > fine (and this feels fine to me). Although we probably don't want to do the > centering vertically. > > alessandroa: I've explained today to David how by having the point in the middle > of the view helps us zooming. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:434: // Pinch start. On 2016/10/06 21:53:18, Kevin McNee wrote: > Comments from previous review: > wjmaclean: Is it worth having an enum like "enum PinchPhase { PinchStart, > PinchUpdate, PinchEnd}" to make tracking of current phase more explicit? > > bokan: +1. In fact, I would argue you should be passing that as an arg into this > class rather than doRender and we should figure out the state of doRender in > here based on the pinch phase. (And you may want to split up update into > PinchInUpdate and PinchOutUpdate since behavior seems to differ based on the two > cases). +1 to bokan's comments here. If we split into two functions though, we need to make sure that transitioning between them mid-gesture is handled cleanly (i.e. from a code-readability point of view). https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:435: if (!do_render && do_render_) { On 2016/10/06 21:53:17, Kevin McNee wrote: > Comment from previous review: > bokan: We can also get into this block if we switch from pinch in to pinch out > during a gesture, no? > > bokan: We should replace these with an actual state enum or something of the > like. Also, can't we get in here if we switch from pinch-in (do_render_ == true) > to pinch-out (!do_render) in one gesture? In that case, it's not really a > pinch-start. Presumably that's true. I guess the re-raster status is most important at the *end* of the gesture, when we release.
A few more js code quality comments and a question for bokan@ about requestAnimationFrame at PinchEnd. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:216: this.didPinch_ = true; There are several places where we access viewport's private properties. didPinch_, didPinchEnd_, pinchPanVector_, pinchCenter_, doRender_ are all private to viewport. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:222: }.bind(this.viewport_)); I'm not really a fan of these kind of binds. It makes it harder to figure out what 'this' refers to in the function. Indeed, nowhere else in this directory do these kind of binds occur, so I'll be changing this. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:227: window.requestAnimationFrame(function() { On 2016/10/06 21:53:17, Kevin McNee wrote: > Comment from previous review: > bokan: I don't think we need to rAF the PinchEnd event. The method that this calls, pinchZoomEnd, does a final render, so wouldn't we want to keep this? https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:714: var doRender = this.viewport.doRender_; Consistent use of viewport / viewport_
https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:227: window.requestAnimationFrame(function() { On 2016/10/07 19:08:49, Kevin McNee wrote: > On 2016/10/06 21:53:17, Kevin McNee wrote: > > Comment from previous review: > > bokan: I don't think we need to rAF the PinchEnd event. > > The method that this calls, pinchZoomEnd, does a final render, so wouldn't we > want to keep this? It's been a while since I've looked at this. I think the reason we have rAFs here at all is to throttle the pinch events to 1 per frame. We won't get multiple PinchEnd events in a single frame so it'd be fine to do it immediately. In any case, I think it's a style issue, fine either way.
https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:227: window.requestAnimationFrame(function() { On 2016/10/07 19:16:03, bokan wrote: > On 2016/10/07 19:08:49, Kevin McNee wrote: > > On 2016/10/06 21:53:17, Kevin McNee wrote: > > > Comment from previous review: > > > bokan: I don't think we need to rAF the PinchEnd event. > > > > The method that this calls, pinchZoomEnd, does a final render, so wouldn't we > > want to keep this? > > It's been a while since I've looked at this. I think the reason we have rAFs > here at all is to throttle the pinch events to 1 per frame. We won't get > multiple PinchEnd events in a single frame so it'd be fine to do it immediately. > In any case, I think it's a style issue, fine either way. Oh, I see. I'll double check, but assuming the event behaves nicely, we could simplify this to something like: this.hammertime_.on('pinchend', this.viewport_.pinchZoomEnd.bind(this.viewport_));
https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:227: window.requestAnimationFrame(function() { On 2016/10/07 19:38:11, Kevin McNee wrote: > On 2016/10/07 19:16:03, bokan wrote: > > On 2016/10/07 19:08:49, Kevin McNee wrote: > > > On 2016/10/06 21:53:17, Kevin McNee wrote: > > > > Comment from previous review: > > > > bokan: I don't think we need to rAF the PinchEnd event. > > > > > > The method that this calls, pinchZoomEnd, does a final render, so wouldn't > we > > > want to keep this? > > > > It's been a while since I've looked at this. I think the reason we have rAFs > > here at all is to throttle the pinch events to 1 per frame. We won't get > > multiple PinchEnd events in a single frame so it'd be fine to do it > immediately. > > In any case, I think it's a style issue, fine either way. > > Oh, I see. I'll double check, but assuming the event behaves nicely, we could > simplify this to something like: > this.hammertime_.on('pinchend', > this.viewport_.pinchZoomEnd.bind(this.viewport_)); It looks like this was preventing a race where a pinch update that was scheduled by rAF gets sent after the pinch end event. It looks like the rAF specification preserves order, so it's probably fine to leave the rAF as is, rather than explicitly managing order. We can get rid of the didPinchEnd though, since it only happens once. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:609: this.keepContentCentered_ = true; Verbose https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:410: dict.Get(pp::Var(kJSPy)).is_number()) { Missing check for kJSPinchVectorX and kJSPinchVectorY.
https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:215: if(!this.didPinch_) { On 2016/10/06 21:53:17, Kevin McNee wrote: > Style: missing space Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:216: this.didPinch_ = true; On 2016/10/07 19:08:49, Kevin McNee wrote: > There are several places where we access viewport's private properties. > > didPinch_, didPinchEnd_, pinchPanVector_, pinchCenter_, doRender_ are all > private to viewport. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:222: }.bind(this.viewport_)); On 2016/10/07 19:08:49, Kevin McNee wrote: > I'm not really a fan of these kind of binds. It makes it harder to figure out > what 'this' refers to in the function. Indeed, nowhere else in this directory do > these kind of binds occur, so I'll be changing this. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:225: if(!this.didPinchEnd_) { On 2016/10/06 21:53:17, Kevin McNee wrote: > Style: missing space Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:714: var doRender = this.viewport.doRender_; On 2016/10/07 19:08:49, Kevin McNee wrote: > Consistent use of viewport / viewport_ Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:725: render: doRender, // Render or not On 2016/10/07 12:30:21, wjmaclean wrote: > On 2016/10/06 21:53:17, Kevin McNee wrote: > > Comment from previous review: > > bokan: I think a better name here would be needsReraster > > Agreed, needsReraster is better. > > Nit: update comment as well. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:178: updateViewport_: function() { this.viewportChangedCallback_(); }, On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:227: get zoom() { return this.zoom_; }, On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:242: this.pinchCenter_ = { On 2016/10/07 12:30:21, wjmaclean wrote: > On 2016/10/06 21:53:17, Kevin McNee wrote: > > Comments from previous review: > > wjmaclean: Perhaps add a TODO here: at present the transition between using > the > > window centre and the gesture centre is liable to be a little 'jarring' ... > > might be nice to think of some way of doing a smooth transition, perhaps a > > weighted average of the window and gesture centres where the weight > transitions > > smoothly from one to the other over a preset number of frames? > > I'm fine with making this a todo ("maybedo"?), and using the window centre for > now. > > > bokan: This is the case where we don't have horizontal scrolling so we want to > > keep the document centered. In that case, I think using the window center is > > fine (and this feels fine to me). Although we probably don't want to do the > > centering vertically. > > > > alessandroa: I've explained today to David how by having the point in the > middle > > of the view helps us zooming. > Acknowledged. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:345: get scrollbarWidth() { return this.scrollbarWidth_; }, On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:350: get fittingType() { return this.fittingType_; }, On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:535: fitToPage: function() { this.fitToPageInternal_(true); }, On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:583: this.pinchPanVector_ = this.vectorDelta(ev.center, this.first_pinch_center_in_frame_); On 2016/10/06 21:53:17, Kevin McNee wrote: > Line too long. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:596: this.setPinchZoomInternal_(scaleDelta, this.frameToPluginCoordinate(ev.center)); On 2016/10/06 21:53:17, Kevin McNee wrote: > Line too long. Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:609: this.keepContentCentered_ = true; On 2016/10/13 18:04:02, Kevin McNee wrote: > Verbose Done. https://codereview.chromium.org/2400743002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/viewport.js:624: this.setPinchZoomInternal_(scaleDelta, this.frameToPluginCoordinate(ev.center)); On 2016/10/06 21:53:17, Kevin McNee wrote: > Line too long. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:62: const char kJSPy[] = "py"; On 2016/10/06 21:53:17, Kevin McNee wrote: > Comment from previous review: > bokan: more descriptive name please Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:404: if (type == kJSViewportType && On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:410: dict.Get(pp::Var(kJSPy)).is_number()) { On 2016/10/13 18:04:02, Kevin McNee wrote: > Missing check for kJSPinchVectorX and kJSPinchVectorY. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:434: // Pinch start. On 2016/10/07 12:30:21, wjmaclean wrote: > On 2016/10/06 21:53:18, Kevin McNee wrote: > > Comments from previous review: > > wjmaclean: Is it worth having an enum like "enum PinchPhase { PinchStart, > > PinchUpdate, PinchEnd}" to make tracking of current phase more explicit? > > > > bokan: +1. In fact, I would argue you should be passing that as an arg into > this > > class rather than doRender and we should figure out the state of doRender in > > here based on the pinch phase. (And you may want to split up update into > > PinchInUpdate and PinchOutUpdate since behavior seems to differ based on the > two > > cases). > > +1 to bokan's comments here. If we split into two functions though, we need to > make sure that transitioning between them mid-gesture is handled cleanly (i.e. > from a code-readability point of view). Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:435: if (!do_render && do_render_) { On 2016/10/07 12:30:21, wjmaclean wrote: > On 2016/10/06 21:53:17, Kevin McNee wrote: > > Comment from previous review: > > bokan: We can also get into this block if we switch from pinch in to pinch out > > during a gesture, no? > > > > bokan: We should replace these with an actual state enum or something of the > > like. Also, can't we get in here if we switch from pinch-in (do_render_ == > true) > > to pinch-out (!do_render) in one gesture? In that case, it's not really a > > pinch-start. > > Presumably that's true. I guess the re-raster status is most important at the > *end* of the gesture, when we release. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:471: // While pinching. On 2016/10/06 21:53:18, Kevin McNee wrote: > Comments from previous review: > wjmaclean: See comment about tracking pinch state above ... > > bokan: The if ... else branch above is also part of "while pinching" right? > > bokan: I think the above if..else is part of "While Pinching" also. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:481: if (do_render && !do_render_) { On 2016/10/06 21:53:18, Kevin McNee wrote: > Comment from previous review: > wjmaclean: PinchEnd state. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:637: paint_manager_.SetSize(view_device_size, device_scale_); On 2016/10/06 21:53:17, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:102: has_pending_resize_ = true; On 2016/10/06 21:53:18, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:107: Invalidate(); On 2016/10/06 21:53:18, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:285: } On 2016/10/06 21:53:18, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:287: callback_factory_.NewCallback(&PaintManager::OnFlushComplete)); On 2016/10/06 21:53:18, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/pdf/paint_manager.cc#newcod... pdf/paint_manager.cc:331: manual_callback_pending_ = false; On 2016/10/06 21:53:18, Kevin McNee wrote: > Unnecessary whitespace change. Done. https://codereview.chromium.org/2400743002/diff/1/third_party/hammer/README.c... File third_party/hammer/README.chromium (right): https://codereview.chromium.org/2400743002/diff/1/third_party/hammer/README.c... third_party/hammer/README.chromium:5: On 2016/10/06 21:53:18, Kevin McNee wrote: > Missing 'Security Critical' field. Done. https://codereview.chromium.org/2400743002/diff/1/ui/webui/resources/js/hamme... File ui/webui/resources/js/hammer.js (right): https://codereview.chromium.org/2400743002/diff/1/ui/webui/resources/js/hamme... ui/webui/resources/js/hammer.js:1: // This file serves as a proxy to bring the included js file from /third_party On 2016/10/06 21:53:18, Kevin McNee wrote: > Missing license header. Done.
Description was changed from ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. The Hammer.js library has been added for touch gesture recognition and event handling within the viewer. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Sorry, I'm a little busy at the moment but I'll take a look at it next week!
This is looking good, mostly just nits and comment clarifications, though I'll want to play with this to see how it feels now, either I'll build it myself or ask you for a demo (either will have to wait until Wednesday). > bokan@ - There was a performance / correctness issue that we discussed w.r.t. to > the previous patch, and I cannot remember what it was; can you remind me? As I > recall it was something that should be addressed as part of landing this patch. As mentioned in some of my comments imported from the old patch, there were still some issues I wanted to see resolved though I'm not sure if they're still present in Kevin's updated patch: "bokan: And the issues I still saw when trying this patch: -Zoom into the PDF. Once the PDF is rerastered at the new level, it "jumps" a few pixels. ~1cm -Zoom in and out repeatedly (in separate gestures). Occasionally, after a zoom in, there's a 2-3 second delay before the PDF is rastered at the new level -Zooming out is non-ideal since we don't generate the surrounding content when zooming out." https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:197: // We must preventDefault if there is a two finger touch. By doing so browser Please replace "browser zoom" with "native pinch-zoom" https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:77: PINCH_IN_UPDATE: 2, There's some disagreement in terminology on which direction "in" and "out" are. One camp (which I fall into) says "pinch-in" is the gesture that makes things on the page larger (i.e. "zoom-in"), thereas the other camp says "pinch-in" is the gesture that makes things smaller (i.e. your fingers are literally pinching-in). Please document which terminology we're using here. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:352: * @param {Object} pinch center This comment block is wrong, this converts a point from "frame" to "content" coordinates. i.e. "window" to "page", and the parameters seem to be out of date. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:626: // user can't zoom in on the non-content area. The if block attached to this comment is for the case where we *are* centering. Perhaps this comment was meant with the if block below: |if (!needsScrollbars.horizontal)|. Perhaps the two blocks would be more natural as an if/else but I'll leave the decision to you. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:637: // when we start using the gesture center. IIRC this didn't actually feel that unnatural, but if you want to experiment with something you think might work better that sgtm :) https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:658: this.keepContentCentered_ = (!needsScrollbars.horizontal); Nit: no need for parentheses. https://codereview.chromium.org/2400743002/diff/20001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/20001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:476: needs_reraster_ = pinch_phase != PINCH_OUT_UPDATE; Isn't this guaranteed to be false? https://codereview.chromium.org/2400743002/diff/20001/third_party/hammer/OWNERS File third_party/hammer/OWNERS (right): https://codereview.chromium.org/2400743002/diff/20001/third_party/hammer/OWNE... third_party/hammer/OWNERS:2: bokan@chromium.org Feel free to add yourself here now.
bokan: I found the issue causing the delay for the reraster. It was a problem with the implementation of SetLayerTransform, nothing in this CL. I've uploaded a separate CL for the issue: https://codereview.chromium.org/2444973002/ Also, here's a few more things I noticed while I was exploring PaintManager. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:119: graphics_.Flush( Use the return value of Flush to determine how to set flush_pending_. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:123: transform_ = pp::Point(); Not used. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:325: graphics_.Flush( See above wrt calling Flush. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.h File pdf/paint_manager.h (right): https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.h#new... pdf/paint_manager.h:156: pp::Point origin_, transform_; Not used.
https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:197: // We must preventDefault if there is a two finger touch. By doing so browser On 2016/10/17 22:20:15, bokan wrote: > Please replace "browser zoom" with "native pinch-zoom" Done. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:77: PINCH_IN_UPDATE: 2, On 2016/10/17 22:20:16, bokan wrote: > There's some disagreement in terminology on which direction "in" and "out" are. > One camp (which I fall into) says "pinch-in" is the gesture that makes things on > the page larger (i.e. "zoom-in"), thereas the other camp says "pinch-in" is the > gesture that makes things smaller (i.e. your fingers are literally pinching-in). > Please document which terminology we're using here. Yeah, I was naming these to be consistent with Hammer.js, but I've been getting confused myself. I've changed these to PINCH_UPDATE_ZOOM_OUT and PINCH_UPDATE_ZOOM_IN to make them unambiguous. When we get the pinchin/pinchout information from Hammer.js, I've included a comment to explain that Hammer.js uses pinchin to mean zooming out. Done. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:352: * @param {Object} pinch center On 2016/10/17 22:20:15, bokan wrote: > This comment block is wrong, this converts a point from "frame" to "content" > coordinates. i.e. "window" to "page", and the parameters seem to be out of date. Done. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:626: // user can't zoom in on the non-content area. On 2016/10/17 22:20:16, bokan wrote: > The if block attached to this comment is for the case where we *are* centering. > Perhaps this comment was meant with the if block below: |if > (!needsScrollbars.horizontal)|. Perhaps the two blocks would be more natural as > an if/else but I'll leave the decision to you. Done. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:637: // when we start using the gesture center. On 2016/10/17 22:20:16, bokan wrote: > IIRC this didn't actually feel that unnatural, but if you want to experiment > with something you think might work better that sgtm :) Yeah, after playing with this a bit, I guess it's not as bad as I thought. I changed the wording of the TODO to be more open-ended. Acknowledged. https://codereview.chromium.org/2400743002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:658: this.keepContentCentered_ = (!needsScrollbars.horizontal); On 2016/10/17 22:20:15, bokan wrote: > Nit: no need for parentheses. Done. https://codereview.chromium.org/2400743002/diff/20001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/20001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:476: needs_reraster_ = pinch_phase != PINCH_OUT_UPDATE; On 2016/10/17 22:20:16, bokan wrote: > Isn't this guaranteed to be false? ...oops Done. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:119: graphics_.Flush( On 2016/10/24 15:57:55, Kevin McNee wrote: > Use the return value of Flush to determine how to set flush_pending_. Done. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:123: transform_ = pp::Point(); On 2016/10/24 15:57:55, Kevin McNee wrote: > Not used. Done. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:325: graphics_.Flush( On 2016/10/24 15:57:55, Kevin McNee wrote: > See above wrt calling Flush. Done. https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.h File pdf/paint_manager.h (right): https://codereview.chromium.org/2400743002/diff/20001/pdf/paint_manager.h#new... pdf/paint_manager.h:156: pp::Point origin_, transform_; On 2016/10/24 15:57:55, Kevin McNee wrote: > Not used. Done. https://codereview.chromium.org/2400743002/diff/20001/third_party/hammer/OWNERS File third_party/hammer/OWNERS (right): https://codereview.chromium.org/2400743002/diff/20001/third_party/hammer/OWNE... third_party/hammer/OWNERS:2: bokan@chromium.org On 2016/10/17 22:20:16, bokan wrote: > Feel free to add yourself here now. Done.
Thanks, code lgtm. Could you give me a demo sometime to see how this feels? https://codereview.chromium.org/2400743002/diff/40001/third_party/hammer/OWNERS File third_party/hammer/OWNERS (right): https://codereview.chromium.org/2400743002/diff/40001/third_party/hammer/OWNE... third_party/hammer/OWNERS:3: mcnee@google.com Do you not have an @chromium yet?
mcnee@google.com changed reviewers: + avi@chromium.org, jochen@chromium.org, thestig@chromium.org
avi@chromium.org: Please review changes in ui/ thestig@chromium.org: Please review pdf related changes jochen@chromium.org: Please review changes in third_party/
https://codereview.chromium.org/2400743002/diff/40001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/2400743002/diff/40001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:19: <include name="IDR_WEBUI_HAMMER_JS" file="js/hammer.js" flattenhtml="true" type="BINDATA" /> Is there some kind of "if pdf enabled" flag appropriate to use here so that we don't ship the js if it's not needed?
https://codereview.chromium.org/2400743002/diff/40001/third_party/hammer/OWNERS File third_party/hammer/OWNERS (right): https://codereview.chromium.org/2400743002/diff/40001/third_party/hammer/OWNE... third_party/hammer/OWNERS:3: mcnee@google.com On 2016/10/26 16:30:41, bokan wrote: > Do you not have an @chromium yet? Nope. I never got one. I was just told to use my google account. https://codereview.chromium.org/2400743002/diff/40001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/2400743002/diff/40001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:19: <include name="IDR_WEBUI_HAMMER_JS" file="js/hammer.js" flattenhtml="true" type="BINDATA" /> On 2016/10/27 14:55:26, Avi wrote: > Is there some kind of "if pdf enabled" flag appropriate to use here so that we > don't ship the js if it's not needed? Oddly enough, I did find an enable_pdf flag, but the pdf viewer actually uses enable_plugins for all of its resources. The viewer fails to load when I tried using enable_pdf with hammer, so I'll just use what the viewer does.
On 2016/10/27 14:48:07, Kevin McNee wrote: > mailto:jochen@chromium.org: Please review changes in third_party/ Adding third party code requires a bit more work: https://www.chromium.org/developers/adding-3rd-party-libraries
thestig@chromium.org changed reviewers: + raymes@chromium.org
Raymes can probably do a better review of the JS code. https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:427: } else if (pinch_phase == PINCH_UPDATE_ZOOM_IN) { No else after return https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:238: enum PinchPhase { Add note that this match the .js version. https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:247: double zoom_; Are we duplicating variables? e.g. pdf/pdfium/pdfium_engine.h already has a |zoom_| variable, and a |position_| for the scroll position. https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:248: double old_zoom_, initial_zoom_delta_; 1 decl per line please https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:251: // Scroll at the begining of zooming. typo: beginning https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:115: graphics_.SetLayerTransform(scale, origin, translate); A few methods check for the following. Should this do the same? graphics_.is_null() && !has_pending_resize_) https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:116: if (!flush_pending_) { Does this read better? if (flush_pending_) { flush_requested_ = true; return; } Flush(); https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.h File pdf/paint_manager.h (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.h#new... pdf/paint_manager.h:150: void SetTransform(float scale, pp::Point Origin, pp::Point Transform); Pass pp:Points by const reference.
So after discussing with wjmaclean@, we're thinking it would be better to just implement the pinch zoom detection ourselves, rather than bringing in a third party library for such a simple task. (Kind of like what's been done here: https://cs.chromium.org/chromium/src/ui/file_manager/gallery/js/slide_mode.js...) https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:427: } else if (pinch_phase == PINCH_UPDATE_ZOOM_IN) { On 2016/10/27 16:56:35, Lei Zhang wrote: > No else after return Done. https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:238: enum PinchPhase { On 2016/10/27 16:56:35, Lei Zhang wrote: > Add note that this match the .js version. Done. https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:247: double zoom_; On 2016/10/27 16:56:35, Lei Zhang wrote: > Are we duplicating variables? e.g. pdf/pdfium/pdfium_engine.h already has a > |zoom_| variable, and a |position_| for the scroll position. This variable was here before this CL and it appears to be used in various calculations in this class. Also, it looks like the zoom that pdfium_engine stores is a combination of this zoom factor and the device scale (see https://cs.chromium.org/chromium/src/pdf/out_of_process_instance.cc?l=1421). https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:248: double old_zoom_, initial_zoom_delta_; On 2016/10/27 16:56:36, Lei Zhang wrote: > 1 decl per line please Done. https://codereview.chromium.org/2400743002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:251: // Scroll at the begining of zooming. On 2016/10/27 16:56:36, Lei Zhang wrote: > typo: beginning Done. https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:115: graphics_.SetLayerTransform(scale, origin, translate); On 2016/10/27 16:56:36, Lei Zhang wrote: > A few methods check for the following. Should this do the same? > > graphics_.is_null() && !has_pending_resize_) Yeah, checking graphics_.is_null() is a good idea, but since we're not using the aggregator to perform the change, we should return even if there is a pending resize. Done. https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:116: if (!flush_pending_) { On 2016/10/27 16:56:36, Lei Zhang wrote: > Does this read better? > > if (flush_pending_) { > flush_requested_ = true; > return; > } > Flush(); Done. https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.h File pdf/paint_manager.h (right): https://codereview.chromium.org/2400743002/diff/40001/pdf/paint_manager.h#new... pdf/paint_manager.h:150: void SetTransform(float scale, pp::Point Origin, pp::Point Transform); On 2016/10/27 16:56:36, Lei Zhang wrote: > Pass pp:Points by const reference. Done. https://codereview.chromium.org/2400743002/diff/40001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/2400743002/diff/40001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:19: <include name="IDR_WEBUI_HAMMER_JS" file="js/hammer.js" flattenhtml="true" type="BINDATA" /> On 2016/10/27 16:00:34, Kevin McNee wrote: > On 2016/10/27 14:55:26, Avi wrote: > > Is there some kind of "if pdf enabled" flag appropriate to use here so that we > > don't ship the js if it's not needed? > > Oddly enough, I did find an enable_pdf flag, but the pdf viewer actually uses > enable_plugins for all of its resources. The viewer fails to load when I tried > using enable_pdf with hammer, so I'll just use what the viewer does. Done.
https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:422: old_zoom_ = zoom; From just reading the header, I thought this was the previous |zoom_| value, but this "old zoom" turns out to actually be the zoom value passed in via JS. Isn't that more like a "new zoom" value? It's a bit confusing. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:423: initial_zoom_delta_ = zoom_delta; It's not clear that this is a ratio and not a difference, when just looking at the variable decl in the header. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:445: pp::Point scroll_delta(0, 0); nit: No need to explicitly initialized to (0, 0). https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:447: // use paint_offset to anchor the paint vertically into the same place. We nit: Refer to variables as |varname|. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:242: PINCH_NONE = 0, nit: 2 space indent. https://codereview.chromium.org/2400743002/diff/60001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/60001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:120: if (flush_pending_) { Given the differences in behavior, maybe this method should be called SetTransformAndFlush() ?
https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:422: old_zoom_ = zoom; On 2016/10/27 23:04:33, Lei Zhang wrote: > From just reading the header, I thought this was the previous |zoom_| value, but > this "old zoom" turns out to actually be the zoom value passed in via JS. Isn't > that more like a "new zoom" value? It's a bit confusing. If I'm following alessandroa's logic correctly, this is only actually relevant to the was_smaller_ case to track the zoom from the previous call, so I've updated the name accordingly and removed the unneeded initialization here. Done. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:423: initial_zoom_delta_ = zoom_delta; On 2016/10/27 23:04:33, Lei Zhang wrote: > It's not clear that this is a ratio and not a difference, when just looking at > the variable decl in the header. Done. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:445: pp::Point scroll_delta(0, 0); On 2016/10/27 23:04:33, Lei Zhang wrote: > nit: No need to explicitly initialized to (0, 0). Done. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:447: // use paint_offset to anchor the paint vertically into the same place. We On 2016/10/27 23:04:33, Lei Zhang wrote: > nit: Refer to variables as |varname|. Done. https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/2400743002/diff/60001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:242: PINCH_NONE = 0, On 2016/10/27 23:04:33, Lei Zhang wrote: > nit: 2 space indent. Done. https://codereview.chromium.org/2400743002/diff/60001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/2400743002/diff/60001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:120: if (flush_pending_) { On 2016/10/27 23:04:33, Lei Zhang wrote: > Given the differences in behavior, maybe this method should be called > SetTransformAndFlush() ? I've made the behaviour w.r.t. flushing more explicit. Done.
raymes@chromium.org changed reviewers: + dsinclair@chromium.org
I'm a bit busy with other things to review new PDF features (which I'm not really working on anymore). dsinclair seems to be getting more involved though - he may be interested in reviewing?
please follow the process outlined here: https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review to add a new third_party library
Description was changed from ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. The Hammer.js library has been added for touch gesture recognition and event handling within the viewer. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
As discussed, we're now using our own gesture detection rather than bringing in a third party library. The patch no longer involves changes to third_party/ or ui/, just changes to the pdf viewer.
thestig@chromium.org changed reviewers: + dpapad@chromium.org
On 2016/10/31 00:00:45, raymes wrote: > I'm a bit busy with other things to review new PDF features (which I'm not > really working on anymore). > > dsinclair seems to be getting more involved though - he may be interested in > reviewing? Not sure about dsinclair, but I'm surely not qualified to review JS. Also adding dpapad who has kindly offered to help take a look.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm on the C++ side https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:285: last_current_scroll_(0, 0), Not needed. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:286: was_smaller_(false), Also initialize |last_zoom_when_smaller_|. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:254: // Scroll at the beginning of zooming. nit: s/Scroll/Scroll position/ ? https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:255: pp::FloatPoint last_current_scroll_; Does "last current" sounds weird to you? https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:257: bool was_smaller_; Maybe "last_bitmap_smaller_" since it's not obvious what "was smaller" ? https://codereview.chromium.org/2400743002/diff/90001/pdf/paint_manager.h File pdf/paint_manager.h (right): https://codereview.chromium.org/2400743002/diff/90001/pdf/paint_manager.h#new... pdf/paint_manager.h:202: bool flush_pending_; Are all four possible state combinations of |flush_pending_| and |flush_requested_| valid? If not and there's only 3 possible states, replace both with an enum?
https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/gesture_detector.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:40: if (this.listeners_[type]) { Since you are already using ES6 in this file ("class" keyword), can we also use a proper Map instead of a a plain object? this.listeners_ = new Map([ ['pinchstart', []], ['pinchupdate', []], ['pinchend', []], ]); ... if (this.listeners_.has(type)) { this.listeners_.get(type).push(listener); } https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:51: var listeners = this.listeners_[pinchEvent.type]; Nit(optional): s/var/let throughout this file? https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:53: for (var i = 0; i < numListeners; i++) { Nit for (let l of listeners) l(pinchEvent); https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:63: onTouchStart_(ev) { Let's be consistent within this file, s/ev/event (because of pinchEvent instead of pinchEv above). If you prefer to use an abbreviation, I think just 'e' is more established for events than 'ev'. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:128: if (distance1 === 0) return distance1 === 0 ? null : distance2 / distance1; https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:194: this.gesture_detector_ = new GestureDetector(this.plugin_); Nit: s/gesture_detector/gestureDetector https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:201: The PDFViwer constructor is a huge function. Perhaps consider moving those listeners to private member functions (onPinchUpdate_, onPinchEnd_ etc) ? https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:310: * @param {Object} center the pinch center in content coordinates If Object can't be null, use !Object instead. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:353: * @return {Object} the content coordinates Is this code actually being compiled by Closure Compiler, or are the annotations mostly used for documentation? You could create a typedef for {x: number, y: number} and use that, instead of using Object and having less type coverage across this file. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:557: * @param {Object} First Point !Object, here and elsewhere.
https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:353: * @return {Object} the content coordinates On 2016/10/31 23:02:23, dpapad wrote: > Is this code actually being compiled by Closure Compiler, or are the annotations > mostly used for documentation? > > You could create a typedef for {x: number, y: number} and use that, instead of > using Object and having less type coverage across this file. We just use it for documentation right now. They're very imperfect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jochen@chromium.org changed reviewers: - jochen@chromium.org
since there are no more third_party changes, I assume I'm no longer needed
On 2016/11/02 10:55:06, jochen wrote: > since there are no more third_party changes, I assume I'm no longer needed Right. And avi@, since I brought you in for the ui/ changes which are now gone, feel free to remove yourself if you like. Once I'm done traveling, I'll be addressing the remaining code review comments.
In addition to addressing the code review comments, I've gone ahead and added some basic tests for the GestureDetector. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/gesture_detector.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:40: if (this.listeners_[type]) { On 2016/10/31 23:02:23, dpapad wrote: > Since you are already using ES6 in this file ("class" keyword), can we also use > a proper Map instead of a a plain object? > > this.listeners_ = new Map([ > ['pinchstart', []], > ['pinchupdate', []], > ['pinchend', []], > ]); > ... > if (this.listeners_.has(type)) { > this.listeners_.get(type).push(listener); > } Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:51: var listeners = this.listeners_[pinchEvent.type]; On 2016/10/31 23:02:23, dpapad wrote: > Nit(optional): s/var/let throughout this file? Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:53: for (var i = 0; i < numListeners; i++) { On 2016/10/31 23:02:23, dpapad wrote: > Nit > for (let l of listeners) > l(pinchEvent); Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:63: onTouchStart_(ev) { On 2016/10/31 23:02:23, dpapad wrote: > Let's be consistent within this file, s/ev/event (because of pinchEvent instead > of pinchEv above). If you prefer to use an abbreviation, I think just 'e' is > more established for events than 'ev'. Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/gesture_detector.js:128: if (distance1 === 0) On 2016/10/31 23:02:23, dpapad wrote: > return distance1 === 0 ? null : distance2 / distance1; Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:194: this.gesture_detector_ = new GestureDetector(this.plugin_); On 2016/10/31 23:02:23, dpapad wrote: > Nit: s/gesture_detector/gestureDetector Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:201: On 2016/10/31 23:02:23, dpapad wrote: > The PDFViwer constructor is a huge function. Perhaps consider moving those > listeners to private member functions (onPinchUpdate_, onPinchEnd_ etc) ? Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:310: * @param {Object} center the pinch center in content coordinates On 2016/10/31 23:02:23, dpapad wrote: > If Object can't be null, use !Object instead. Done. https://codereview.chromium.org/2400743002/diff/90001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:557: * @param {Object} First Point On 2016/10/31 23:02:23, dpapad wrote: > !Object, here and elsewhere. Done. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:285: last_current_scroll_(0, 0), On 2016/10/31 22:49:04, Lei Zhang wrote: > Not needed. Done. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:286: was_smaller_(false), On 2016/10/31 22:49:04, Lei Zhang wrote: > Also initialize |last_zoom_when_smaller_|. Done. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:254: // Scroll at the beginning of zooming. On 2016/10/31 22:49:04, Lei Zhang wrote: > nit: s/Scroll/Scroll position/ ? Done. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:255: pp::FloatPoint last_current_scroll_; On 2016/10/31 22:49:04, Lei Zhang wrote: > Does "last current" sounds weird to you? Done. https://codereview.chromium.org/2400743002/diff/90001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:257: bool was_smaller_; On 2016/10/31 22:49:04, Lei Zhang wrote: > Maybe "last_bitmap_smaller_" since it's not obvious what "was smaller" ? Done. https://codereview.chromium.org/2400743002/diff/90001/pdf/paint_manager.h File pdf/paint_manager.h (right): https://codereview.chromium.org/2400743002/diff/90001/pdf/paint_manager.h#new... pdf/paint_manager.h:202: bool flush_pending_; On 2016/10/31 22:49:04, Lei Zhang wrote: > Are all four possible state combinations of |flush_pending_| and > |flush_requested_| valid? If not and there's only 3 possible states, replace > both with an enum? Yes, all are valid: When |flush_requested_| is false, the behaviour of |flush_pending_| is the same as before. When |flush_requested_| is true and |flush_pending_| is true, we have an ongoing flush and the request for an additional flush after completion. When |flush_requested_| is true and |flush_pending_| is false, then a flush has completed and there have been no flushes scheduled by paints before we carry out the flush request.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avi@chromium.org changed reviewers: - avi@chromium.org
Wandering off then...
https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:75: PINCH_NONE: 0, Nit: You can probably drop PINCH_ prefix from all values, without any loss of useful information. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:263: return this.pinchCenter_; This member variable does not appear anywhere in the constructor. It is basically non existent until it is augmented wihith pinchZoom. Can you declare it in the constructor, document its type and perhaps give it a default value of null? https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:314: throw 'Called Viewport.setZoomInternal_ without calling ' + Could this use assert() from https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=18? If not, can you throw a proper Error instead? throw new Error('Called ......'); https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:342: * @param {number} scale the new scale level Per styleguide: "Complete sentences are recommended but not required. Complete sentences should use appropriate capitalization and punctuation." s/the new scale level/The new scale level./ https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:353: * @return {Object} the content coordinates !Object https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:615: this.pinchPhase_ = (e.direction == 'out') ? Nit: No need for parentheses. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:662: this.first_pinch_center_in_frame_ = e.center; s/first_pinch_center_in_frame_/firstPinchCenterInFrame_/ Also can you declare and initialize this in the constructor? https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:678: frameToPluginCoordinate: function(coordinateInFrame) { clampScale(), verctorDelta() and frameToPluginCoordinate() don't use |this|. Can you make them functions instead of member methods, similar to getIntersectionHeight at the top of this file? https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... File chrome/test/data/pdf/gesture_detector_test.js (right): https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... chrome/test/data/pdf/gesture_detector_test.js:19: if (this.listeners_.has(type)) { Let's be consistent within this file regarding when to use or omit curly braces. https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... chrome/test/data/pdf/gesture_detector_test.js:27: for (let l of listeners) for (let l of this.listeners_.get(event.type)) l(event); https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... chrome/test/data/pdf/gesture_detector_test.js:86: chrome.test.assertEq('pinchupdate', pinchListener.lastEvent.type); There are multiple blocks of code that assert lastEvent type, scaleRatio, direction, startScaleRation, center.x, center.y. Can you avoid the code duplication by creating a helper assertEventEquals(expected, actual) {...} and use it as follows var expected = {type: '...', scaleRatio: ...', .... }; assertEventEquals(expected, pinchListener.lastEvent);
https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:75: PINCH_NONE: 0, On 2016/11/08 18:32:34, dpapad wrote: > Nit: You can probably drop PINCH_ prefix from all values, without any loss of > useful information. This is to be consistent with the C++ enum in pdf/out_of_process_instance.h where we want prefixes for clarity. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:263: return this.pinchCenter_; On 2016/11/08 18:32:35, dpapad wrote: > This member variable does not appear anywhere in the constructor. It is > basically non existent until it is augmented wihith pinchZoom. Can you declare > it in the constructor, document its type and perhaps give it a default value of > null? Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:314: throw 'Called Viewport.setZoomInternal_ without calling ' + On 2016/11/08 18:32:35, dpapad wrote: > Could this use assert() from > https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=18? > > If not, can you throw a proper Error instead? > throw new Error('Called ......'); Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:342: * @param {number} scale the new scale level On 2016/11/08 18:32:35, dpapad wrote: > Per styleguide: "Complete sentences are recommended but not required. Complete > sentences should use appropriate capitalization and punctuation." > > s/the new scale level/The new scale level./ Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:353: * @return {Object} the content coordinates On 2016/11/08 18:32:35, dpapad wrote: > !Object Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:615: this.pinchPhase_ = (e.direction == 'out') ? On 2016/11/08 18:32:35, dpapad wrote: > Nit: No need for parentheses. Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:662: this.first_pinch_center_in_frame_ = e.center; On 2016/11/08 18:32:35, dpapad wrote: > s/first_pinch_center_in_frame_/firstPinchCenterInFrame_/ > > Also can you declare and initialize this in the constructor? Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:678: frameToPluginCoordinate: function(coordinateInFrame) { On 2016/11/08 18:32:35, dpapad wrote: > clampScale(), verctorDelta() and frameToPluginCoordinate() don't use |this|. Can > you make them functions instead of member methods, similar to > getIntersectionHeight at the top of this file? Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... File chrome/test/data/pdf/gesture_detector_test.js (right): https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... chrome/test/data/pdf/gesture_detector_test.js:19: if (this.listeners_.has(type)) { On 2016/11/08 18:32:35, dpapad wrote: > Let's be consistent within this file regarding when to use or omit curly braces. Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... chrome/test/data/pdf/gesture_detector_test.js:27: for (let l of listeners) On 2016/11/08 18:32:35, dpapad wrote: > for (let l of this.listeners_.get(event.type)) > l(event); Done. https://codereview.chromium.org/2400743002/diff/110001/chrome/test/data/pdf/g... chrome/test/data/pdf/gesture_detector_test.js:86: chrome.test.assertEq('pinchupdate', pinchListener.lastEvent.type); On 2016/11/08 18:32:35, dpapad wrote: > There are multiple blocks of code that assert lastEvent type, scaleRatio, > direction, startScaleRation, center.x, center.y. > > Can you avoid the code duplication by creating a helper > assertEventEquals(expected, actual) {...} > > and use it as follows > > var expected = {type: '...', scaleRatio: ...', .... }; > assertEventEquals(expected, pinchListener.lastEvent); It looks like chrome.test.assertEq compares objects by value, so I just used that. Done.
JS LGTM https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... chrome/browser/resources/pdf/pdf.js:694: var pinchVector = this.viewport_.pinchPanVector; Nit: var pinchVector = this.viewport_.pinchPanVector || {x: 0, y: 0}; var pinchCenter = this.viewport_.pinchCenter || {x: 0, y: 0}; Then you don't need lines 698-701 https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:379: return { For a future CL cleanup, I think this entire class would benefit from a helper Point class that can perform basic operations on its x,y (shift, scale, diff), instead of duplicating operations on plain {x,y} objects. For example this could look as // Assuming this.position was also a Point instance. return framePoint.shift(this.position).scale(this.zoom);
https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... chrome/browser/resources/pdf/pdf.js:694: var pinchVector = this.viewport_.pinchPanVector; On 2016/11/09 01:12:26, dpapad wrote: > Nit: > var pinchVector = this.viewport_.pinchPanVector || {x: 0, y: 0}; > var pinchCenter = this.viewport_.pinchCenter || {x: 0, y: 0}; > > Then you don't need lines 698-701 Done. https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2400743002/diff/130001/chrome/browser/resourc... chrome/browser/resources/pdf/viewport.js:379: return { On 2016/11/09 01:12:27, dpapad wrote: > For a future CL cleanup, I think this entire class would benefit from a helper > Point class that can perform basic operations on its x,y (shift, scale, diff), > instead of duplicating operations on plain {x,y} objects. For example this could > look as > > // Assuming this.position was also a Point instance. > return framePoint.shift(this.position).scale(this.zoom); I've added a TODO about this. Acknowledged.
The CQ bit was checked by mcnee@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mcnee@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, thestig@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2400743002/#ps150001 (title: "Small changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improved Pinch-Zoom for PDF. Currently, when the PDF viewer is pinch-zoomed, the content is not rerastered and the viewer's controls are scaled off screen. We now have the viewer handle the pinch-zoom events, instead of the browser. We compute scrollbars and scale after a pinch and apply transforms on the existing bitmap. This allows us to keep the controls fixed and only scale the content as well as reraster the content when needed. BUG=605379 patch from issue 1901903002 at patchset 80001 (http://crrev.com/1901903002#ps80001) This is a continuation of 1901903002 under a new owner (alessandroa -> mcnee). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6e1abbfb2450eedddb1ab128be1b31cc93104e41 Cr-Commit-Position: refs/heads/master@{#430973} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6e1abbfb2450eedddb1ab128be1b31cc93104e41 Cr-Commit-Position: refs/heads/master@{#430973} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
