Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1673703002:
support zoom and move image by mouse. (Closed)
|
Created:
4 years, 10 months ago by ryoh Modified:
4 years, 10 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsupport zoom and move image by mouse.
BUG=510614
TEST=manually
Committed: https://crrev.com/b2f88eeb0310e882707d873a973c5d35f4dbfdae
Cr-Commit-Position: refs/heads/master@{#374829}
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove dups and fix for closure compiler. #
Total comments: 10
Patch Set 3 : fix nits #
Total comments: 4
Patch Set 4 : check mousebutton also in onMouseUp, stopOperation before moving #
Total comments: 1
Patch Set 5 : nits #Messages
Total messages: 25 (11 generated)
Description was changed from ========== support zoom and move image by mouse. BUG=510614 ========== to ========== support zoom and move image by mouse. BUG=510614 TEST=manually ==========
ryoh@chromium.org changed reviewers: + hirono@chromium.org
Hi, here is the patch to zoom/move image by mouse. Could you take a look?
https://codereview.chromium.org/1673703002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:2084: TouchHandler.prototype.onMouseWheel_ = function(event) { nit: It looks we have two onMouseWheel_.
Thanks you for the review. I removed the duplicated code and added type annotations for closure compiler. Please take another look! https://codereview.chromium.org/1673703002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:2084: TouchHandler.prototype.onMouseWheel_ = function(event) { On 2016/02/06 07:27:59, hirono wrote: > nit: It looks we have two onMouseWheel_. Done.
https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2094: if (!this.enabled_) Maybe we should check button property of event. https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2097: } nit: add ; https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2111: (/** @type {{movementX: number}} */(event)).movementX, For this CL, it's fine. But maybe we can extend the definition of MouseEvent for closure compiler. Could you ask @fukino-san for the details? https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2115: console.log(event); nit: Please remove debug log. https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2116: } ditto https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2125: } ditto
lgtm after fixing the comments.
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org Link to the patchset: https://codereview.chromium.org/1673703002/#ps40001 (title: "fix nits")
Thanks you for the reply. I will remove the type annotation at #2111, #2112 after adding some extra type declarations for closure compiler. https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2094: if (!this.enabled_) On 2016/02/09 17:57:44, hirono wrote: > Maybe we should check button property of event. Done. https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2097: } On 2016/02/09 17:57:44, hirono wrote: > nit: add ; Done. https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2111: (/** @type {{movementX: number}} */(event)).movementX, On 2016/02/09 17:57:44, hirono wrote: > For this CL, it's fine. > But maybe we can extend the definition of MouseEvent for closure compiler. > Could you ask @fukino-san for the details? The similar probrem occured at set-wallpaper pactch: https://codereview.chromium.org/1679983003/ I will add extra declarations also for MouseEvent. Thanks. https://codereview.chromium.org/1673703002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2115: console.log(event); On 2016/02/09 17:57:44, hirono wrote: > nit: Please remove debug log. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673703002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673703002/40001
Sorry I found two more comments. Still lgtm after resolving the issues. Thanks! https://codereview.chromium.org/1673703002/diff/40001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2109: viewport.setOffset( Please call this.stopOperation() before setting offset as well as line #2074. https://codereview.chromium.org/1673703002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2123: this.clickStarted_ = false; I think you also need to check event.button here. 1. Right down 2. Left down 3. Right up, which should not stop left dragging.
The CQ bit was unchecked by hirono@chromium.org
Thank you for your review! Could you take a look? https://codereview.chromium.org/1673703002/diff/40001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2109: viewport.setOffset( On 2016/02/10 02:06:53, hirono wrote: > Please call this.stopOperation() before setting offset as well as line #2074. Done. https://codereview.chromium.org/1673703002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2123: this.clickStarted_ = false; On 2016/02/10 02:06:53, hirono wrote: > I think you also need to check event.button here. > > 1. Right down > 2. Left down > 3. Right up, which should not stop left dragging. That's right!
lgtm! https://codereview.chromium.org/1673703002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1673703002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:2078: zoom = zoom * TouchHandler.WHEEL_ZOOM_FACTOR; nit: *= and /= ?
The CQ bit was checked by ryoh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673703002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673703002/60001
The CQ bit was unchecked by ryoh@chromium.org
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org Link to the patchset: https://codereview.chromium.org/1673703002/#ps80001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673703002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673703002/80001
Message was sent while issue was closed.
Description was changed from ========== support zoom and move image by mouse. BUG=510614 TEST=manually ========== to ========== support zoom and move image by mouse. BUG=510614 TEST=manually ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== support zoom and move image by mouse. BUG=510614 TEST=manually ========== to ========== support zoom and move image by mouse. BUG=510614 TEST=manually Committed: https://crrev.com/b2f88eeb0310e882707d873a973c5d35f4dbfdae Cr-Commit-Position: refs/heads/master@{#374829} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b2f88eeb0310e882707d873a973c5d35f4dbfdae Cr-Commit-Position: refs/heads/master@{#374829} |
