Fix stylus tools palette.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/53e9c2e81bf4fe7d0f6bcaebca0c717b19430313

[wutao, 2017-04-19 22:26:42]

wutao@chromium.org changed reviewers:
+ oshima@chromium.org, stevenjb@chromium.org

[wutao, 2017-04-19 22:26:43]

Hi stevenjb@: Please review changes in 
ash/system/* and
chrome/browser/*


Hi oshima@: Please review changes in 
ash/*.cc+h and
ash/accelerators/*.cc


Hi holte@, Please review changes in tools/metrics/histograms/histograms.xml

Thank you,
Tao

[wutao, 2017-04-19 22:28:22]

wutao@chromium.org changed reviewers:
+ holte@chromium.org

[wutao, 2017-04-19 22:28:22]

Hi holte@,
Please review tools/metrics/histograms/histograms.xml.

Thank you,
Tao

[wutao, 2017-04-19 22:28:59]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 22:29:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2017-04-19 22:32:07]

oshima@chromium.org changed reviewers:
+ jdufault@chromium.org

[oshima, 2017-04-19 22:32:08]

I think it's better for jdufault@ to review this first.

[jdufault, 2017-04-19 22:58:38]

lgtm

https://codereview.chromium.org/2825383003/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2825383003/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:524:
palette_utils::ShouldCreatePalette();
Check this first

https://codereview.chromium.org/2825383003/diff/1/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/2825383003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:212: const display::Display& display =
Instead of checking in the if below just short-circuit the function.

  if (!palette_utils::HasStylusInput())
    return;

[commit-bot: I haz the power, 2017-04-19 23:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 23:33:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steven Holte, 2017-04-20 00:28:31]

histograms lgtm

[wutao, 2017-04-20 16:17:52]

jdufault@, ptal.

Fixed some tests failed in Config::MUS/MASH. 
It seems every test try to AddPaletteTray(). Now we check HasStylusInput() in
AddPaletteTray() and HasStylusInput() uses InputDeviceManager, which is not
created in mus/mash. 

http://crbug.com/712290.

https://codereview.chromium.org/2825383003/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2825383003/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:524:
palette_utils::ShouldCreatePalette();
On 2017/04/19 22:58:38, jdufault wrote:
> Check this first

Done.

https://codereview.chromium.org/2825383003/diff/1/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/2825383003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:212: const display::Display& display =
On 2017/04/19 22:58:38, jdufault wrote:
> Instead of checking in the if below just short-circuit the function.
> 
>   if (!palette_utils::HasStylusInput())
>     return;

Done.

[wutao, 2017-04-20 16:48:23]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 16:49:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2017-04-20 17:41:39]

lgtm

[commit-bot: I haz the power, 2017-04-20 18:01:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 18:01:23]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-04-20 18:25:47]

owner lgtm

[oshima, 2017-04-20 19:53:58]

lgtm

[wutao, 2017-04-22 02:05:08]

Hi jdufault@,

StatusAreaWidget try to AddPaletteTray before InputDeviceManager has the full
device list. So at kevin the palette will be not added.

As discussed, I add StatusAreaWidget to observe InputDeviceManager. And
AddPaletteTray when the devices are updated.

Thanks,
Tao

[wutao, 2017-04-22 02:05:16]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 02:05:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 03:08:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-22 03:08:21]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-04-24 20:10:06]

On 2017/04/22 02:05:08, wutao wrote:
> Hi jdufault@,
> 
> StatusAreaWidget try to AddPaletteTray before InputDeviceManager has the full
> device list. So at kevin the palette will be not added.
> 
> As discussed, I add StatusAreaWidget to observe InputDeviceManager. And
> AddPaletteTray when the devices are updated.
> 
> Thanks,
> Tao

I think the visibility checks are complex enough now that it makes sense to try
to unify them to one location; taking a closer look at SystemTray and
StatusAreaWidget, it looks like the existing pattern is to always create the
tray item even if it will not be visible. The tray instance then fully controls
visibility.

So to unify all of the visibility logic into PaletteTray, we can have one
PaletteTray instance per display (ie, unconditionally create it in
StatusAreaWidget). PaletteTray then hides itself automatically if it is not on
the primary display or if there is no stylus device. The only downside here is a
bit more memory usage for all of the unused PaletteTray instances.

[wutao, 2017-04-24 22:53:03]

Hi jdufault@, ptal.

InputDeviceManager has been wired up for mus/mash test, so do no need to check
Config anymore.

palette_tray icon visibility got updated when: OnSessionStateChanged,
OnLockStateChanged, OnTouchscreenDeviceConfigurationChanged, and
OnPaletteEnabledPrefChanged. It will be visible when user login if available.

Thanks,
Tao

[jdufault, 2017-04-26 18:50:43]

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
File ash/system/palette/palette_utils.h (right):

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
ash/system/palette/palette_utils.h:17: // Returns true if there is a stylus
input device on the internal display.
Revert this change?

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
ash/system/palette/palette_utils.h:18: ASH_EXPORT bool HasStylusInput();
Does this still need to be in the header?

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
ash/system/palette/palette_utils.h:23: // Returns true if we should show
palette.
Can you be a bit more detailed in this comment, ie, what is the difference
between ShouldShowPalette and HasStylusInput?

[wutao, 2017-04-26 22:01:50]

Hi jdufault@, ptal.

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
File ash/system/palette/palette_utils.h (right):

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
ash/system/palette/palette_utils.h:17: // Returns true if there is a stylus
input device on the internal display.
On 2017/04/26 18:50:43, jdufault wrote:
> Revert this change?

Done.

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
ash/system/palette/palette_utils.h:18: ASH_EXPORT bool HasStylusInput();
On 2017/04/26 18:50:43, jdufault wrote:
> Does this still need to be in the header?

Yes. There are multiple sites calling this.

https://codereview.chromium.org/2825383003/diff/60001/ash/system/palette/pale...
ash/system/palette/palette_utils.h:23: // Returns true if we should show
palette.
On 2017/04/26 18:50:42, jdufault wrote:
> Can you be a bit more detailed in this comment, ie, what is the difference
> between ShouldShowPalette and HasStylusInput?

Done.

[jdufault, 2017-05-02 22:11:40]

lgtm, can you also add a class-level comment to PaletteTray?

// PaletteTray has one instance per-display. It is only made visible if the
display is primary and if the device has stylus hardware.

[wutao, 2017-05-04 02:05:18]

Description was changed from

==========
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
TEST=Manual && AboutFlagsHistogramTest.CheckHistograms
==========

to

==========
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
==========

[wutao, 2017-05-04 02:12:51]

wutao@chromium.org changed reviewers:
+ jamescook@chromium.org, sky@chromium.org

[wutao, 2017-05-04 02:12:52]

Hi James: Please review the fix for showing palette under mus:
 ui/events/devices/touchscreen_device.cc

Hi sky@: Please review changes in as owner:
 ui/events/devices/touchscreen_device.cc


Fixing a new bug[1] in this cl. Please let me know if it is not a quick fix, I
will land this cl first and upload another cl to fix this bug[1].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=717674

[sky, 2017-05-04 02:16:23]

LGTM

[wutao, 2017-05-04 02:20:12]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 02:20:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 02:24:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 02:24:16]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[wutao, 2017-05-04 03:56:20]

enums was split from histograms.xml. Rebased and updated.

[wutao, 2017-05-04 03:56:32]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 03:56:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 04:53:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 04:53:46]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-05-04 14:10:01]

LGTM with nit

https://codereview.chromium.org/2825383003/diff/120001/ui/events/devices/touc...
File ui/events/devices/touchscreen_device.cc (right):

https://codereview.chromium.org/2825383003/diff/120001/ui/events/devices/touc...
ui/events/devices/touchscreen_device.cc:13:
TouchscreenDevice::TouchscreenDevice() : touch_points(0), is_stylus(false) {}
nit: This class should probably have a virtual destructor.

Also, vendor_id and product_id look uninitialized in the base class.

[wutao, 2017-05-04 17:59:23]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 17:59:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wutao, 2017-05-04 18:03:58]

On 2017/05/04 14:10:01, James Cook wrote:
> LGTM with nit
> 
>
https://codereview.chromium.org/2825383003/diff/120001/ui/events/devices/touc...
> File ui/events/devices/touchscreen_device.cc (right):
> 
>
https://codereview.chromium.org/2825383003/diff/120001/ui/events/devices/touc...
> ui/events/devices/touchscreen_device.cc:13:
> TouchscreenDevice::TouchscreenDevice() : touch_points(0), is_stylus(false) {}
> nit: This class should probably have a virtual destructor.
> 
> Also, vendor_id and product_id look uninitialized in the base class.

Thanks. Fixed nits in patch 8.

[commit-bot: I haz the power, 2017-05-04 19:06:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 19:06:31]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-05-04 19:10:03]

The CQ bit was checked by wutao@chromium.org

[wutao, 2017-05-04 19:10:04]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
stevenjb@chromium.org, oshima@chromium.org, jdufault@chromium.org,
sky@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2825383003/#ps140001
(title: "Fix nits.")

[commit-bot: I haz the power, 2017-05-04 19:10:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 19:19:33]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493925003944050,
"parent_rev": "4252105d0539d5e9968f42f21d3387dc834b09a4", "commit_rev":
"53e9c2e81bf4fe7d0f6bcaebca0c717b19430313"}

[commit-bot: I haz the power, 2017-05-04 19:19:49]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/53e9c2e81bf4fe7d0f6bcaebca0c...
==========

[commit-bot: I haz the power, 2017-05-04 19:19:51]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/53e9c2e81bf4fe7d0f6bcaebca0c...