Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by alessandroa Modified:
4 years ago Reviewers:
bokan, wjmaclean, nishila2 CC:
arv+watch_chromium.org, chromium-reviews, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@pepper_add_set_layer_transform Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImproved Pinch-Zoom for PDF.
-> Added Hammer.js for touch events handling.
-> Computed scrollbars & scale after pinch.
-> Applied transforms on existing bitmap.
BUG=605379
Patch Set 1 #Patch Set 2 : Code clean part 1 #Patch Set 3 : Clean code part 2 #
Total comments: 101
Patch Set 4 : Fixed some of the comments #Patch Set 5 : Another patch of fixes #
Total comments: 9
Patch Set 6 : Added LICENSE file #
Total comments: 4
Created: 4 years, 8 months ago
Depends on Patchset: Messages
Total messages: 25 (6 generated)
Description was changed from ========== Improved Pinch-Zoom for PDF BUG= ========== to ========== Improved Pinch-Zoom for PDF BUG= ==========
alessandroa@google.com changed reviewers: + bokan@chromium.org, wjmaclean@chromium.org
Description was changed from ========== Improved Pinch-Zoom for PDF BUG= ========== to ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG= ==========
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= ========== to ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=605379#c1 ==========
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=https://bugs.chromium.org/p/chromium/issues/detail?id=605379#c1 ========== to ========== Improved Pinch-Zoom for PDF. -> Added Hammer.js for touch events handling. -> Computed scrollbars & scale after pinch. -> Applied transforms on existing bitmap. BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=605379 ==========
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=https://bugs.chromium.org/p/chromium/issues/detail?id=605379 ========== 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 ==========
I'm about half way through but need to go to bed, figured I'd release my comments so far :) 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. Also, should the hammer.js LICENSE be empty? https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/index.html (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/index.html:47: Remove extra line https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:220: // Setup hammer.js touch event listener. Comments should describe "why", not "what". In this case, "why are we adding hammer.js?" https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:223: // We must preventDefault if there are is a two fingers touch. "are is" -> "is" "fingers" -> "finger" Same as above, "why must we preventDefault if there's a two finger touch?" https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:228: } else Else block should also have curly braces {} (if one branch has curly braces, both should have) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:234: this.hammertime_.get('pinch').set({enable: true}); I think you only need the "enable: false" branch in touchend and "enable: start" in touchstart. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:247: this.pinchZoom(ev); The way these are set up, if "pinch" is called 5 times in one frame, we'll call this function 5 times when requestAnimationFrames are fired. What you really want is something like this: var didPinch = false; on('pinch', function(ev) { if (!didPinch) { didPinch = true; window.requestAnimationFrame(function() { didPinch = false; this.pinchZoom(ev); }).bind(this)) } }).bind(this.viewport_); https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:490: print_: function() { this.plugin_.postMessage({type: 'print'}); }, Undo the whitespace change (makes going back in time with `git blame` more tedious) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:917: get viewport() { return this.viewport_; }, Ditto on whitespace here and below https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:274: * Sets the cusotm zoom of the viewport. "cusotm" -> "custom" https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:575: this.doRender_ = 1; Should doRender be true/false rather than a numeric value? https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:600: //By doing so we ensure the image_data_ in the plugin is the correct one Shouldn't doRender_ be = 1 then? Or I'm misunderstanding the comment...
A few comments. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:723: doRender = false; I don't understand ... isn't this the equivalent of var doRender = this.viewport_.doRender_; ? https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:239: this.pinchCenter_ = { 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? https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:522: vectorDistance: function(p1, p2) { Maybe call the vectorDelta instead of invoking the notion of distance, since this really isn't a distance? https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:575: this.doRender_ = 1; Agreed, seems like doRender should be a Boolean. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:584: // We must change the new content in center because now we have fake I'm never comfortable with this term 'fake scrollbars'. We either have scrollbars or we don't, so please explain what you mean by fake :-) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:600: //By doing so we ensure the image_data_ in the plugin is the correct one On 2016/04/21 at 02:24:01, bokan wrote: > Shouldn't doRender_ be = 1 then? Or I'm misunderstanding the comment... I would have thought that we are continuing to use the rendering (texture) from before the pinch started, which (if everything is working correctly) should be completely up to date. Is that correct? https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (left): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:571: Undo white-space changes. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:420: // If we have the grey parts we will use paint_offset to anchor the paint "have the grey parts" -> "the rendered document doesn't fill the display area" (someday we might change the background colour to something other than grey) https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:425: // Pinch start. Is it worth having an enum like "enum PinchPhase { PinchStart, PinchUpdate, PinchEnd}" to make tracking of current phase more explicit? https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:448: if(was_smaller_) { space after 'if' https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:462: // While pinching. See comment about tracking pinch state above ... https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:471: // position. PinchEnd state. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:798: Ditto. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:946: if (!image_data_.is_null()) { You don't need the curly braces here. I assume this crept in during development, but it should be restored to its original state. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1395: available_area_.Offset((available_area_.width() - doc_width) / 2 , 0); Undo this change. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1450: Undo. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:229: int do_render_; Make do_render_ bool. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:232: // True if last bitmap was smaller then screen. "then" -> "than" By "screen", we mean the PDF display area in the tab? https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc File pdf/paint_manager.cc (left): https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ol... pdf/paint_manager.cc:128: Undo white space changes. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ol... pdf/paint_manager.cc:166: Ditto. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:216: Remove blank line. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:223: } Undo this change ... https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/js/h... File ui/webui/resources/js/hammer.js (right): https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/js/h... ui/webui/resources/js/hammer.js:5: Remove blank line. https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:20: Remove blank line.
Some comments on viewport.js and pdf.js https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:722: if (!this.viewport_.doRender_) This can just be doRender = this.viewport_.doRender https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:302: this.prevScale *= scaleDelta; prevScale is used entirely in the handlers to calculate the delta so it makes more sense to set it at the end of pinchZoom(). In fact, after calculating scaleDelta you can set prevScale = ev.scale; https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:322: x: (framePoint.x + this.position.x) / scale, scale is always this.zoom_ so remove the argument and just use this.zoom_ here. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:323: y: (framePoint.y + this.position.y) / scale This doesn't really get you into the content since position.x doesn't take into account the "gutter". So if you're zoomed out far so that you have ~200px on each side of the document, this.position.x = 0. I'd consider this an existing bug since position.y takes into account the top gutter. I suspect this is the cause of some of the position jumps. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:574: if (ev.additionalEvent == 'pinchin') Add a comment explaining why we render only on pinchin https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:585: // scrollbars This comment should elaborate on what we're doing and why. Something along the lines of: "If there's no horizontal scrolling, keep the content centered so the user can't zoom in on the non-content area". The changeCenter_ variable is likewise vague. It might be more descriptive as "keepContentCentered_" or similar. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:613: this.pinch_start_ = ev.center; More descriptive name please. pinch_start_ -> first_pinch_center_in_frame_ https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:628: containerCenter: function(pageCenter) { This function would be better described as "frameToPluginCoordinate". The argument should just be called "coordinateInFrame". container.getBoundingClientRect().{left|top} will always be 0 today but I think its good to keep this if that ever changes. Unless you meant this function to convert into the "content" (i.e. non-grey gutter), in which case this is not what that does.
https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:239: this.pinchCenter_ = { On 2016/04/21 14:11:01, wjmaclean wrote: > 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? 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. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:584: // We must change the new content in center because now we have fake On 2016/04/21 14:11:01, wjmaclean wrote: > I'm never comfortable with this term 'fake scrollbars'. We either have > scrollbars or we don't, so please explain what you mean by fake :-) Unrelated to this, but fun anecdote anyway, did you know that the scrollbars in the PDF viewer are actually caused by keeping an invisible DIV behind the plugin and growing it as the plugin zooms so that it causes scrollbars? In that sense, they really are fake :) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:600: //By doing so we ensure the image_data_ in the plugin is the correct one On 2016/04/21 14:11:01, wjmaclean wrote: > On 2016/04/21 at 02:24:01, bokan wrote: > > Shouldn't doRender_ be = 1 then? Or I'm misunderstanding the comment... > > I would have thought that we are continuing to use the rendering (texture) from > before the pinch started, which (if everything is working correctly) should be > completely up to date. Is that correct? If so, this comment is misleading.
https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:323: y: (framePoint.y + this.position.y) / scale On 2016/04/21 15:22:06, bokan wrote: > This doesn't really get you into the content since position.x doesn't take into > account the "gutter". So if you're zoomed out far so that you have ~200px on > each side of the document, this.position.x = 0. I'd consider this an existing > bug since position.y takes into account the top gutter. I suspect this is the > cause of some of the position jumps. You can disregard this comment. While strange, there's a sort of consistency here, the top gutter is considered part of the "scrollable area" while the side gutters are not. Strange but...whatever.
https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:277: * @param {Object} center the pinch center Describe the coordinates of the center value. i.e. "the pinch center in frame coordinates" or "the pinch center in content coordinates" https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:601: this.doRender_ = 0; This isn't inside a mightZoom though so we won't send the "viewport" message to the plugin until we call pinchZoom but there we set doRender so this will get clobbered. Either remove this line or maybe we need pinchZoomStart to also use mightZoom_. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:68: const char kJSPy[] = "py"; more descriptive name please https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:69: const char kJSPinchVectorX[] = "pinchVectorX"; add a comment to say what pinch vector is (i.e. the amount of panning caused by the pinch gesture) https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:282: last_current_scroll_(0,0), nit: space after comma: (0,0) -> (0, 0) https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:410: // Pinch vector is the vector that computes fingers panning. These kinds of comments belong where kJSPinchVector is declared. The comment should also be more detailed. "Is the panning caused due to change in pinch center between start and end of the gesture". Also, what coordinate space is it in? (it's in frame coords here) Everywhere you have vectors/points you should be sure to document either in comments or, even better, in the name, what the coordinates are. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:425: // Pinch start. On 2016/04/21 14:11:02, wjmaclean wrote: > Is it worth having an enum like "enum PinchPhase { PinchStart, PinchUpdate, > PinchEnd}" to make tracking of current phase more explicit? +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/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:426: if (!do_render && do_render_) { We can also get into this block if we switch from pinch in to pinch out during a gesture, no? https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:439: paint_offset = pp::Point(0, (1 - zoom_delta) * pinch_center.y()); I don't understand what we're doing here. Why are we centering vertically? https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:462: // While pinching. The if ... else branch above is also part of "while pinching" right? https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:214: if (pending_device_scale_ != device_scale_) Is this related to the pinch zoom functionality or is this an unrelated fix?
I fixed part of them :) still wip https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/index.html (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/index.html:47: On 2016/04/21 02:24:00, bokan wrote: > Remove extra line Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:220: // Setup hammer.js touch event listener. On 2016/04/21 02:24:00, bokan wrote: > Comments should describe "why", not "what". In this case, "why are we adding > hammer.js?" Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:223: // We must preventDefault if there are is a two fingers touch. On 2016/04/21 02:24:00, bokan wrote: > "are is" -> "is" > "fingers" -> "finger" > > Same as above, "why must we preventDefault if there's a two finger touch?" Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:228: } else On 2016/04/21 02:24:00, bokan wrote: > Else block should also have curly braces {} (if one branch has curly braces, > both should have) Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:234: this.hammertime_.get('pinch').set({enable: true}); On 2016/04/21 02:24:00, bokan wrote: > I think you only need the "enable: false" branch in touchend and "enable: start" > in touchstart. Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:247: this.pinchZoom(ev); On 2016/04/21 02:24:00, bokan wrote: > The way these are set up, if "pinch" is called 5 times in one frame, we'll call > this function 5 times when requestAnimationFrames are fired. What you really > want is something like this: > > var didPinch = false; > > on('pinch', function(ev) { > if (!didPinch) { > didPinch = true; > window.requestAnimationFrame(function() { > didPinch = false; > this.pinchZoom(ev); > }).bind(this)) > } > }).bind(this.viewport_); Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:490: print_: function() { this.plugin_.postMessage({type: 'print'}); }, On 2016/04/21 02:24:00, bokan wrote: > Undo the whitespace change (makes going back in time with `git blame` more > tedious) Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:722: if (!this.viewport_.doRender_) On 2016/04/21 15:22:06, bokan wrote: > This can just be doRender = this.viewport_.doRender Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:723: doRender = false; On 2016/04/21 14:11:01, wjmaclean wrote: > I don't understand ... isn't this the equivalent of > > var doRender = this.viewport_.doRender_; > > ? I changed from 1 and 0 to true and false and I forgot about this. Sorry. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:917: get viewport() { return this.viewport_; }, On 2016/04/21 02:24:00, bokan wrote: > Ditto on whitespace here and below Sorry .. ctrl + f screwed this up :) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:239: this.pinchCenter_ = { On 2016/04/21 15:27:04, bokan wrote: > On 2016/04/21 14:11:01, wjmaclean wrote: > > 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? > > 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. I've explained today to David how by having the point in the middle of the view helps us zooming. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:274: * Sets the cusotm zoom of the viewport. On 2016/04/21 02:24:01, bokan wrote: > "cusotm" -> "custom" Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:277: * @param {Object} center the pinch center On 2016/04/21 18:20:17, bokan wrote: > Describe the coordinates of the center value. i.e. "the pinch center in frame > coordinates" or "the pinch center in content coordinates" Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:322: x: (framePoint.x + this.position.x) / scale, On 2016/04/21 15:22:06, bokan wrote: > scale is always this.zoom_ so remove the argument and just use this.zoom_ here. Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:522: vectorDistance: function(p1, p2) { On 2016/04/21 14:11:01, wjmaclean wrote: > Maybe call the vectorDelta instead of invoking the notion of distance, since > this really isn't a distance? Done. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:575: this.doRender_ = 1; On 2016/04/21 02:24:01, bokan wrote: > Should doRender be true/false rather than a numeric value? True :) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:584: // We must change the new content in center because now we have fake On 2016/04/21 15:27:04, bokan wrote: > On 2016/04/21 14:11:01, wjmaclean wrote: > > I'm never comfortable with this term 'fake scrollbars'. We either have > > scrollbars or we don't, so please explain what you mean by fake :-) > > Unrelated to this, but fun anecdote anyway, did you know that the scrollbars in > the PDF viewer are actually caused by keeping an invisible DIV behind the plugin > and growing it as the plugin zooms so that it causes scrollbars? In that sense, > they really are fake :) That's pretty much my definition of fake .. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:585: // scrollbars On 2016/04/21 15:22:06, bokan wrote: > This comment should elaborate on what we're doing and why. Something along the > lines of: "If there's no horizontal scrolling, keep the content centered so the > user can't zoom in on the non-content area". The changeCenter_ variable is > likewise vague. It might be more descriptive as "keepContentCentered_" or > similar. Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:600: //By doing so we ensure the image_data_ in the plugin is the correct one On 2016/04/21 14:11:01, wjmaclean wrote: > On 2016/04/21 at 02:24:01, bokan wrote: > > Shouldn't doRender_ be = 1 then? Or I'm misunderstanding the comment... > > I would have thought that we are continuing to use the rendering (texture) from > before the pinch started, which (if everything is working correctly) should be > completely up to date. Is that correct? Yes. We are using the texture from before the pinch start and that means we do not want a render => this.doRender_ = false is ok :) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:600: //By doing so we ensure the image_data_ in the plugin is the correct one On 2016/04/21 15:27:04, bokan wrote: > On 2016/04/21 14:11:01, wjmaclean wrote: > > On 2016/04/21 at 02:24:01, bokan wrote: > > > Shouldn't doRender_ be = 1 then? Or I'm misunderstanding the comment... > > > > I would have thought that we are continuing to use the rendering (texture) > from > > before the pinch started, which (if everything is working correctly) should be > > completely up to date. Is that correct? > > If so, this comment is misleading. Old comment. Sorry! https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:601: this.doRender_ = 0; On 2016/04/21 18:20:17, bokan wrote: > This isn't inside a mightZoom though so we won't send the "viewport" message to > the plugin until we call pinchZoom but there we set doRender so this will get > clobbered. Either remove this line or maybe we need pinchZoomStart to also use > mightZoom_. True. I removed the doRender_ as we don't send the message to the plugin. If you remember by sending both pinchStart and pinch we will get the same event twice in the plugin and we don't want that :) https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:613: this.pinch_start_ = ev.center; On 2016/04/21 15:22:06, bokan wrote: > More descriptive name please. pinch_start_ -> first_pinch_center_in_frame_ Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:628: containerCenter: function(pageCenter) { On 2016/04/21 15:22:06, bokan wrote: > This function would be better described as "frameToPluginCoordinate". The > argument should just be called "coordinateInFrame". > > container.getBoundingClientRect().{left|top} will always be 0 today but I think > its good to keep this if that ever changes. Unless you meant this function to > convert into the "content" (i.e. non-grey gutter), in which case this is not > what that does. Acknowledged.
https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:68: const char kJSPy[] = "py"; On 2016/04/21 18:20:17, bokan wrote: > more descriptive name please Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:69: const char kJSPinchVectorX[] = "pinchVectorX"; On 2016/04/21 18:20:17, bokan wrote: > add a comment to say what pinch vector is (i.e. the amount of panning caused by > the pinch gesture) Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:282: last_current_scroll_(0,0), On 2016/04/21 18:20:17, bokan wrote: > nit: space after comma: (0,0) -> (0, 0) Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:410: // Pinch vector is the vector that computes fingers panning. On 2016/04/21 18:20:17, bokan wrote: > These kinds of comments belong where kJSPinchVector is declared. The comment > should also be more detailed. "Is the panning caused due to change in pinch > center between start and end of the gesture". Also, what coordinate space is it > in? (it's in frame coords here) Everywhere you have vectors/points you should be > sure to document either in comments or, even better, in the name, what the > coordinates are. Acknowledged. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:420: // If we have the grey parts we will use paint_offset to anchor the paint On 2016/04/21 14:11:02, wjmaclean wrote: > "have the grey parts" -> "the rendered document doesn't fill the display area" > (someday we might change the background colour to something other than grey) Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:439: paint_offset = pp::Point(0, (1 - zoom_delta) * pinch_center.y()); On 2016/04/21 18:20:17, bokan wrote: > I don't understand what we're doing here. Why are we centering vertically? We anchor the paint on the Oy axis and after that we apply the scroll delta. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:448: if(was_smaller_) { On 2016/04/21 14:11:02, wjmaclean wrote: > space after 'if' Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:946: if (!image_data_.is_null()) { On 2016/04/21 14:11:02, wjmaclean wrote: > You don't need the curly braces here. I assume this crept in during development, > but it should be restored to its original state. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1395: available_area_.Offset((available_area_.width() - doc_width) / 2 , 0); On 2016/04/21 14:11:01, wjmaclean wrote: > Undo this change. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1450: On 2016/04/21 14:11:01, wjmaclean wrote: > Undo. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.h (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:229: int do_render_; On 2016/04/21 14:11:02, wjmaclean wrote: > Make do_render_ bool. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.h:232: // True if last bitmap was smaller then screen. On 2016/04/21 14:11:02, wjmaclean wrote: > "then" -> "than" > > By "screen", we mean the PDF display area in the tab? Done. Yep, if the pdf document size is smaller then the page that means we had the grey parts and we must deal with the document little bit different. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc File pdf/paint_manager.cc (left): https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ol... pdf/paint_manager.cc:128: On 2016/04/21 14:11:02, wjmaclean wrote: > Undo white space changes. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ol... pdf/paint_manager.cc:166: On 2016/04/21 14:11:02, wjmaclean wrote: > Ditto. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc File pdf/paint_manager.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:214: if (pending_device_scale_ != device_scale_) On 2016/04/21 18:20:17, bokan wrote: > Is this related to the pinch zoom functionality or is this an unrelated fix? Reverted :) https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:216: On 2016/04/21 14:11:02, wjmaclean wrote: > Remove blank line. Done. https://codereview.chromium.org/1901903002/diff/30001/pdf/paint_manager.cc#ne... pdf/paint_manager.cc:223: } On 2016/04/21 14:11:02, wjmaclean wrote: > Undo this change ... Done. https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/js/h... File ui/webui/resources/js/hammer.js (right): https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/js/h... ui/webui/resources/js/hammer.js:5: On 2016/04/21 14:11:02, wjmaclean wrote: > Remove blank line. Done. https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/1901903002/diff/30001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:20: On 2016/04/21 14:11:02, wjmaclean wrote: > Remove blank line. Done.
Some issues from the original comments still remain. Why is the hammer LICENSE file blank? https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:439: paint_offset = pp::Point(0, (1 - zoom_delta) * pinch_center.y()); On 2016/04/22 14:33:05, alessandroa wrote: > On 2016/04/21 18:20:17, bokan wrote: > > I don't understand what we're doing here. Why are we centering vertically? > > We anchor the paint on the Oy axis and after that we apply the scroll delta. Acknowledged. https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:241: this.didPinchEnd_ = false; These should be on this.viewport_ since you bind your handlers to it, right? https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:302: this.prevScale *= scaleDelta; This should still be moved to pinchZoom as prevScale = ev.scale https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:344: get scrollbarWidth() { return this.scrollbarWidth_; }, Revert whitespace change. https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:574: if (ev.additionalEvent == 'pinchin') Add comment explaining why we render in one direction and not the other https://codereview.chromium.org/1901903002/diff/70001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/70001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:429: // Pinch start. Still need to change these to an enum as James suggested
https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:241: this.didPinchEnd_ = false; On 2016/04/22 15:45:35, bokan wrote: > These should be on this.viewport_ since you bind your handlers to it, right? That's true! Ups :) https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:302: this.prevScale *= scaleDelta; On 2016/04/22 15:45:35, bokan wrote: > This should still be moved to pinchZoom as prevScale = ev.scale Done. https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:344: get scrollbarWidth() { return this.scrollbarWidth_; }, On 2016/04/22 15:45:35, bokan wrote: > Revert whitespace change. Done. https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:574: if (ev.additionalEvent == 'pinchin') On 2016/04/22 15:45:35, bokan wrote: > Add comment explaining why we render in one direction and not the other Done.
https://codereview.chromium.org/1901903002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:253: window.requestAnimationFrame(function() { I don't think we need to rAF the PinchEnd event. https://codereview.chromium.org/1901903002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:743: render: doRender, // Render or not I think a better name here would be needsReraster https://codereview.chromium.org/1901903002/diff/80001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/80001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:430: if (!do_render && do_render_) { 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/1901903002/diff/80001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:466: // While pinching. I think the above if..else is part of "While Pinching" also.
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.
Issue moved here: https://codereview.chromium.org/2400743002/
Message was sent while issue was closed.
lgtm Hi, I have problem of pinch zoom with pdfjs. I am trying with your code. But unfortunately I couldn't find some method srefereed. 1) contentSizeChanged_() 2) viewportChangedCallback_() 3) documentNeedsScrollbars_() Please help me on this. because i am at deadend
Message was sent while issue was closed.
lgtm lgtm Hi, I have problem of pinch zoom with pdfjs. I am trying with your code. But unfortunately I couldn't find some method srefereed. 1) contentSizeChanged_() 2) viewportChangedCallback_() 3) documentNeedsScrollbars_() Please help me on this. because i am at deadend
Message was sent while issue was closed.
lgtm lgtm lgtm Hi, I have problem of pinch zoom with pdfjs. I am trying with your code. But unfortunately I couldn't find some method srefereed. 1) contentSizeChanged_() 2) viewportChangedCallback_() 3) documentNeedsScrollbars_() Please help me on this. because i am at deadend
Message was sent while issue was closed.
Hi, I have problem of pinch zoom with pdfjs. I am trying with your code. But unfortunately I couldn't find some method srefereed. 1) contentSizeChanged_() 2) viewportChangedCallback_() 3) documentNeedsScrollbars_() Please help me on this. because I am at deadend. Hope you all help me on this.
Message was sent while issue was closed.
On 2016/11/23 06:33:30, nishila2 wrote: > Hi, > > I have problem of pinch zoom with pdfjs. I am trying with your code. But > unfortunately I couldn't find some method srefereed. > > 1) contentSizeChanged_() > 2) viewportChangedCallback_() > 3) documentNeedsScrollbars_() > > Please help me on this. because I am at deadend. > > Hope you all help me on this. Hi, this code has landed in trunk in https://codereview.chromium.org/2400743002/ so there's no need to apply this patch. In general, you may have more luck searching the code using CodeSearch: https://cs.chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
