Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(339)

Issue 1901903002: Improved Pinch-Zoom for PDF (Closed)

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.

Description

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 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+2926 lines, -33 lines) Patch
M chrome/browser/resources/pdf/index.html View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/pdf/pdf.js View 1 2 3 4 5 2 chunks +54 lines, -1 line 2 comments Download
M chrome/browser/resources/pdf/viewport.js View 1 2 3 4 5 8 chunks +151 lines, -15 lines 0 comments Download
M pdf/out_of_process_instance.h View 1 2 3 4 1 chunk +11 lines, -3 lines 0 comments Download
M pdf/out_of_process_instance.cc View 1 2 3 4 5 chunks +86 lines, -4 lines 2 comments Download
M pdf/paint_manager.h View 1 2 2 chunks +8 lines, -0 lines 0 comments Download
M pdf/paint_manager.cc View 1 2 3 4 5 chunks +31 lines, -6 lines 0 comments Download
A + third_party/hammer/LICENSE View 1 2 3 4 5 1 chunk +4 lines, -3 lines 0 comments Download
A + third_party/hammer/OWNERS View 1 chunk +1 line, -1 line 0 comments Download
A third_party/hammer/README.chromium View 1 chunk +6 lines, -0 lines 0 comments Download
A third_party/hammer/hammer.js View 1 chunk +2568 lines, -0 lines 0 comments Download
A ui/webui/resources/js/hammer.js View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M ui/webui/resources/webui_resources.grd View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 25 (6 generated)
alessandroa
4 years, 8 months ago (2016-04-21 00:05:41 UTC) #4
bokan
I'm about half way through but need to go to bed, figured I'd release my ...
4 years, 8 months ago (2016-04-21 02:24:01 UTC) #8
wjmaclean
A few comments. https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/pdf.js File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/pdf.js#newcode723 chrome/browser/resources/pdf/pdf.js:723: doRender = false; I don't understand ...
4 years, 8 months ago (2016-04-21 14:11:02 UTC) #9
bokan
Some comments on viewport.js and pdf.js https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/pdf.js File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/pdf.js#newcode722 chrome/browser/resources/pdf/pdf.js:722: if (!this.viewport_.doRender_) This ...
4 years, 8 months ago (2016-04-21 15:22:06 UTC) #10
bokan
https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/viewport.js File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/viewport.js#newcode239 chrome/browser/resources/pdf/viewport.js:239: this.pinchCenter_ = { On 2016/04/21 14:11:01, wjmaclean wrote: > ...
4 years, 8 months ago (2016-04-21 15:27:04 UTC) #11
bokan
https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/viewport.js File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/viewport.js#newcode323 chrome/browser/resources/pdf/viewport.js:323: y: (framePoint.y + this.position.y) / scale On 2016/04/21 15:22:06, ...
4 years, 8 months ago (2016-04-21 15:59:02 UTC) #12
bokan
https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/viewport.js File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/viewport.js#newcode277 chrome/browser/resources/pdf/viewport.js:277: * @param {Object} center the pinch center Describe the ...
4 years, 8 months ago (2016-04-21 18:20:17 UTC) #13
alessandroa
I fixed part of them :) still wip https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/index.html File chrome/browser/resources/pdf/index.html (right): https://codereview.chromium.org/1901903002/diff/30001/chrome/browser/resources/pdf/index.html#newcode47 chrome/browser/resources/pdf/index.html:47: On ...
4 years, 8 months ago (2016-04-21 20:39:40 UTC) #14
alessandroa
https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/1901903002/diff/30001/pdf/out_of_process_instance.cc#newcode68 pdf/out_of_process_instance.cc:68: const char kJSPy[] = "py"; On 2016/04/21 18:20:17, bokan ...
4 years, 8 months ago (2016-04-22 14:33:06 UTC) #15
bokan
Some issues from the original comments still remain. Why is the hammer LICENSE file blank? ...
4 years, 8 months ago (2016-04-22 15:45:36 UTC) #16
alessandroa
https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resources/pdf/pdf.js File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/70001/chrome/browser/resources/pdf/pdf.js#newcode241 chrome/browser/resources/pdf/pdf.js:241: this.didPinchEnd_ = false; On 2016/04/22 15:45:35, bokan wrote: > ...
4 years, 8 months ago (2016-04-22 18:59:03 UTC) #17
bokan
https://codereview.chromium.org/1901903002/diff/80001/chrome/browser/resources/pdf/pdf.js File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/1901903002/diff/80001/chrome/browser/resources/pdf/pdf.js#newcode253 chrome/browser/resources/pdf/pdf.js:253: window.requestAnimationFrame(function() { I don't think we need to rAF ...
4 years, 7 months ago (2016-05-12 14:57:58 UTC) #18
bokan
And the issues I still saw when trying this patch: -Zoom into the PDF. Once ...
4 years, 7 months ago (2016-05-12 15:00:50 UTC) #19
wjmaclean
Issue moved here: https://codereview.chromium.org/2400743002/
4 years, 2 months ago (2016-10-07 18:59:04 UTC) #20
nishila2
lgtm Hi, I have problem of pinch zoom with pdfjs. I am trying with your ...
4 years, 1 month ago (2016-11-23 06:33:00 UTC) #21
nishila2
lgtm lgtm Hi, I have problem of pinch zoom with pdfjs. I am trying with ...
4 years, 1 month ago (2016-11-23 06:33:01 UTC) #22
nishila2
lgtm lgtm lgtm Hi, I have problem of pinch zoom with pdfjs. I am trying ...
4 years, 1 month ago (2016-11-23 06:33:02 UTC) #23
nishila2
Hi, I have problem of pinch zoom with pdfjs. I am trying with your code. ...
4 years, 1 month ago (2016-11-23 06:33:30 UTC) #24
bokan
4 years ago (2016-11-23 13:16:49 UTC) #25
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.

Powered by Google App Engine
This is Rietveld 408576698