Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1865543003:
Update input_injector_win.cc to support injecting wheel-x and wheel-y events together (Closed)
DescriptionUpdate input_injector_win.cc to support injecting wheel-x and wheel-y events together
BUG=598434
Committed: https://crrev.com/0d2570c7d26fd026fa7f720aef5b0d4d049c643d
Cr-Commit-Position: refs/heads/master@{#385594}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Resolve CR comments #Patch Set 3 : Restore original position of using statements #Patch Set 4 : Revert last change #Messages
Total messages: 15 (6 generated)
Description was changed from ========== Update input_injector_win.cc to support injecting wheel-x and wheel-y events together BUG=598434 ========== to ========== Update input_injector_win.cc to support injecting wheel-x and wheel-y events together BUG=598434 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:34: using std::vector; Please just refer to std::vector<> everywhere below instead of adding this using statement. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:66: DCHECK(output != nullptr); This should be DCHECK(output), but I don't think this DCHECK is useful. Remove it? https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:67: INPUT input{}; C++11 initialization syntax is currently banned, see http://chromium-cpp.appspot.com/ This can be "input = {0}" https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:89: output->push_back(std::move(input)); INPUT is a just a struct. I don't think you need std::move() here. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:139: // > 0 will cause this INPUT to be invalid. This comment is confusing. Maybe just say that both flags need to be set for horizontal wheel? Does this mean that horizontal wheel movement were broken before this change?
Resolve CR comments https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:34: using std::vector; On 2016/04/06 18:25:31, Sergey Ulanov wrote: > Please just refer to std::vector<> everywhere below instead of adding this using > statement. Yes, I can do it, but why do we prefer removing this using statement? https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:66: DCHECK(output != nullptr); On 2016/04/06 18:25:31, Sergey Ulanov wrote: > This should be DCHECK(output), but I don't think this DCHECK is useful. Remove > it? Similar as above, why do we prefer to use, say, 'if (a_pointer)' instead of 'if (a_pointer != nullptr)'? https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:67: INPUT input{}; On 2016/04/06 18:25:31, Sergey Ulanov wrote: > C++11 initialization syntax is currently banned, see > http://chromium-cpp.appspot.com/ > This can be "input = {0}" Done. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:89: output->push_back(std::move(input)); On 2016/04/06 18:25:31, Sergey Ulanov wrote: > INPUT is a just a struct. I don't think you need std::move() here. Since it's a trivial type, a move constructor is automatically added by compiler. So an std::move can eventually trigger the optimized move constructor. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:139: // > 0 will cause this INPUT to be invalid. On 2016/04/06 18:25:31, Sergey Ulanov wrote: > This comment is confusing. Maybe just say that both flags need to be set for > horizontal wheel? > Does this mean that horizontal wheel movement were broken before this change? Updated. The original implementation has two issues, 1. Even with only horizontal wheel movement, we need to set both MOUSEEVENTF_WHEEL and MOUSEEVENTF_HWHELL for dwFlags. 2. If one MouseEvent contains both horizontal wheel movement and vertical wheel movement, vertical one will be ignored, but the delta value is still coming from vertical one. Well, it was totally wrong.
https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:34: using std::vector; On 2016/04/06 19:56:34, Hzj_jie wrote: > On 2016/04/06 18:25:31, Sergey Ulanov wrote: > > Please just refer to std::vector<> everywhere below instead of adding this > using > > statement. > > Yes, I can do it, but why do we prefer removing this using statement? We don't use using statement for STL types anywhere else. In general using statements have some costs, they are not free. They make the code harder to read (one have to scroll to find that using statement). The code is also harder to move, e.g. to a file that doesn't have the same using statement. In this particular case std::vector is used in 4 places in this file, so you don't save much by having this statement. Ideally I would prefer removing the other using statements above for protocol:: types as well, but I don't mind keeping them as they were there before. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:66: DCHECK(output != nullptr); On 2016/04/06 19:56:34, Hzj_jie wrote: > On 2016/04/06 18:25:31, Sergey Ulanov wrote: > > This should be DCHECK(output), but I don't think this DCHECK is useful. Remove > > it? > > Similar as above, why do we prefer to use, say, > 'if (a_pointer)' instead of 'if (a_pointer != nullptr)'? Mainly for consistency with all other code. In this case it makes the code shorter and just as readable, and there are no downsides. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:89: output->push_back(std::move(input)); On 2016/04/06 19:56:34, Hzj_jie wrote: > On 2016/04/06 18:25:31, Sergey Ulanov wrote: > > INPUT is a just a struct. I don't think you need std::move() here. > > Since it's a trivial type, a move constructor is automatically added by > compiler. So an std::move can eventually trigger the optimized move constructor. Interesting. How can it be optimized? push_back() is normally inlined, so I think the compiler will generate code that just copies data to the new location and there is not much room to optimize it. Anyway, I think it's ok to keep std::move() here. The style guide says this: "You may use std::move to express moving a value from one object to another rather than copying it."
lgtm
Resolve CR comments. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:34: using std::vector; On 2016/04/06 20:42:20, Sergey Ulanov wrote: > On 2016/04/06 19:56:34, Hzj_jie wrote: > > On 2016/04/06 18:25:31, Sergey Ulanov wrote: > > > Please just refer to std::vector<> everywhere below instead of adding this > > using > > > statement. > > > > Yes, I can do it, but why do we prefer removing this using statement? > > We don't use using statement for STL types anywhere else. In general using > statements have some costs, they are not free. They make the code harder to read > (one have to scroll to find that using statement). The code is also harder to > move, e.g. to a file that doesn't have the same using statement. In this > particular case std::vector is used in 4 places in this file, so you don't save > much by having this statement. > Ideally I would prefer removing the other using statements above for protocol:: > types as well, but I don't mind keeping them as they were there before. Done. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:66: DCHECK(output != nullptr); On 2016/04/06 20:42:20, Sergey Ulanov wrote: > On 2016/04/06 19:56:34, Hzj_jie wrote: > > On 2016/04/06 18:25:31, Sergey Ulanov wrote: > > > This should be DCHECK(output), but I don't think this DCHECK is useful. > Remove > > > it? > > > > Similar as above, why do we prefer to use, say, > > 'if (a_pointer)' instead of 'if (a_pointer != nullptr)'? > > Mainly for consistency with all other code. In this case it makes the code > shorter and just as readable, and there are no downsides. Done. https://codereview.chromium.org/1865543003/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:89: output->push_back(std::move(input)); On 2016/04/06 20:42:20, Sergey Ulanov wrote: > On 2016/04/06 19:56:34, Hzj_jie wrote: > > On 2016/04/06 18:25:31, Sergey Ulanov wrote: > > > INPUT is a just a struct. I don't think you need std::move() here. > > > > Since it's a trivial type, a move constructor is automatically added by > > compiler. So an std::move can eventually trigger the optimized move > constructor. > > Interesting. How can it be optimized? push_back() is normally inlined, so I > think the compiler will generate code that just copies data to the new location > and there is not much room to optimize it. > Anyway, I think it's ok to keep std::move() here. The style guide says this: > "You may use std::move to express moving a value from one object to another > rather than copying it." > Sorry, I did not express my opinion clear. I do not focus on this specific case, but just a very common opinion. To any types we do not want to pass via value in function parameters, using std::move is not a bad idea.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1865543003/#ps60001 (title: "Revert last change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1865543003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865543003/60001
Message was sent while issue was closed.
Description was changed from ========== Update input_injector_win.cc to support injecting wheel-x and wheel-y events together BUG=598434 ========== to ========== Update input_injector_win.cc to support injecting wheel-x and wheel-y events together BUG=598434 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update input_injector_win.cc to support injecting wheel-x and wheel-y events together BUG=598434 ========== to ========== Update input_injector_win.cc to support injecting wheel-x and wheel-y events together BUG=598434 Committed: https://crrev.com/0d2570c7d26fd026fa7f720aef5b0d4d049c643d Cr-Commit-Position: refs/heads/master@{#385594} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0d2570c7d26fd026fa7f720aef5b0d4d049c643d Cr-Commit-Position: refs/heads/master@{#385594} |
