Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690323002:
Make Interaction Media Features MQ dynamic on Windows. (Closed)
DescriptionMake Interaction Media Features MQ dynamic on Windows.
With the new convertible and detachable form factors for
laptops it is important to make sure the interaction media
features are updated whenever the keyboard/trackpad combo is
flipped (so inactive) or detached. This will allow content
author to react to media query changes to adapt the user
interface to better suit the new interaction method (often
touch vs trackpad).
BUG=442418
Patch Set 1 : Make Interaction Media Features MQ dynamic on Windows. #Patch Set 2 : Make Interaction Media Features MQ dynamic on Windows. #
Total comments: 3
Patch Set 3 : Rebased #
Total comments: 2
Patch Set 4 : No debug :) #
Total comments: 1
Patch Set 5 : Only update when ConvertibleSlateMode changes #
Total comments: 2
Patch Set 6 : Fix nits from mustaq #Patch Set 7 : Added test #
Total comments: 2
Patch Set 8 : Added Trace #
Total comments: 7
Patch Set 9 : Fix review comments #Patch Set 10 : Nit fix #
Total comments: 1
Messages
Total messages: 85 (43 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP BUG= ========== to ========== With the new convertible and detachable form factors for laptops it is important to make sure the interaction media features are updated whenever the keyboard/trackpad combo is flipped (so inactive) or detached. This will allow content author to react to media query changes to adapt the user interface to better suit the new interaction method (often touch vs trackpad). Intent to ship : ... BUG=442418 ==========
Description was changed from ========== WIP BUG= ========== to ========== With the new convertible and detachable form factors for laptops it is important to make sure the interaction media features are updated whenever the keyboard/trackpad combo is flipped (so inactive) or detached. This will allow content author to react to media query changes to adapt the user interface to better suit the new interaction method (often touch vs trackpad). Intent to ship : ... BUG=442418 ==========
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
alexis.menard@intel.com changed reviewers: + mustaq@chromium.org, rbyers@chromium.org
Description was changed from ========== With the new convertible and detachable form factors for laptops it is important to make sure the interaction media features are updated whenever the keyboard/trackpad combo is flipped (so inactive) or detached. This will allow content author to react to media query changes to adapt the user interface to better suit the new interaction method (often touch vs trackpad). Intent to ship : ... BUG=442418 ========== to ========== With the new convertible and detachable form factors for laptops it is important to make sure the interaction media features are updated whenever the keyboard/trackpad combo is flipped (so inactive) or detached. This will allow content author to react to media query changes to adapt the user interface to better suit the new interaction method (often touch vs trackpad). BUG=442418 ==========
alexis.menard@intel.com changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... File ui/base/touch/touch_device_win.cc (right): https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... ui/base/touch/touch_device_win.cc:26: // while the device is used as a tablet. This is because Windows doesn't provide Sorry I missed this in your previous CL: Can't we change (override) only the primary-pointer-type to hide the GetSystemMetrics bug in tablet mode? https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... ui/base/touch/touch_device_win.cc:39: if (base::win::IsTabletDevice(nullptr)) I think you need to rebase your git branch. Some of these changes are already checked in.
https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... File ui/base/touch/touch_device_win.cc (right): https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... ui/base/touch/touch_device_win.cc:26: // while the device is used as a tablet. This is because Windows doesn't provide On 2017/03/21 17:05:38, mustaq wrote: > Sorry I missed this in your previous CL: Can't we change (override) only the > primary-pointer-type to hide the GetSystemMetrics bug in tablet mode? Assuming you talk about the case a mouse is connected in USB on a Surface Pro where the original keyboard/trackpad is detached it is not possible to do it (or will be finicky) because : - base::win::IsTabletDevice() will return true. - GetSystemMetrics will return true or false (depending how the OEM implemented their drivers). Aka it's possible to have a trackpad listed in Device Manager (the one of the dock) even if it's detached. We could probably dig the HID infos and figure out maybe if the keyboard/trackpad connected is the one of the dock or say an external mouse but I couldn't find a reliable way to do it across OEMs (most of them uses the Generic Microsoft Drivers).
On 2017/03/21 17:14:45, darktears wrote: > https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... > File ui/base/touch/touch_device_win.cc (right): > > https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... > ui/base/touch/touch_device_win.cc:26: // while the device is used as a tablet. > This is because Windows doesn't provide > On 2017/03/21 17:05:38, mustaq wrote: > > Sorry I missed this in your previous CL: Can't we change (override) only the > > primary-pointer-type to hide the GetSystemMetrics bug in tablet mode? > > Assuming you talk about the case a mouse is connected in USB on a Surface Pro > where the original keyboard/trackpad is detached it is not possible to do it (or > will be finicky) because : > - base::win::IsTabletDevice() will return true. > - GetSystemMetrics will return true or false (depending how the OEM implemented > their drivers). Aka it's possible to have a trackpad listed in Device Manager > (the one of the dock) even if it's detached. > > We could probably dig the HID infos and figure out maybe if the > keyboard/trackpad connected is the one of the dock or say an external mouse but > I couldn't find a reliable way to do it across OEMs (most of them uses the > Generic Microsoft Drivers). Not sure I was clear but basically when detached I'm not able to tell if the trackpad listed in Device Manager is the old trackpad from the detached dock or a USB mouse connected. GetSystemMetrics will always return true if the trackpad of the dock is not removed from Device Manager (USB mouse connected or not) and that will defeat the purpose of the common use case which is detaching/flipping and no mouse/trackpad is available so we should tell the website about it.
On 2017/03/21 17:21:08, darktears wrote: > On 2017/03/21 17:14:45, darktears wrote: > > > https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... > > File ui/base/touch/touch_device_win.cc (right): > > > > > https://codereview.chromium.org/2690323002/diff/60001/ui/base/touch/touch_dev... > > ui/base/touch/touch_device_win.cc:26: // while the device is used as a tablet. > > This is because Windows doesn't provide > > On 2017/03/21 17:05:38, mustaq wrote: > > > Sorry I missed this in your previous CL: Can't we change (override) only the > > > primary-pointer-type to hide the GetSystemMetrics bug in tablet mode? > > > > Assuming you talk about the case a mouse is connected in USB on a Surface Pro > > where the original keyboard/trackpad is detached it is not possible to do it > (or > > will be finicky) because : > > - base::win::IsTabletDevice() will return true. > > - GetSystemMetrics will return true or false (depending how the OEM > implemented > > their drivers). Aka it's possible to have a trackpad listed in Device Manager > > (the one of the dock) even if it's detached. > > > > We could probably dig the HID infos and figure out maybe if the > > keyboard/trackpad connected is the one of the dock or say an external mouse > but > > I couldn't find a reliable way to do it across OEMs (most of them uses the > > Generic Microsoft Drivers). > > Not sure I was clear but basically when detached I'm not able to tell if the > trackpad listed in Device Manager is the old trackpad from the detached dock or > a USB mouse connected. GetSystemMetrics will always return true if the trackpad > of the dock is not removed from Device Manager (USB mouse connected or not) and > that will defeat the purpose of the common use case which is detaching/flipping > and no mouse/trackpad is available so we should tell the website about it. sg, thanks for the explanation.
https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/devic... File ui/events/devices/device_data_manager.cc (right): https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/devic... ui/events/devices/device_data_manager.cc:167: LOG(ERROR) << "mDebug " << __FUNCTION__; Please remove the debug logs. https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... File ui/events/devices/input_device_observer_win.cc (right): https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... ui/events/devices/input_device_observer_win.cc:77: NOTIFY_OBSERVERS(NotifyObserversKeyboardDeviceConfigurationChanged(), I guess the the registry key reflects changes in touch-screen setting too? In that case, I suggest adding a OnTouchscreenDeviceConfigurationChanged() too.
On 2017/03/21 17:53:08, mustaq wrote: > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/devic... > File ui/events/devices/device_data_manager.cc (right): > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/devic... > ui/events/devices/device_data_manager.cc:167: LOG(ERROR) << "mDebug " << > __FUNCTION__; > Please remove the debug logs. > Oops, missteps on my side. > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > File ui/events/devices/input_device_observer_win.cc (right): > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > ui/events/devices/input_device_observer_win.cc:77: > NOTIFY_OBSERVERS(NotifyObserversKeyboardDeviceConfigurationChanged(), > I guess the the registry key reflects changes in touch-screen setting too? In > that case, I suggest adding a OnTouchscreenDeviceConfigurationChanged() too. Well the screen configuration did not changed. Aka it's always been there. The registry key just tell that the device is in tablet mode but you could use the touch screen before.
> https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > ui/events/devices/input_device_observer_win.cc:77: > > NOTIFY_OBSERVERS(NotifyObserversKeyboardDeviceConfigurationChanged(), > > I guess the the registry key reflects changes in touch-screen setting too? In > > that case, I suggest adding a OnTouchscreenDeviceConfigurationChanged() too. > > Well the screen configuration did not changed. Aka it's always been there. The > registry key just tell that the device is in tablet mode but you could use the > touch screen before. I was thinking about a different dynamic case: with your current patch if a user disables the trackpad through an OS option, we will get a notification. But if the user disables the touchscreen sensor (perhaps through Device_Manager -> Human_Interface_Devices), we won't. It seems just an easy+cheap call away, no?
On 2017/03/21 19:52:06, mustaq wrote: > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > > ui/events/devices/input_device_observer_win.cc:77: > > > NOTIFY_OBSERVERS(NotifyObserversKeyboardDeviceConfigurationChanged(), > > > I guess the the registry key reflects changes in touch-screen setting too? > In > > > that case, I suggest adding a OnTouchscreenDeviceConfigurationChanged() too. > > > > Well the screen configuration did not changed. Aka it's always been there. The > > registry key just tell that the device is in tablet mode but you could use the > > touch screen before. > > I was thinking about a different dynamic case: with your current patch if a user > disables the trackpad through an OS option, we will get a notification. But if > the user disables the touchscreen sensor (perhaps through Device_Manager -> > Human_Interface_Devices), we won't. It seems just an easy+cheap call away, no? Ok why not...
On 2017/03/21 19:52:06, mustaq wrote: > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > > ui/events/devices/input_device_observer_win.cc:77: > > > NOTIFY_OBSERVERS(NotifyObserversKeyboardDeviceConfigurationChanged(), > > > I guess the the registry key reflects changes in touch-screen setting too? > In > > > that case, I suggest adding a OnTouchscreenDeviceConfigurationChanged() too. > > > > Well the screen configuration did not changed. Aka it's always been there. The > > registry key just tell that the device is in tablet mode but you could use the > > touch screen before. > > I was thinking about a different dynamic case: with your current patch if a user > disables the trackpad through an OS option, we will get a notification. But if > the user disables the touchscreen sensor (perhaps through Device_Manager -> > Human_Interface_Devices), we won't. It seems just an easy+cheap call away, no? Actually disabling the touch screen will not update ConvertibleSlateMode in Windows so we'll not hit the code.
On 2017/03/21 21:38:47, darktears wrote: > On 2017/03/21 19:52:06, mustaq wrote: > > > > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2690323002/diff/80001/ui/events/devices/input... > > > > ui/events/devices/input_device_observer_win.cc:77: > > > > NOTIFY_OBSERVERS(NotifyObserversKeyboardDeviceConfigurationChanged(), > > > > I guess the the registry key reflects changes in touch-screen setting too? > > In > > > > that case, I suggest adding a OnTouchscreenDeviceConfigurationChanged() > too. > > > > > > Well the screen configuration did not changed. Aka it's always been there. > The > > > registry key just tell that the device is in tablet mode but you could use > the > > > touch screen before. > > > > I was thinking about a different dynamic case: with your current patch if a > user > > disables the trackpad through an OS option, we will get a notification. But if > > the user disables the touchscreen sensor (perhaps through Device_Manager -> > > Human_Interface_Devices), we won't. It seems just an easy+cheap call away, no? > > Actually disabling the touch screen will not update ConvertibleSlateMode in > Windows so we'll not hit the code. Actually disabling the touch through the Device Manager is a pretty corner case I would argue, not sure most users knows how to do that.
alexis.menard@intel.com changed reviewers: + ananta@chromium.org
https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... File ui/events/devices/input_device_observer_win.cc (right): https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... ui/events/devices/input_device_observer_win.cc:27: // https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. I couldn't find a connection between ConvertibleSlateMode & this particular registry key, so we need more info about how these two are related. Web search for "PriorityControl" key suggests it's about process priority/scheduling which vaguely matches the key's name. Looks like "ConvertibleSlateMode" is only a part of the settings covered by this key, right? Then we are definitely throwing redundant notifications here, which could be bad for Chrome's run-time behavior. Do we have a way to "read" the ConvertibleSlateMode value through this (or any other) registry key? We can then cache the mode state here in this class, and fire notifications (from within OnRegistryKeyChanged callback) only when the value changes. In any case, we need to make sure that we don't have too frequent notifications. Any Windows doc on which settings are covered by this key?
On 2017/03/22 18:10:03, mustaq wrote: > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > File ui/events/devices/input_device_observer_win.cc (right): > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > ui/events/devices/input_device_observer_win.cc:27: // > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. > I couldn't find a connection between ConvertibleSlateMode & this particular > registry key, so we need more info about how these two are related. Web search > for "PriorityControl" key suggests it's about process priority/scheduling which > vaguely matches the key's name. https://msdn.microsoft.com/en-us/windows/hardware/commercialize/customize/des... The documentation says that "Set up a GPIO pin and a GPIO Injection Driver to detect the mode. When the mode changes, the driver should update the registry value of HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode." Windows Shell uses the ConvertibleSlateMode to display the popup asking the user if he/she wants to switch to tablet/desktop mode. > > Looks like "ConvertibleSlateMode" is only a part of the settings covered by this > key, right? Then we are definitely throwing redundant notifications here, which > could be bad for Chrome's run-time behavior. Do we have a way to "read" the > ConvertibleSlateMode value through this (or any other) registry key? We can then > cache the mode state here in this class, and fire notifications (from within > OnRegistryKeyChanged callback) only when the value changes. I'm not following you here. There is only one notification here. The registry is only updated by the keyboard/trackpad driver when it's connected or disconnected (or flipped). There are two ways (that I know of to know if Windows is in tablet mode) : - The registry key used here - RuntimeClass_Windows_UI_ViewManagement_UIViewSettings which is a Windows RT component to get the value (this is what base/win is doing). As for getting notified when the mode changes we can either listen the registry key or I think there is a WM_SETTINGCHANGE that is sent on the WndProc fct (I have not verified on a win32 apps like Chrome). > > In any case, we need to make sure that we don't have too frequent notifications. > Any Windows doc on which settings are covered by this key? https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device...
On 2017/03/22 21:49:19, darktears wrote: > On 2017/03/22 18:10:03, mustaq wrote: > > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > > ui/events/devices/input_device_observer_win.cc:27: // > > > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. > > I couldn't find a connection between ConvertibleSlateMode & this particular > > registry key, so we need more info about how these two are related. Web search > > for "PriorityControl" key suggests it's about process priority/scheduling > which > > vaguely matches the key's name. > > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/customize/des... > > The documentation says that "Set up a GPIO pin and a GPIO Injection Driver to > detect the mode. When the mode changes, the driver should update the registry > value of > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode." > > Windows Shell uses the ConvertibleSlateMode to display the popup asking the user > if he/she wants to switch to tablet/desktop mode. You are right but my concern is that the key you mentioned here is more "precise" than the one used in this CL. If the key being "watched" was: HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode instead of: HKLM/System/CurrentControlSet/Control/PriorityControl that would be perfect. You must have a reason for not watching the more "precise" key, and my naive guess is that some Windows limitations apply here. Am I right? Assuming I am right so far, I am fearing that OnRegistryKeyChanged() will be called for any change covered by the less "precise" key, like any change in any of the following: HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode HKLM/System/CurrentControlSet/Control/PriorityControl/XyzMode HKLM/System/CurrentControlSet/Control/PriorityControl/PqrSettings HKLM/System/CurrentControlSet/Control/PriorityControl For example, the following "tip" seems to suggest that OnRegistryKeyChanged() will be called whenever a process's priority is changed: http://www.ownedcore.com/forums/world-of-warcraft/world-of-warcraft-guides/13... My suggestion was: we should be able to "read" & cache the value of .../PriorityControl/ConvertibleSlateMode in OnRegistryKeyChanged() so that redundant calls NotifyObserversTouchpadDeviceConfigurationChanged() etc could be suppressed. > > > > > Looks like "ConvertibleSlateMode" is only a part of the settings covered by > this > > key, right? Then we are definitely throwing redundant notifications here, > which > > could be bad for Chrome's run-time behavior. Do we have a way to "read" the > > ConvertibleSlateMode value through this (or any other) registry key? We can > then > > cache the mode state here in this class, and fire notifications (from within > > OnRegistryKeyChanged callback) only when the value changes. > > I'm not following you here. There is only one notification here. The registry is > only updated by the keyboard/trackpad driver when it's connected or disconnected > (or flipped). > > There are two ways (that I know of to know if Windows is in tablet mode) : > - The registry key used here > - RuntimeClass_Windows_UI_ViewManagement_UIViewSettings which is a Windows RT > component to get the value (this is what base/win is doing). > > As for getting notified when the mode changes we can either listen the registry > key or I think there is a WM_SETTINGCHANGE that is sent on the WndProc fct (I > have not verified on a win32 apps like Chrome). > > > > > In any case, we need to make sure that we don't have too frequent > notifications. > > Any Windows doc on which settings are covered by this key? > > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device... This link doesn't mention the "key" to watch for changes. See my explanation above.
On 2017/03/23 14:55:00, mustaq wrote: > On 2017/03/22 21:49:19, darktears wrote: > > On 2017/03/22 18:10:03, mustaq wrote: > > > > > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > > > ui/events/devices/input_device_observer_win.cc:27: // > > > > > > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. > > > I couldn't find a connection between ConvertibleSlateMode & this particular > > > registry key, so we need more info about how these two are related. Web > search > > > for "PriorityControl" key suggests it's about process priority/scheduling > > which > > > vaguely matches the key's name. > > > > > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/customize/des... > > > > The documentation says that "Set up a GPIO pin and a GPIO Injection Driver to > > detect the mode. When the mode changes, the driver should update the registry > > value of > > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode." > > > > Windows Shell uses the ConvertibleSlateMode to display the popup asking the > user > > if he/she wants to switch to tablet/desktop mode. > > You are right but my concern is that the key you mentioned here is more > "precise" > than the one used in this CL. If the key being "watched" was: > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode > instead of: > HKLM/System/CurrentControlSet/Control/PriorityControl > that would be perfect. You must have a reason for not watching the more > "precise" > key, and my naive guess is that some Windows limitations apply here. Am I right? Yes, RegKey let you listen keys. ConvertibleSlateMode is a value. > > Assuming I am right so far, I am fearing that OnRegistryKeyChanged() will be > called for any change covered by the less "precise" key, like any change in any > of the following: > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode > HKLM/System/CurrentControlSet/Control/PriorityControl/XyzMode > HKLM/System/CurrentControlSet/Control/PriorityControl/PqrSettings > HKLM/System/CurrentControlSet/Control/PriorityControl > > For example, the following "tip" seems to suggest that OnRegistryKeyChanged() > will be called whenever a process's priority is changed: > http://www.ownedcore.com/forums/world-of-warcraft/world-of-warcraft-guides/13... > > My suggestion was: we should be able to "read" & cache the value of > .../PriorityControl/ConvertibleSlateMode in OnRegistryKeyChanged() so that > redundant calls NotifyObserversTouchpadDeviceConfigurationChanged() etc > could be suppressed. Ah... I finally got what you meant... Good suggestion let me see if I can improve this to avoid calling the notifiers for nothing. > > > > > > > > > Looks like "ConvertibleSlateMode" is only a part of the settings covered by > > this > > > key, right? Then we are definitely throwing redundant notifications here, > > which > > > could be bad for Chrome's run-time behavior. Do we have a way to "read" the > > > ConvertibleSlateMode value through this (or any other) registry key? We can > > then > > > cache the mode state here in this class, and fire notifications (from within > > > OnRegistryKeyChanged callback) only when the value changes. > > > > I'm not following you here. There is only one notification here. The registry > is > > only updated by the keyboard/trackpad driver when it's connected or > disconnected > > (or flipped). > > > > There are two ways (that I know of to know if Windows is in tablet mode) : > > - The registry key used here > > - RuntimeClass_Windows_UI_ViewManagement_UIViewSettings which is a Windows RT > > component to get the value (this is what base/win is doing). > > > > As for getting notified when the mode changes we can either listen the > registry > > key or I think there is a WM_SETTINGCHANGE that is sent on the WndProc fct (I > > have not verified on a win32 apps like Chrome). > > > > > > > > In any case, we need to make sure that we don't have too frequent > > notifications. > > > Any Windows doc on which settings are covered by this key? > > > > > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device... > > This link doesn't mention the "key" to watch for changes. See my explanation > above.
On 2017/03/23 15:31:21, darktears wrote: > On 2017/03/23 14:55:00, mustaq wrote: > > On 2017/03/22 21:49:19, darktears wrote: > > > On 2017/03/22 18:10:03, mustaq wrote: > > > > > > > > > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > > > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2690323002/diff/100001/ui/events/devices/inpu... > > > > ui/events/devices/input_device_observer_win.cc:27: // > > > > > > > > > > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. > > > > I couldn't find a connection between ConvertibleSlateMode & this > particular > > > > registry key, so we need more info about how these two are related. Web > > search > > > > for "PriorityControl" key suggests it's about process priority/scheduling > > > which > > > > vaguely matches the key's name. > > > > > > > > > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/customize/des... > > > > > > The documentation says that "Set up a GPIO pin and a GPIO Injection Driver > to > > > detect the mode. When the mode changes, the driver should update the > registry > > > value of > > > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode." > > > > > > Windows Shell uses the ConvertibleSlateMode to display the popup asking the > > user > > > if he/she wants to switch to tablet/desktop mode. > > > > You are right but my concern is that the key you mentioned here is more > > "precise" > > than the one used in this CL. If the key being "watched" was: > > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode > > instead of: > > HKLM/System/CurrentControlSet/Control/PriorityControl > > that would be perfect. You must have a reason for not watching the more > > "precise" > > key, and my naive guess is that some Windows limitations apply here. Am I > right? > > Yes, RegKey let you listen keys. ConvertibleSlateMode is a value. > > > > > Assuming I am right so far, I am fearing that OnRegistryKeyChanged() will be > > called for any change covered by the less "precise" key, like any change in > any > > of the following: > > HKLM/System/CurrentControlSet/Control/PriorityControl/ConvertibleSlateMode > > HKLM/System/CurrentControlSet/Control/PriorityControl/XyzMode > > HKLM/System/CurrentControlSet/Control/PriorityControl/PqrSettings > > HKLM/System/CurrentControlSet/Control/PriorityControl > > > > For example, the following "tip" seems to suggest that OnRegistryKeyChanged() > > will be called whenever a process's priority is changed: > > > http://www.ownedcore.com/forums/world-of-warcraft/world-of-warcraft-guides/13... > > > > My suggestion was: we should be able to "read" & cache the value of > > .../PriorityControl/ConvertibleSlateMode in OnRegistryKeyChanged() so that > > redundant calls NotifyObserversTouchpadDeviceConfigurationChanged() etc > > could be suppressed. > > > Ah... I finally got what you meant... > > Good suggestion let me see if I can improve this to avoid calling the notifiers > for nothing. > > > > > > > > > > > > > > Looks like "ConvertibleSlateMode" is only a part of the settings covered > by > > > this > > > > key, right? Then we are definitely throwing redundant notifications here, > > > which > > > > could be bad for Chrome's run-time behavior. Do we have a way to "read" > the > > > > ConvertibleSlateMode value through this (or any other) registry key? We > can > > > then > > > > cache the mode state here in this class, and fire notifications (from > within > > > > OnRegistryKeyChanged callback) only when the value changes. > > > > > > I'm not following you here. There is only one notification here. The > registry > > is > > > only updated by the keyboard/trackpad driver when it's connected or > > disconnected > > > (or flipped). > > > > > > There are two ways (that I know of to know if Windows is in tablet mode) : > > > - The registry key used here > > > - RuntimeClass_Windows_UI_ViewManagement_UIViewSettings which is a Windows > RT > > > component to get the value (this is what base/win is doing). > > > > > > As for getting notified when the mode changes we can either listen the > > registry > > > key or I think there is a WM_SETTINGCHANGE that is sent on the WndProc fct > (I > > > have not verified on a win32 apps like Chrome). > > > > > > > > > > > In any case, we need to make sure that we don't have too frequent > > > notifications. > > > > Any Windows doc on which settings are covered by this key? > > > > > > > > > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device... > > > > This link doesn't mention the "key" to watch for changes. See my explanation > > above. What about the latest version?
LGTM except for two nits: https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... File ui/events/devices/input_device_observer_win.cc (right): https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... ui/events/devices/input_device_observer_win.cc:27: // https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. Nit: Perhaps replace this URL with the redirected address you mentioned in #msg29? https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... ui/events/devices/input_device_observer_win.cc:73: NotifyObserversTouchscreenDeviceConfigurationChanged(); Another nit: now that you have focused the "watch" to only ConvertibleSlateMode which includes only keyboard & touchpad, please omit other notifications (mouse & touchscreen). For dynamic MQs, the end result would be the same since all these 4 calls end up in |notifyRenderViewHost()|.
On 2017/03/23 20:09:10, mustaq wrote: > LGTM except for two nits: > > https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... > File ui/events/devices/input_device_observer_win.cc (right): > > https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... > ui/events/devices/input_device_observer_win.cc:27: // > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. > Nit: Perhaps replace this URL with the redirected address you mentioned in > #msg29? You mean this one ? https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device... ? because https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx points to https://msdn.microsoft.com/windows/hardware/commercialize/customize/desktop/u... > > https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... > ui/events/devices/input_device_observer_win.cc:73: > NotifyObserversTouchscreenDeviceConfigurationChanged(); > Another nit: now that you have focused the "watch" to only ConvertibleSlateMode > which includes only keyboard & touchpad, please omit other notifications (mouse > & touchscreen). For dynamic MQs, the end result would be the same since all > these 4 calls end up in |notifyRenderViewHost()|. Will fix.
On 2017/03/23 20:27:54, darktears wrote: > On 2017/03/23 20:09:10, mustaq wrote: > > LGTM except for two nits: > > > > > https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... > > File ui/events/devices/input_device_observer_win.cc (right): > > > > > https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... > > ui/events/devices/input_device_observer_win.cc:27: // > > > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx. > > Nit: Perhaps replace this URL with the redirected address you mentioned in > > #msg29? > > You mean this one ? > https://msdn.microsoft.com/en-us/windows/hardware/commercialize/design/device... > ? > > because > https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653(v=vs.85).aspx > points to > https://msdn.microsoft.com/windows/hardware/commercialize/customize/desktop/u... > Pick whichever looks more "stable". I thought "dn922653" is some h/w specific. Don't know, your call. (I didn't mean the "continuum" link, no useful here.) > > > > > https://codereview.chromium.org/2690323002/diff/120001/ui/events/devices/inpu... > > ui/events/devices/input_device_observer_win.cc:73: > > NotifyObserversTouchscreenDeviceConfigurationChanged(); > > Another nit: now that you have focused the "watch" to only > ConvertibleSlateMode > > which includes only keyboard & touchpad, please omit other notifications > (mouse > > & touchscreen). For dynamic MQs, the end result would be the same since all > > these 4 calls end up in |notifyRenderViewHost()|. > > Will fix.
alexis.menard@intel.com changed reviewers: + cpu@chromium.org, jam@chromium.org
cpu@chromium.org: Please review changes as it's windows. jam@chromium.org: Please review changes in content/
On 2017/03/23 20:41:10, darktears wrote: > mailto:cpu@chromium.org: Please review changes as it's windows. > > mailto:jam@chromium.org: Please review changes in content/ mailto:sadrul@chromium.org: Please review ui/events/devices/
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not an OWNER in content at all, so I'll leave the detailed reviews to the owners. But a couple questions: 1) What are the other cases where we update all the webkit prefs for ALL renderers? I found one "OnGpuChanged". Is this a pattern we follow elsewhere? Just wondering about the perf implications here. Presumably this shouldn't ever happen frequently (though maybe it could in pathological cases like a BlueTooth mouse with low batter / weak signal). For example, when a Chrome window is moved to a different monitor, do we similar thing? I know viewport size isn't part of WebPreferences (though there are things like default_font_size which might change based on display details?), but do we still send an IPC to every renderer when the display size changes? I had originally assumed this would would include moving the pointer/hover data out of WebPreferences into something that's designed to be more dynamic. But if that's not necessary then that's great! 2) Is it possible to write a content_browser_test for this which verifies that media query event listeners really do get invoked when this changes? It seems like it would be easy for something in the chain to break some day - it would be nice to have a test which verifies that the dynamic nature is truly exposed all the way down to JS/CSS.
On 2017/03/24 16:20:04, Rick Byers wrote: > I'm not an OWNER in content at all, so I'll leave the detailed reviews to the > owners. But a couple questions: https://codereview.chromium.org/2690323002 > > 1) What are the other cases where we update all the webkit prefs for ALL > renderers? I found one "OnGpuChanged". Is this a pattern we follow elsewhere? > > Just wondering about the perf implications here. Presumably this shouldn't ever > happen frequently (though maybe it could in pathological cases like a BlueTooth > mouse with low batter / weak signal). For example, when a Chrome window is Indeed. In that particular case of Windows it would never happen so often only if you flip/detach like crazy. But yes as other OSes are going to use the same mechanism and these do fetch their pointer data differently I see your concern. > moved to a different monitor, do we similar thing? I know viewport size isn't > part of WebPreferences (though there are things like default_font_size which > might change based on display details?), but do we still send an IPC to every > renderer when the display size changes? Resize events are sent over IPC as far as I can tell. https://chromium.googlesource.com/chromium/chromium/+/master/content/renderer... > > I had originally assumed this would would include moving the pointer/hover data > out of WebPreferences into something that's designed to be more dynamic. But if > that's not necessary then that's great! > > 2) Is it possible to write a content_browser_test for this which verifies that > media query event listeners really do get invoked when this changes? It seems > like it would be easy for something in the chain to break some day - it would be > nice to have a test which verifies that the dynamic nature is truly exposed all > the way down to JS/CSS. I have added a test, tell me what you think.
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... File content/browser/renderer_host/input/input_device_change_observer.cc (right): https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... content/browser/renderer_host/input/input_device_change_observer.cc:44: if (render_view_host_->InputDeviceFeaturesChanged()) In case there turns out to be some pathological case where this gets called a lot, please add a TRACE here. Eg. TRACE_EVENT("input", "InputDeviceChangeObserver::notifyRendererViewHost"). Should be unlikely though. https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... File content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc (right): https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc:28: #if defined(OS_WIN) Since this doesn't actually use the InputDeviceObserver, it doesn't need to be windows-specific, does it?
On 2017/04/04 22:39:10, darktears wrote: > On 2017/03/24 16:20:04, Rick Byers wrote: > > I'm not an OWNER in content at all, so I'll leave the detailed reviews to the > > owners. But a couple questions: > > https://codereview.chromium.org/2690323002 > > > > > 1) What are the other cases where we update all the webkit prefs for ALL > > renderers? I found one "OnGpuChanged". Is this a pattern we follow > elsewhere? > > > > Just wondering about the perf implications here. Presumably this shouldn't > ever > > happen frequently (though maybe it could in pathological cases like a > BlueTooth > > mouse with low batter / weak signal). For example, when a Chrome window is > > Indeed. In that particular case of Windows it would never happen so often only > if you flip/detach like crazy. But yes as other OSes are going to use the same > mechanism and these do fetch their pointer data differently I see your concern. I think just adding a TRACE as suggested is enough to mitigate the risk here. As long as people can easily spot the problem / find the expert, we can always revisit in the future in the unlikely event a problematic scenario arises. > > moved to a different monitor, do we similar thing? I know viewport size isn't > > part of WebPreferences (though there are things like default_font_size which > > might change based on display details?), but do we still send an IPC to every > > renderer when the display size changes? > > Resize events are sent over IPC as far as I can tell. > > https://chromium.googlesource.com/chromium/chromium/+/master/content/renderer... Yeah and I just did a test which confirms that when we resize a browser window with lots of tabs we send this IPC in parallel to all renderer processes at once. That seems wasteful to me - I would have thought we'd wait for the user to switch to the tab before resizing it, but I guess it makes sense that we want the tab to be completely fresh when someone switches to it. The same principle applies here for device change, so following the same pattern as resize makes sense to me. Thanks. > > > > > I had originally assumed this would would include moving the pointer/hover > data > > out of WebPreferences into something that's designed to be more dynamic. But > if > > that's not necessary then that's great! > > > > 2) Is it possible to write a content_browser_test for this which verifies that > > media query event listeners really do get invoked when this changes? It seems > > like it would be easy for something in the chain to break some day - it would > be > > nice to have a test which verifies that the dynamic nature is truly exposed > all > > the way down to JS/CSS. > > I have added a test, tell me what you think. Thanks, seems reasonable to me. It's not really testing your CL but it does test all the important downstream bits and what's left in your CL is mostly plumbing and OS-specific stuff we likely can't reasonably test. So I agree what you've done is where the real test value is - verifies that when the device info in the WebKitPreferences is updated, the MQ is actually updated dynamically. So the big picture makes sense to me. sadrul@, are you perhaps the right OWNER to finish the review here?
On 2017/04/05 18:51:24, Rick Byers wrote: > On 2017/04/04 22:39:10, darktears wrote: > > On 2017/03/24 16:20:04, Rick Byers wrote: > > > I'm not an OWNER in content at all, so I'll leave the detailed reviews to > the > > > owners. But a couple questions: > > > > https://codereview.chromium.org/2690323002 > > > > > > > > 1) What are the other cases where we update all the webkit prefs for ALL > > > renderers? I found one "OnGpuChanged". Is this a pattern we follow > > elsewhere? > > > > > > Just wondering about the perf implications here. Presumably this shouldn't > > ever > > > happen frequently (though maybe it could in pathological cases like a > > BlueTooth > > > mouse with low batter / weak signal). For example, when a Chrome window is > > > > Indeed. In that particular case of Windows it would never happen so often only > > if you flip/detach like crazy. But yes as other OSes are going to use the same > > mechanism and these do fetch their pointer data differently I see your > concern. > > I think just adding a TRACE as suggested is enough to mitigate the risk here. > As long as people can easily spot the problem / find the expert, we can always > revisit in the future in the unlikely event a problematic scenario arises. I can add the trace. > > > > moved to a different monitor, do we similar thing? I know viewport size > isn't > > > part of WebPreferences (though there are things like default_font_size which > > > might change based on display details?), but do we still send an IPC to > every > > > renderer when the display size changes? > > > > Resize events are sent over IPC as far as I can tell. > > > > > https://chromium.googlesource.com/chromium/chromium/+/master/content/renderer... > > Yeah and I just did a test which confirms that when we resize a browser window > with lots of tabs we send this IPC in parallel to all renderer processes at > once. That seems wasteful to me - I would have thought we'd wait for the user > to switch to the tab before resizing it, but I guess it makes sense that we want > the tab to be completely fresh when someone switches to it. The same principle > applies here for device change, so following the same pattern as resize makes > sense to me. Thanks. > > > > > > > > > I had originally assumed this would would include moving the pointer/hover > > data > > > out of WebPreferences into something that's designed to be more dynamic. > But > > if > > > that's not necessary then that's great! > > > > > > 2) Is it possible to write a content_browser_test for this which verifies > that > > > media query event listeners really do get invoked when this changes? It > seems > > > like it would be easy for something in the chain to break some day - it > would > > be > > > nice to have a test which verifies that the dynamic nature is truly exposed > > all > > > the way down to JS/CSS. > > > > I have added a test, tell me what you think. > > Thanks, seems reasonable to me. It's not really testing your CL but it does > test all the important downstream bits and what's left in your CL is mostly > plumbing and OS-specific stuff we likely can't reasonably test. So I agree what > you've done is where the real test value is - verifies that when the device info > in the WebKitPreferences is updated, the MQ is actually updated dynamically. Right it doesn't test the entire thing however the only way I could do it would be to manually change the registry value of Windows. (and restore it after) but that's impossible because I can't write in there without proper privileges. So I'm testing as best as possible, hopefully on other platforms we can do better. > > So the big picture makes sense to me. > > sadrul@, are you perhaps the right OWNER to finish the review here? I would appreciate. I will upload the requested changes.
On 2017/04/05 18:45:50, Rick Byers wrote: > https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... > File content/browser/renderer_host/input/input_device_change_observer.cc > (right): > > https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... > content/browser/renderer_host/input/input_device_change_observer.cc:44: if > (render_view_host_->InputDeviceFeaturesChanged()) > In case there turns out to be some pathological case where this gets called a > lot, please add a TRACE here. Eg. TRACE_EVENT("input", > "InputDeviceChangeObserver::notifyRendererViewHost"). Should be unlikely > though. > > https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... > File content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc > (right): > > https://codereview.chromium.org/2690323002/diff/160001/content/browser/render... > content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc:28: > #if defined(OS_WIN) > Since this doesn't actually use the InputDeviceObserver, it doesn't need to be > windows-specific, does it? I intend to remove the ifdef when all OSes are implemented but right now the test will fail on other platforms (see that I modified only touch_device_win).
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
definitely need ananta input here, I see we are doing work arounds about windows behaviors that need some verification. As for input_device_observer_win.cc it seems reasonable but I am kind of surprised that we did not had this before.
One quick nit: Please copy the CL title into the first line of the CL description (followed by an empty line). Otherwise I have seen a long description becoming the title (in reverts, etc)! Still LGTM.
Description was changed from ========== With the new convertible and detachable form factors for laptops it is important to make sure the interaction media features are updated whenever the keyboard/trackpad combo is flipped (so inactive) or detached. This will allow content author to react to media query changes to adapt the user interface to better suit the new interaction method (often touch vs trackpad). BUG=442418 ========== to ========== Make Interaction Media Features MQ dynamic on Windows. With the new convertible and detachable form factors for laptops it is important to make sure the interaction media features are updated whenever the keyboard/trackpad combo is flipped (so inactive) or detached. This will allow content author to react to media query changes to adapt the user interface to better suit the new interaction method (often touch vs trackpad). BUG=442418 ==========
some questions https://codereview.chromium.org/2690323002/diff/180001/ui/events/devices/inpu... File ui/events/devices/input_device_observer_win.cc (right): https://codereview.chromium.org/2690323002/diff/180001/ui/events/devices/inpu... ui/events/devices/input_device_observer_win.cc:44: weak_factory_.GetWeakPtr(), registry_key_.get()); Doesn't WM_SETTINGCHANGE fire when this happens?. Listening to registry change notifications seems like a big hammer here. https://codereview.chromium.org/2690323002/diff/180001/ui/events/devices/inpu... ui/events/devices/input_device_observer_win.cc:64: base::Unretained(this), base::Unretained(key))); Shouldn't we use weakptrs here?
https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... File content/browser/renderer_host/input/input_device_change_observer.cc (right): https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... content/browser/renderer_host/input/input_device_change_observer.cc:44: void InputDeviceChangeObserver::notifyRenderViewHost() { please follow google style guide (i.e NotifyRenderViewHost0 https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... File content/browser/renderer_host/input/input_device_change_observer.h (right): https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... content/browser/renderer_host/input/input_device_change_observer.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" per updated header guidelines https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... content/browser/renderer_host/input/input_device_change_observer.h:12: class CONTENT_EXPORT InputDeviceChangeObserver I don't think the export is needed since you're not instantiating it in a test. please add comments to the class explaining what it does. https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... File content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc (right): https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc:38: rvh->OnWebkitPreferencesChanged(); why do you need to call this? I thought the observer will do this? https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:909: bool RenderViewHostImpl::InputDeviceFeaturesChanged() { why not do this in InputDeviceChangeObserver, then you can avoid exposing this one-off method on RVH? RVH already has a public GetWebKitPreferences.
On 2017/04/08 00:54:47, ananta wrote: > some questions > > https://codereview.chromium.org/2690323002/diff/180001/ui/events/devices/inpu... > File ui/events/devices/input_device_observer_win.cc (right): > > https://codereview.chromium.org/2690323002/diff/180001/ui/events/devices/inpu... > ui/events/devices/input_device_observer_win.cc:44: weak_factory_.GetWeakPtr(), > registry_key_.get()); > Doesn't WM_SETTINGCHANGE fire when this happens?. Listening to registry change > notifications seems like a big hammer here. So I spent some time to check out if WM_SETTINGCHANGE would be suitable. I patched https://chromium.googlesource.com/chromium/src/+/master/ui/views/win/hwnd_mes... so I could listen WM_SETTINGCHANGE. I'm afraid it's not suitable because WM_SETTINGCHANGE is sent when the laptop is switching to tablet mode (the option in Windows). While this is fine when the keyboard/mouse combo is flipped/detached, the event also sent whenever the user manually trigger the tablet mode (in the settings or in the notification center), therefore it's not reflecting the actual status of the pointer (you may still have the device with a physical keyboard/combo attached or not flipped and still use the tablet mode). Please note that ConvertibleSlateMode registry key is independent of WM_SETTINGCHANGE, what happens is that Windows Shell listen the registry to prompt the user to switch to tablet mode (or do it automatically if the user selected that option). ConvertibleSlateMode can still be false (and the keyboard/trackpad connected) even if the device is in tablet mode. IMHO even if it's a hammer, the registry value is the only reliable way to make sure the combo keyboard/mouse is attached/flipped and be notified when the status changed. > > https://codereview.chromium.org/2690323002/diff/180001/ui/events/devices/inpu... > ui/events/devices/input_device_observer_win.cc:64: base::Unretained(this), > base::Unretained(key))); > Shouldn't we use weakptrs here? Fixed. Not sure how I would do with the key parameter. I believe that with weak_factory_.GetWeakPtr() it's enough.
On 2017/04/10 15:01:06, jam wrote: > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > File content/browser/renderer_host/input/input_device_change_observer.cc > (right): > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > content/browser/renderer_host/input/input_device_change_observer.cc:44: void > InputDeviceChangeObserver::notifyRenderViewHost() { > please follow google style guide (i.e NotifyRenderViewHost0 > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > File content/browser/renderer_host/input/input_device_change_observer.h (right): > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > content/browser/renderer_host/input/input_device_change_observer.h:1: // > Copyright (c) 2017 The Chromium Authors. All rights reserved. > no "(c)" per updated header guidelines > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > content/browser/renderer_host/input/input_device_change_observer.h:12: class > CONTENT_EXPORT InputDeviceChangeObserver > I don't think the export is needed since you're not instantiating it in a test. > > please add comments to the class explaining what it does. > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > File content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc > (right): > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > content/browser/renderer_host/input/interaction_mq_dynamic_browsertest.cc:38: > rvh->OnWebkitPreferencesChanged(); > why do you need to call this? I thought the observer will do this? The observer depends on the hardware changes. I tried to emulate the registry change so the observer would be called but it requires elevated privileges do so (for obvious reasons). To the extent I can I'm testing that if the webkit preferences changes then the MQ are changed and the events correctly fired in JS. Thus emulating that the preferences changes by setting some default pointer/hover types, then modifying them in the test and simulate the notification from the platform (part that unfortunately I can't emulate). > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > File content/browser/renderer_host/render_view_host_impl.cc (right): > > https://codereview.chromium.org/2690323002/diff/180001/content/browser/render... > content/browser/renderer_host/render_view_host_impl.cc:909: bool > RenderViewHostImpl::InputDeviceFeaturesChanged() { > why not do this in InputDeviceChangeObserver, then you can avoid exposing this > one-off method on RVH? RVH already has a public GetWebKitPreferences. Fixed all the other comments.
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by alexis.menard@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/04/11 14:53:04, jam wrote: > lgtm Thanks @jam. I will wait ananta feedback now.
On 2017/04/11 15:03:37, darktears wrote: > On 2017/04/11 14:53:04, jam wrote: > > lgtm > > Thanks @jam. > > I will wait ananta feedback now. ping @ananta : mind reviewing?
lgtm
The CQ bit was checked by alexis.menard@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2690323002/#ps220001 (title: "Nit fix")
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": 220001, "attempt_start_ts": 1492236648408730,
"parent_rev": "aec0f17718b8e13656d7c9f9fe795a0b8066ffcc", "commit_rev":
"b12ba34d06c80bf0d6287f429de1b8ced0db0f78"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
Message was sent while issue was closed.
On 2017/04/15 09:06:52, commit-bot: I haz the power wrote: > Prior attempt to commit was detected, but we were not able to check whether the > issue was successfully committed. Please check Git history manually and re-check > CQ or close this issue as needed. Landed here : https://chromium.googlesource.com/chromium/src.git/+/b12ba34d06c80bf0d6287f42...
Message was sent while issue was closed.
Patchset #11 (id:240001) has been deleted
Message was sent while issue was closed.
https://codereview.chromium.org/2690323002/diff/220001/ui/events/devices/inpu... File ui/events/devices/input_device_observer_win.h (right): https://codereview.chromium.org/2690323002/diff/220001/ui/events/devices/inpu... ui/events/devices/input_device_observer_win.h:18: class EVENTS_DEVICES_EXPORT InputDeviceObserverWin { Can this be an InputDeviceManager? e.g. class InputDeviceManagerWin : public InputDeviceManager { };
Message was sent while issue was closed.
On 2017/05/10 01:38:33, sadrul wrote: > https://codereview.chromium.org/2690323002/diff/220001/ui/events/devices/inpu... > File ui/events/devices/input_device_observer_win.h (right): > > https://codereview.chromium.org/2690323002/diff/220001/ui/events/devices/inpu... > ui/events/devices/input_device_observer_win.h:18: class EVENTS_DEVICES_EXPORT > InputDeviceObserverWin { > Can this be an InputDeviceManager? e.g. > > class InputDeviceManagerWin : public InputDeviceManager { > }; It was discussed in this CL but Windows can't reliably tell you the connected devices in the system (sometimes even if the device is physically disconnected, it still appears in the Device Manager). Therefore implementing a subclass of InputDeviceManager is pointless as it will hold potentially bogus list of devices. Even considering it as a subclass I'm not sure the benefits of it. |
