Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1554253004:
Made page zoom handling work with smooth scroll devices (Closed)
|
Created:
4 years, 11 months ago by Will Shackleton Modified:
4 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMade page zoom handling work with smooth scroll devices
This CL is a smaller set of the CL https://codereview.chromium.org/688253002/
It fixes some broken functionality exposed by that CL.
R=avi
BUG=384970
Committed: https://crrev.com/49bcd394cf41b1c330c3ffbcae42ff5161352380
Cr-Commit-Position: refs/heads/master@{#367850}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Switched to lround, renamed to zoom_scroll_remainder_ #Patch Set 3 : Switched to std::lround #
Messages
Total messages: 20 (8 generated)
Description was changed from ========== Made page zoom handling work with smooth scroll devices This CL is a smaller set of the CL https://codereview.chromium.org/688253002/ It fixes some broken functionality exposed by that CL. R=avi BUG=384970 ========== to ========== Made page zoom handling work with smooth scroll devices This CL is a smaller set of the CL https://codereview.chromium.org/688253002/ It fixes some broken functionality exposed by that CL. R=avi BUG=384970 ==========
w.shackleton@gmail.com changed reviewers: + sadrul@chromium.org
Nits. https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1594: int whole_zoom_scroll_amount = round(zoom_scroll_amount_); Let's make it C++, so do std::lround, since we're returning to an integer value. Also, include <cmath> as that's where it's defined. https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.h:1184: float zoom_scroll_amount_; This holds the non-integer portion of the zoom. Maybe |zoom_scroll_remainder_|? Or something else that indicates that it's a portion of the zoom scroll amount, not the whole thing.
I've made those changes. https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1594: int whole_zoom_scroll_amount = round(zoom_scroll_amount_); On 2016/01/05 01:46:24, Avi wrote: > Let's make it C++, so do std::lround, since we're returning to an integer value. > > Also, include <cmath> as that's where it's defined. Done. https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1554253004/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.h:1184: float zoom_scroll_amount_; On 2016/01/05 01:46:24, Avi wrote: > This holds the non-integer portion of the zoom. Maybe |zoom_scroll_remainder_|? > Or something else that indicates that it's a portion of the zoom scroll amount, > not the whole thing. Done.
lgtm Looks good!
The CQ bit was checked by w.shackleton@gmail.com
The CQ bit was unchecked by w.shackleton@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554253004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554253004/40001
The CQ bit was unchecked by w.shackleton@gmail.com
On 2016/01/06 16:27:59, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1554253004/40001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1554253004/40001 Oh dear what did I just do? I ticked the commit checkbox - is that what I'm supposed to do?
On 2016/01/06 16:29:36, Will Shackleton wrote: > On 2016/01/06 16:27:59, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1554253004/40001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1554253004/40001 > > Oh dear what did I just do? I ticked the commit checkbox - is that what I'm > supposed to do? Yeah, you're good! I approved the change, so you clicked the checkbox, which isn't really a checkbox but an instruction to the Commit Queue to do the commit. It will do all the work of testing the patch and landing it. The UI of a checkbox is not great, and rather disconcerting when you first use it, but that's how it is.
On 2016/01/06 16:36:41, Avi wrote: > On 2016/01/06 16:29:36, Will Shackleton wrote: > > On 2016/01/06 16:27:59, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/patch-status/1554253004/40001 > > > View timeline at > > > https://chromium-cq-status.appspot.com/patch-timeline/1554253004/40001 > > > > Oh dear what did I just do? I ticked the commit checkbox - is that what I'm > > supposed to do? > > Yeah, you're good! > > I approved the change, so you clicked the checkbox, which isn't really a > checkbox but an instruction to the Commit Queue to do the commit. It will do all > the work of testing the patch and landing it. > > The UI of a checkbox is not great, and rather disconcerting when you first use > it, but that's how it is. Ah that makes more sense. Thanks!
On 2016/01/06 16:37:36, Will Shackleton wrote: > On 2016/01/06 16:36:41, Avi wrote: > > On 2016/01/06 16:29:36, Will Shackleton wrote: > > > On 2016/01/06 16:27:59, commit-bot: I haz the power wrote: > > > > CQ is trying da patch. Follow status at > > > > https://chromium-cq-status.appspot.com/patch-status/1554253004/40001 > > > > View timeline at > > > > https://chromium-cq-status.appspot.com/patch-timeline/1554253004/40001 > > > > > > Oh dear what did I just do? I ticked the commit checkbox - is that what I'm > > > supposed to do? > > > > Yeah, you're good! > > > > I approved the change, so you clicked the checkbox, which isn't really a > > checkbox but an instruction to the Commit Queue to do the commit. It will do > all > > the work of testing the patch and landing it. > > > > The UI of a checkbox is not great, and rather disconcerting when you first use > > it, but that's how it is. > > Ah that makes more sense. Thanks! Did you uncheck the box? It looks like you did about forty seconds into the CQ. Check the box again and let it stay checked.
The CQ bit was checked by w.shackleton@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554253004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554253004/40001
Message was sent while issue was closed.
Description was changed from ========== Made page zoom handling work with smooth scroll devices This CL is a smaller set of the CL https://codereview.chromium.org/688253002/ It fixes some broken functionality exposed by that CL. R=avi BUG=384970 ========== to ========== Made page zoom handling work with smooth scroll devices This CL is a smaller set of the CL https://codereview.chromium.org/688253002/ It fixes some broken functionality exposed by that CL. R=avi BUG=384970 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Made page zoom handling work with smooth scroll devices This CL is a smaller set of the CL https://codereview.chromium.org/688253002/ It fixes some broken functionality exposed by that CL. R=avi BUG=384970 ========== to ========== Made page zoom handling work with smooth scroll devices This CL is a smaller set of the CL https://codereview.chromium.org/688253002/ It fixes some broken functionality exposed by that CL. R=avi BUG=384970 Committed: https://crrev.com/49bcd394cf41b1c330c3ffbcae42ff5161352380 Cr-Commit-Position: refs/heads/master@{#367850} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/49bcd394cf41b1c330c3ffbcae42ff5161352380 Cr-Commit-Position: refs/heads/master@{#367850} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
