Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2077163003:
Reduced number of UpdateDeviceList calls (Closed)
|
Created:
4 years, 6 months ago by Will Shackleton Modified:
4 years, 4 months ago CC:
chromium-reviews, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReduced number of UpdateDeviceList calls
I stopped UpdateDeviceList being called every time XISlaveSwitch events occur.
BUG=609748
Committed: https://crrev.com/e620fd41c0c6acb7ffb3c44710553797b3c9deb9
Cr-Commit-Position: refs/heads/master@{#407597}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Applied comments, added functionality to only invalidate current scroll device #
Total comments: 4
Patch Set 3 : Removed default parameter value, changed if-return to CHECK #
Total comments: 4
Patch Set 4 : Changed DCHECK_EQ to else if, added ALL_DEVICES enum #
Total comments: 2
Patch Set 5 : Changed ALL_DEVICES to kAllDevices #
Messages
Total messages: 28 (6 generated)
Description was changed from ========== Reduced number of UpdateDeviceList calls I stopped UpdateDeviceList being called every time XISlaveSwitch events occur. BUG=609748 ========== to ========== Reduced number of UpdateDeviceList calls I stopped UpdateDeviceList being called every time XISlaveSwitch events occur. BUG=609748 ==========
w.shackleton@gmail.com changed reviewers: + bokan@chromium.org, sadrul@chromium.org
https://codereview.chromium.org/2077163003/diff/1/ui/events/platform/x11/x11_... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/1/ui/events/platform/x11/x11_... ui/events/platform/x11/x11_event_source.cc:228: How about: bool should_update_list = false; if (xevent->type == GenericEvent) { if (xevent->xgeneric.evtype == XI_HierarchyChanged) { should_update_list = true; } else if (XI_DeviceChanged) { if (XIDeviceChange) { should_update_list = true; } else { DCHECK_EQ(XISlaveSwitch, xev->reason); InvalidateScrollClasses(); } } } if (should_update_list) { UpdateDeviceList(); hotplug... }
Done that. I've also changed it to only invalidate the current device, as per https://github.com/GNOME/gtk/blob/master/gdk/x11/gdkdevicemanager-xi2.c#L974 We still need to confirm that this CL actually fixes the issue as well.
Done that. I've also changed it to only invalidate the current device, as per https://github.com/GNOME/gtk/blob/master/gdk/x11/gdkdevicemanager-xi2.c#L974 We still need to confirm that this CL actually fixes the issue as well. (I'll work out the subtleties of Rietveld one day!) https://codereview.chromium.org/2077163003/diff/1/ui/events/platform/x11/x11_... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/1/ui/events/platform/x11/x11_... ui/events/platform/x11/x11_event_source.cc:228: On 2016/06/20 19:37:30, sadrul wrote: > How about: > > bool should_update_list = false; > if (xevent->type == GenericEvent) { > if (xevent->xgeneric.evtype == XI_HierarchyChanged) { > should_update_list = true; > } else if (XI_DeviceChanged) { > if (XIDeviceChange) { > should_update_list = true; > } else { > DCHECK_EQ(XISlaveSwitch, xev->reason); > InvalidateScrollClasses(); > } > } > } > if (should_update_list) { > UpdateDeviceList(); > hotplug... > } Done.
I tried the patch. It seems to work nicely with multiple devices and the "first tick of scrolling" seems to apply ok. The last check should be taviso@'s broken mouse in issue 593453. I'll ask him to try this patch. (or wait until it hits dev channel).
Can we land this and give it a try in dev channel?
https://codereview.chromium.org/2077163003/diff/20001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/2077163003/diff/20001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:205: void InvalidateScrollClasses(int device_id = -1); Remove the default param-value. Document that using -1 as the device-id would invalidate all devices. https://codereview.chromium.org/2077163003/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:224: xev->sourceid); Can |xev->sourceid| here be really -1?
Will, we're unfortunately out of runway for M52. Is there a small patch we could land that would disable xinput2 in M52 until we can get the remaining edge cases fixed?
On 2016/06/29 18:02:03, bokan wrote: > Will, we're unfortunately out of runway for M52. Is there a small patch we could > land that would disable xinput2 in M52 until we can get the remaining edge cases > fixed? That's probably sensible, I'll submit something quickly now. Sorry, I just graduated last week and I'm off on holiday tomorrow. Should I make it a chrome://flags flag?
On 2016/06/29 18:10:08, Will Shackleton wrote: > On 2016/06/29 18:02:03, bokan wrote: > > Will, we're unfortunately out of runway for M52. Is there a small patch we > could > > land that would disable xinput2 in M52 until we can get the remaining edge > cases > > fixed? > > That's probably sensible, I'll submit something quickly now. Sorry, I just > graduated last week and I'm off on holiday tomorrow. Should I make it a > chrome://flags flag? Thanks, I think just turning it off should be fine - should be for just one release and since we're merging it pretty late, smaller is better. (PS Congrats on graduating :)
What's the recommended way in chromium to disable some code? #if 0 and a comment explaining why? On Wed, 29 Jun 2016 at 19:11 <bokan@chromium.org> wrote: > On 2016/06/29 18:10:08, Will Shackleton wrote: > > On 2016/06/29 18:02:03, bokan wrote: > > > Will, we're unfortunately out of runway for M52. Is there a small > patch we > > could > > > land that would disable xinput2 in M52 until we can get the remaining > edge > > cases > > > fixed? > > > > That's probably sensible, I'll submit something quickly now. Sorry, I > just > > graduated last week and I'm off on holiday tomorrow. Should I make it a > > chrome://flags flag? > > Thanks, I think just turning it off should be fine - should be for just one > release and since we're merging it pretty late, smaller is better. > > (PS Congrats on graduating :) > > https://codereview.chromium.org/2077163003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/29 18:13:59, Will Shackleton wrote: > What's the recommended way in chromium to disable some code? #if 0 and a > comment explaining why? #if'ing out code is discouraged. Easiest way is probably just adding an early return in UpdateScrollClassDevice like you did for legacy devices in https://codereview.chromium.org/1792623003 with a TODO/comment about why. That should work right?
https://codereview.chromium.org/2108283002/ created. On Wed, 29 Jun 2016 at 19:19 <bokan@chromium.org> wrote: > On 2016/06/29 18:13:59, Will Shackleton wrote: > > What's the recommended way in chromium to disable some code? #if 0 and a > > comment explaining why? > > #if'ing out code is discouraged. Easiest way is probably just adding an > early > return in UpdateScrollClassDevice like you did for legacy devices in > https://codereview.chromium.org/1792623003 with a TODO/comment about why. > That > should work right? > > https://codereview.chromium.org/2077163003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've made those changes. What's the process for getting this feature re-enabled for testing? A chrome://flags flag? Can we enable it only in google-chrome-unstable builds? Or do we let those with weird hardware just build with a patch to re-enable? https://codereview.chromium.org/2077163003/diff/20001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/2077163003/diff/20001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:205: void InvalidateScrollClasses(int device_id = -1); On 2016/06/27 14:49:21, sadrul wrote: > Remove the default param-value. Document that using -1 as the device-id would > invalidate all devices. Done. https://codereview.chromium.org/2077163003/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:224: xev->sourceid); On 2016/06/27 14:49:21, sadrul wrote: > Can |xev->sourceid| here be really -1? There are other places |InvalidateScrollClasses| is called - see new change now default parameter doesn't exist.
> What's the process for getting this feature re-enabled for testing? > A chrome://flags flag? Can we enable it only in google-chrome-unstable > builds? Or do we let those with weird hardware just build with a patch > to re-enable? I think adding a chrome://flag is probably the best way to get this reenabled (it can default to "on"). Given the diversity of devices and issues we should definitely give people an off switch until we can get everything hammered out. For adding a flag, you can see an example CL like https://codereview.chromium.org/1386403003. You can also do that in a separate CL, this CL lgtm % nits (but you still need Sadrul's stamp). https://codereview.chromium.org/2077163003/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:220: DCHECK_EQ(XISlaveSwitch, xev->reason); Is it guaranteed that there won't ever be a new reason code added? https://codereview.chromium.org/2077163003/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:236: ui::DeviceDataManagerX11::GetInstance()->InvalidateScrollClasses(-1); Make the -1 an Enum (e.g. ALL_DEVICES) for readability (update the comment in the declaration to use that instead of -1 too).
On 2016/07/13 14:45:50, bokan wrote: > > What's the process for getting this feature re-enabled for testing? > > A chrome://flags flag? Can we enable it only in google-chrome-unstable > > builds? Or do we let those with weird hardware just build with a patch > > to re-enable? > > I think adding a chrome://flag is probably the best way to get this reenabled > (it can default to "on"). Given the diversity of devices and issues we should > definitely give people an off switch until we can get everything hammered out. I don't think chrome:flags is a good place for this. Can we go with a cmd-line flag instead? I think that'd be best. (it wouldn't affect a lot of users, and the users it would affect are more likely to be able to launch with a cmdline flag).
On 2016/07/13 15:08:41, sadrul wrote: > On 2016/07/13 14:45:50, bokan wrote: > > > What's the process for getting this feature re-enabled for testing? > > > A chrome://flags flag? Can we enable it only in google-chrome-unstable > > > builds? Or do we let those with weird hardware just build with a patch > > > to re-enable? > > > > I think adding a chrome://flag is probably the best way to get this reenabled > > (it can default to "on"). Given the diversity of devices and issues we should > > definitely give people an off switch until we can get everything hammered out. > > I don't think chrome:flags is a good place for this. Can we go with a cmd-line > flag instead? I think that'd be best. (it wouldn't affect a lot of users, and > the users it would affect are more likely to be able to launch with a cmdline > flag). I hadn't thought of that since I've never done a command-line flag without a chrome://flag but I think you're right, that's probably a better idea.
https://codereview.chromium.org/2077163003/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:220: DCHECK_EQ(XISlaveSwitch, xev->reason); On 2016/07/13 14:45:50, bokan wrote: > Is it guaranteed that there won't ever be a new reason code added? Hah. Not in the current X11 spec but I'll make this an else if just in case. https://codereview.chromium.org/2077163003/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:236: ui::DeviceDataManagerX11::GetInstance()->InvalidateScrollClasses(-1); On 2016/07/13 14:45:50, bokan wrote: > Make the -1 an Enum (e.g. ALL_DEVICES) for readability (update the comment in > the declaration to use that instead of -1 too). Done.
Thanks, still lgtm.
lgtm https://codereview.chromium.org/2077163003/diff/60001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/2077163003/diff/60001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.h:116: }; Use "static const int kAllDevices = -1;" instead? https://codereview.chromium.org/2077163003/diff/60001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2077163003/diff/60001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:235: ui::DeviceDataManagerX11::GetInstance()->InvalidateScrollClasses(-1); ALL_DEVICES?
The CQ bit was checked by w.shackleton@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2077163003/#ps80001 (title: "Changed ALL_DEVICES to kAllDevices")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reduced number of UpdateDeviceList calls I stopped UpdateDeviceList being called every time XISlaveSwitch events occur. BUG=609748 ========== to ========== Reduced number of UpdateDeviceList calls I stopped UpdateDeviceList being called every time XISlaveSwitch events occur. BUG=609748 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Reduced number of UpdateDeviceList calls I stopped UpdateDeviceList being called every time XISlaveSwitch events occur. BUG=609748 ========== to ========== Reduced number of UpdateDeviceList calls I stopped UpdateDeviceList being called every time XISlaveSwitch events occur. BUG=609748 Committed: https://crrev.com/e620fd41c0c6acb7ffb3c44710553797b3c9deb9 Cr-Commit-Position: refs/heads/master@{#407597} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e620fd41c0c6acb7ffb3c44710553797b3c9deb9 Cr-Commit-Position: refs/heads/master@{#407597} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
