Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541903002:
PDF pinch zoom: Update layer transform for a zoom out after a zoom in. (Closed)
|
Created:
4 years ago by Kevin McNee Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPDF pinch zoom: Update layer transform for a zoom out after a zoom in.
Currently, whenever we pinch zoom out, we clear any layer transform and
reraster at the new zoom level. However, during a pinch zoom in (and
especially when slowly panning), we may get occasional zoom out events
which cause us to clear the layer transform. As the zoom in continues
we will then set the layer transform again. This repeated setting and
clearing causes noticeable jumping back and forth of the PDF content.
In this patch, when we get a pinch zoom out, if a layer transform is
already set due to a previous zoom in, we update the layer transform
instead of clearing it.
BUG=664157
Committed: https://crrev.com/a6e4fc5905ae3e09f54d0c7a2e0fb9d89badb435
Cr-Commit-Position: refs/heads/master@{#436640}
Patch Set 1 #
Messages
Total messages: 22 (10 generated)
mcnee@chromium.org changed reviewers: + thestig@chromium.org
Hello, thestig@. Here's a change to fix the jittery panning when pinch zooming PDFs.
The CQ bit was checked by mcnee@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...
mcnee@chromium.org changed reviewers: + raymes@chromium.org
Oops.. I see thestig is on leave. raymes@: Could you take a look at this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raymes@chromium.org changed reviewers: + bokan@chromium.org
I don't know too much about pinch zoom. Would it make sense for bokan@ to take a look first? Happy to rubberstamp afterward.
Looks reasonable to me. Is there a way we can test this behavior?
On 2016/12/01 16:26:12, bokan wrote: > Looks reasonable to me. Is there a way we can test this behavior? Chatted with Kevin off-review, the C++ side of the PDF plugin doesn't have good test infrastructure so lgtm as is.
On 2016/12/01 16:55:13, bokan wrote: > On 2016/12/01 16:26:12, bokan wrote: > > Looks reasonable to me. Is there a way we can test this behavior? > > Chatted with Kevin off-review, the C++ side of the PDF plugin doesn't have good > test infrastructure so lgtm as is. What would be needed to write a test for this? I do have a browsertest in pdf_extension_test.cc.
On 2016/12/05 01:49:43, raymes wrote: > On 2016/12/01 16:55:13, bokan wrote: > > On 2016/12/01 16:26:12, bokan wrote: > > > Looks reasonable to me. Is there a way we can test this behavior? > > > > Chatted with Kevin off-review, the C++ side of the PDF plugin doesn't have > good > > test infrastructure so lgtm as is. > > What would be needed to write a test for this? I do have a browsertest in > pdf_extension_test.cc. *We* do have a browsertest in pdf_extension_test.cc.
On 2016/12/05 01:50:07, raymes wrote: > On 2016/12/05 01:49:43, raymes wrote: > > On 2016/12/01 16:55:13, bokan wrote: > > > On 2016/12/01 16:26:12, bokan wrote: > > > > Looks reasonable to me. Is there a way we can test this behavior? > > > > > > Chatted with Kevin off-review, the C++ side of the PDF plugin doesn't have > > good > > > test infrastructure so lgtm as is. > > > > What would be needed to write a test for this? I do have a browsertest in > > pdf_extension_test.cc. > > *We* do have a browsertest in pdf_extension_test.cc. Right, so I previously added a js test file there, but I don't see anything that tests the behaviour of pdf/out_of_process_instance.cc or pdf/paint_manager.cc. As an example of a test, if we could mock out |paint_manager_| and |engine_| from OutOfProcessInstance, we could feed it a sequence of viewport messages and verify it called the correct zoom/transform functions as a result.
Ah I see. It would be great to make those things testable but I won't hold up this CL on that. lgtm
The CQ bit was checked by mcnee@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481043551044540, "parent_rev":
"2a560d7b41f5e1651c888e74e768e10677761f53", "commit_rev":
"810c2dafdee8dfb6d49fc917c50a74d4757f5fdb"}
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== PDF pinch zoom: Update layer transform for a zoom out after a zoom in. Currently, whenever we pinch zoom out, we clear any layer transform and reraster at the new zoom level. However, during a pinch zoom in (and especially when slowly panning), we may get occasional zoom out events which cause us to clear the layer transform. As the zoom in continues we will then set the layer transform again. This repeated setting and clearing causes noticeable jumping back and forth of the PDF content. In this patch, when we get a pinch zoom out, if a layer transform is already set due to a previous zoom in, we update the layer transform instead of clearing it. BUG=664157 ========== to ========== PDF pinch zoom: Update layer transform for a zoom out after a zoom in. Currently, whenever we pinch zoom out, we clear any layer transform and reraster at the new zoom level. However, during a pinch zoom in (and especially when slowly panning), we may get occasional zoom out events which cause us to clear the layer transform. As the zoom in continues we will then set the layer transform again. This repeated setting and clearing causes noticeable jumping back and forth of the PDF content. In this patch, when we get a pinch zoom out, if a layer transform is already set due to a previous zoom in, we update the layer transform instead of clearing it. BUG=664157 Committed: https://crrev.com/a6e4fc5905ae3e09f54d0c7a2e0fb9d89badb435 Cr-Commit-Position: refs/heads/master@{#436640} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a6e4fc5905ae3e09f54d0c7a2e0fb9d89badb435 Cr-Commit-Position: refs/heads/master@{#436640} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
