New issue
Advanced search Search tips

Issue 717674 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 777570
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

mushrome: Stylus icon shows in status area on non-stylus device (link)

Project Member Reported by jamescook@chromium.org, May 2 2017

Issue description

chrome --mus on link, Chrome r468701, self-built test image 60.0.3088.0

The TrayPalette system tray icon appears. The menu spawns when I click on it. This is weird, since this isn't a stylus device.

The icon does not show up with normal chrome or with chrome --mash.

This does not repro on desktop.

Are we installing an incorrect delegate here? Is there some chromeos::GetAshConfig() check that needs to be fixed?



 
Owner: wutao@chromium.org
Status: Assigned (was: Untriaged)
wutao@ has been reworking how the stylus palette is initialized (which would have also fixed https://codereview.chromium.org/2858583002).

Comment 3 by wutao@chromium.org, May 2 2017

This bug might be slightly different, the bug (in 712887) will add the palette but no show it until press the accelerator as in normal chrome.
I will look into why --mus has this different behavior.
To run --mus you need to edit /etc/chrome_dev.conf to add the --mus flag.

How I do this:
stop ui
cp /etc/chrome_dev.conf /home/
# edit file
mount --bind /home/chrome_dev.conf /etc/chrome_dev.conf
start ui

Comment 5 by wutao@chromium.org, May 3 2017

When use the --mus at link, it returns true that the InputDeviceManager has stylus:
https://cs.chromium.org/chromium/src/ash/system/palette/palette_utils.cc?type=cs&l=34

Comment 6 by wutao@chromium.org, May 4 2017

I found out the reason is that when TouchscreenDevice() is called, the is_stylus value is not explicitly init to false: https://cs.chromium.org/chromium/src/ui/events/devices/touchscreen_device.cc?dr=C&l=13

Add "is_stylus(false)" to the init list, will fix this bug.

Comment 7 by wutao@chromium.org, May 4 2017

sky@ and jamescook@,
I updated the cl[1] to fix this bug as well, please review. Please let me know if I should open another cl for fixing this bug only so that I can land the original cl[1] without delay too long.

Thanks.
[1] https://codereview.chromium.org/2825383003/
Project Member

Comment 8 by bugdroid1@chromium.org, May 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313

commit 53e9c2e81bf4fe7d0f6bcaebca0c717b19430313
Author: wutao <wutao@chromium.org>
Date: Thu May 04 19:19:29 2017

Fix stylus tools palette.

HandleShowStylusTools crashes at desktop ChromeOS UI and shows stylus
tools on device does not have stylus. Adding more checks when we should
HandleShowStylusTools and when to AddPaletteTray.

BUG= 712887 ,  717674 ,  717422 
TEST=Manual && AboutFlagsHistogramTest.CheckHistograms

Review-Url: https://codereview.chromium.org/2825383003
Cr-Commit-Position: refs/heads/master@{#469418}

[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/ash_switches.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/ash_switches.h
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/system/palette/palette_tray.h
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/system/palette/palette_utils.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/system/palette/palette_utils.h
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/system/status_area_widget.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ash/system/status_area_widget_unittest.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/chrome/browser/about_flags.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/chrome/browser/chromeos/login/chrome_restart_request.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/chrome/browser/chromeos/note_taking_helper_unittest.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ui/events/devices/input_device.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ui/events/devices/touchscreen_device.cc
[modify] https://crrev.com/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313/ui/events/devices/touchscreen_device.h

Comment 9 by wutao@chromium.org, May 16 2017

Cc: wutao@chromium.org
Owner: rjkroege@chromium.org
Re assign this to rjkroege@

I fixed part of the problem so the stylus palette will not show in mus mode in LINK, to set the is_stylus(false) in TouchscreenDevice() [1]
[1] https://cs.chromium.org/chromium/src/ui/events/devices/touchscreen_device.cc?dr=C&l=13

But there is another problem. To show the stylus palette, the device type must also be ui::InputDeviceType::INPUT_DEVICE_INTERNAL (enum, 0) [2]
[2] https://cs.chromium.org/chromium/src/ash/system/palette/palette_utils.cc?type=cs&l=36

I am not familiar with the flow of code in mus, but I am afraid the device type is not set to ui::InputDeviceType::INPUT_DEVICE_UNKNOWN (enum, 2) in the input_devices service in mus [3] and set is_stylus to false in [4].
InputDevice::InputDevice(), the type() default is 0? Which means it is internal device.
[3] https://cs.chromium.org/chromium/src/out/Debug/gen/ui/events/devices/mojo/input_devices.mojom.cc?dr=C&l=33
and
TouchscreenDevice::TouchscreenDevice()
[4] https://cs.chromium.org/chromium/src/out/Debug/gen/ui/events/devices/mojo/input_devices.mojom.cc?dr=C&l=62



Comment 10 by wutao@chromium.org, May 16 2017

Also, bool is_stylus is missing in the struct TouchscreenDevice in input_devices.mojom:

https://cs.chromium.org/chromium/src/ui/events/devices/mojo/input_devices.mojom?type=cs&l=30

It seems hard to keep them sync, if someone adds new members in input_device or touchscreen_device.
Owner: sky@chromium.org
sky@ can you find someone else to work on this?

Comment 12 by sky@chromium.org, Aug 22 2017

Status: WontFix (was: Assigned)
This no longer repros on tip of tree.
Mergedinto: 777570
Status: Duplicate (was: WontFix)
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment