Show overlay displaying the state of all sticky keys when it is enabled.

Show overlay displaying the state of all sticky keys when it is enabled.

BUG=229042
TEST=manual, new unit tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245382

[Tim Song, 2014-01-13 21:17:41]

Please take a look. This overlay UI is based on the exit warning overlay. 

You can play around interactive mock can be found here:

https://folio.googleplex.com/chrome-ux/mocks/172-nattoki/00-visual_feedback/0...

[James Cook, 2014-01-14 00:59:03]

A few things:
* I'm unclear on what is happening here with Search - does that key actually
stick?  If so, where is that handled?
* I think you should use a layer animation.
* The new overlay class needs a test.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_constants.h (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_constants.h:24: // modified. After that, sticky key
state become DISABLED.
become -> becomes

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_controller.cc:190: overlay_->SetModifierKeyState(
You don't update for the SEARCH key?

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:27: // Horizontal and vertical offset of
the overlay on the screen.
Offset from... top-left corner?  And maybe this should be separate constants

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:52: StickyKeyOverlayLabel(const
std::string& key_name);
explicit

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:68: SetText(base::UTF8ToUTF16(key_name));
Should you be using localized strings here? I don't know if those keys ever have
different labels.  If you don't have to localize them, comment so here.

I see constants for IDS_FILE_BROWSER_SHORTCUT_CTRL, _SHIFT, etc. in
generated_resources.grd.  If you need them, maybe you could rename those
constants and share them?

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:72: SetMultiLine(true);
Does it have to be multiline? The text is single lines in the mock.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:85: int style;
either init style or don't init label_color, it's weird to see one initialized
but not the other

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:112: flags |
gfx::Canvas::NO_SUBPIXEL_RENDERING);
Why are you turning off subpixel rendering? Because the view is transparent?

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:161: if (it != modifier_label_map_.end())
{
optional: You could also DCHECK(it != map_.end()), which would eliminate a
slightly awkward NOTREACHED in the else

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:181: overlay_view_ = new
StickyKeysOverlayView();
do this in overlay_view_() above

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:203: animation_.reset(new
gfx::SlideAnimation(this));
I think you should use layer animations for this. It will happen mostly on the
GPU, be smoother, and make it easier to add things like fade in/out, ease
in/out, etc. For an example see c/b/ui/ash/ash_keyboard_controller_proxy.cc. The
only tricky bit is making sure to clean up the observer when the animation is
aborted.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_overlay.h (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:26: // Controls the overlay UI for sticky
keys.
nit: This comment could mention what the UI looks like, or where it goes on the
screen.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:51: scoped_ptr<gfx::Size> widget_size_;
Why not just a member gfx::Size?

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:52: StickyKeysOverlayView* overlay_view_;
I presume this is not owned, might be nice to comment that.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:58: #endif 
//ASH_STICKY_KEYS_STICKY_KEYS_OVERLAY_H_
nit: space after //

[Tim Song, 2014-01-14 03:01:10]

Thank you for the code review, James. I will change the animation to use a layer
animation and write some tests. Stay tuned...

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_constants.h (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_constants.h:24: // modified. After that, sticky key
state become DISABLED.
On 2014/01/14 00:59:03, James Cook wrote:
> become -> becomes

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_controller.cc:190: overlay_->SetModifierKeyState(
On 2014/01/14 00:59:03, James Cook wrote:
> You don't update for the SEARCH key?

Support for the search key hasn't been implemented yet (tracked in
crbug.com/322118). I looked into it, but unlike the other modifier keys, the
functionality of the search key is mostly driven by rewriting the input events,
and it didn't look trivial to untangle that web.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:27: // Horizontal and vertical offset of
the overlay on the screen.
On 2014/01/14 00:59:03, James Cook wrote:
> Offset from... top-left corner?  And maybe this should be separate constants

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:52: StickyKeyOverlayLabel(const
std::string& key_name);
On 2014/01/14 00:59:03, James Cook wrote:
> explicit

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:68: SetText(base::UTF8ToUTF16(key_name));
On 2014/01/14 00:59:03, James Cook wrote:
> Should you be using localized strings here? I don't know if those keys ever
have
> different labels.  If you don't have to localize them, comment so here.
> 
> I see constants for IDS_FILE_BROWSER_SHORTCUT_CTRL, _SHIFT, etc. in
> generated_resources.grd.  If you need them, maybe you could rename those
> constants and share them?

Done. Good point, I'm not really sure, but if I had to guess, it makes sense for
the search key to be localized. I don't think ash can depend on
generated_resources.grd, so I'll add some new strings to ash_strings.grd.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:72: SetMultiLine(true);
On 2014/01/14 00:59:03, James Cook wrote:
> Does it have to be multiline? The text is single lines in the mock.

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:85: int style;
On 2014/01/14 00:59:03, James Cook wrote:
> either init style or don't init label_color, it's weird to see one initialized
> but not the other

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:112: flags |
gfx::Canvas::NO_SUBPIXEL_RENDERING);
On 2014/01/14 00:59:03, James Cook wrote:
> Why are you turning off subpixel rendering? Because the view is transparent?

Yeah, I was seeing some weird artifacts with the text label on the transparent
background.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:161: if (it != modifier_label_map_.end())
{
On 2014/01/14 00:59:03, James Cook wrote:
> optional: You could also DCHECK(it != map_.end()), which would eliminate a
> slightly awkward NOTREACHED in the else

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.cc:181: overlay_view_ = new
StickyKeysOverlayView();
On 2014/01/14 00:59:03, James Cook wrote:
> do this in overlay_view_() above

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
File ash/sticky_keys/sticky_keys_overlay.h (right):

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:26: // Controls the overlay UI for sticky
keys.
On 2014/01/14 00:59:03, James Cook wrote:
> nit: This comment could mention what the UI looks like, or where it goes on
the
> screen.

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:51: scoped_ptr<gfx::Size> widget_size_;
On 2014/01/14 00:59:03, James Cook wrote:
> Why not just a member gfx::Size?

Done. We may need support for resizing the overlay based on left vs. right
modifier keys, but a member variable for this should be fine too.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:52: StickyKeysOverlayView* overlay_view_;
On 2014/01/14 00:59:03, James Cook wrote:
> I presume this is not owned, might be nice to comment that.

Done.

https://codereview.chromium.org/137373003/diff/1/ash/sticky_keys/sticky_keys_...
ash/sticky_keys/sticky_keys_overlay.h:58: #endif 
//ASH_STICKY_KEYS_STICKY_KEYS_OVERLAY_H_
On 2014/01/14 00:59:03, James Cook wrote:
> nit: space after //

Done.

[James Cook, 2014-01-14 16:44:21]

If search doesn't work yet I would take it out of this patch.

[Tim Song, 2014-01-15 21:07:34]

I took the search key out of this patch as you suggested, switched to using
layer animations, and wrote some unit tests. Please take another look, James.

[James Cook, 2014-01-15 22:53:52]

Looking better, a few more comments / questions

https://codereview.chromium.org/137373003/diff/220001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/137373003/diff/220001/ash/ash_strings.grd#new...
ash/ash_strings.grd:601: <message name="IDS_ASH_STICKY_KEY_CONTROL" desc="Name
of [Ctrl] key name used for sticky keys. Shouldn't be translated in many
languages actually.">
These could probably have more generic names like IDS_ASH_CONTROL_KEY so other
features would be OK with using them.  You might also want to explicitly say
they should be lower-case.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_constants.h (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_constants.h:31: }  // ash
nit: "namespace ash"

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_controller.cc:27: switch (modifier) {
It's unfortunate you need to have your own set of modifier flags. I chatted with
sky@ -- he would be OK with adding ui::EF_SEARCH_DOWN (similar to how we have
EF_COMMAND_DOWN for Mac OS).  Would that make things easier for you?

I'm not sure what implications that would have for the rest of the input stack,
but it might make eventual sticky-search support easier.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:56: StickyKeyState state() { return
state_; }
nit: const, and usually we put these above the Set* functions

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:59: virtual void PaintText(gfx::Canvas*
canvas,
nit: comment what class you are overriding, like "views::Label overrides:"

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:79:
set_border(views::Border::CreateEmptyBorder(0, 0, 0, 0));
I'm a bit confused -- this adds an empty, zero-width border?  Do we need a
border at all?

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:127: virtual void OnPaint(gfx::Canvas*
canvas) OVERRIDE;
nit: comment what you are overriding

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:228: if (animator)
Is animator ever null?  I don't think so, and probably the code before would
have problems if it was.  Probably you don't need this if.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:234:
settings.SetTweenType(gfx::Tween::EASE_IN_OUT);
You might want to use EASE_OUT here, or EASE_OUT as it becomes visible and
EASE_IN as it goes away.  (I hate the names of these tween types; "in" and "out"
correspond to the beginning and end of the animation curve, not direction.)

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:251: gfx::Transform
StickyKeysOverlay::CalculateOverlayTransform() {
This doesn't seem quite right. It looks like the widget position is 0,0 and
you're just moving the layer.  I think what you want to do is position the
widget at spacing,spacing and then animate the transform X coordinate from
-widget_size.width() to 0.  Generally we want the layer to match the widget/view
location unless you are in the middle of an animation.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:281: }  // ash
"namespace ash"

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.h (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.h:26: // Controls the overlay UI for sticky
keys, a transparent overlay at the top
nit: Might mention the word "accessibility" in this comment.  (I'm on a
class-header-comment rampage right now, since I'm refactoring code I've never
seen before and good class header descriptions make it much much easier.)

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.h:45: bool is_showing() { return
is_showing_; }
optional: Would this be better as is_visible()?  I first thought "is_showing()"
meant that the show-animation was playing.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.h:66: }  // ash
"namespace ash"

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay_unittest.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay_unittest.cc:18: protected:
public is OK here

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay_unittest.cc:220: }
Nice test suite! Very thorough.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay_unittest.cc:222: }  // ash
namespace ash

[Tim Song, 2014-01-16 00:01:59]

https://codereview.chromium.org/137373003/diff/220001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/137373003/diff/220001/ash/ash_strings.grd#new...
ash/ash_strings.grd:601: <message name="IDS_ASH_STICKY_KEY_CONTROL" desc="Name
of [Ctrl] key name used for sticky keys. Shouldn't be translated in many
languages actually.">
On 2014/01/15 22:53:53, James Cook wrote:
> These could probably have more generic names like IDS_ASH_CONTROL_KEY so other
> features would be OK with using them.  You might also want to explicitly say
> they should be lower-case.

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_constants.h (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_constants.h:31: }  // ash
On 2014/01/15 22:53:53, James Cook wrote:
> nit: "namespace ash"

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_controller.cc:27: switch (modifier) {
On 2014/01/15 22:53:53, James Cook wrote:
> It's unfortunate you need to have your own set of modifier flags. I chatted
with
> sky@ -- he would be OK with adding ui::EF_SEARCH_DOWN (similar to how we have
> EF_COMMAND_DOWN for Mac OS).  Would that make things easier for you?
> 
> I'm not sure what implications that would have for the rest of the input
stack,
> but it might make eventual sticky-search support easier.

It would certainly be cleaner and I think a bit more consistent, as the ChromeOS
EventRewriter is using the Mod4Mask XEvent flag rather than a more platform
independent one search flag.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:56: StickyKeyState state() { return
state_; }
On 2014/01/15 22:53:53, James Cook wrote:
> nit: const, and usually we put these above the Set* functions

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:59: virtual void PaintText(gfx::Canvas*
canvas,
On 2014/01/15 22:53:53, James Cook wrote:
> nit: comment what class you are overriding, like "views::Label overrides:"

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:79:
set_border(views::Border::CreateEmptyBorder(0, 0, 0, 0));
On 2014/01/15 22:53:53, James Cook wrote:
> I'm a bit confused -- this adds an empty, zero-width border?  Do we need a
> border at all?

Done. I wanted to ensure the insets to be zero, but it appears to already be the
case.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:127: virtual void OnPaint(gfx::Canvas*
canvas) OVERRIDE;
On 2014/01/15 22:53:53, James Cook wrote:
> nit: comment what you are overriding

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:228: if (animator)
On 2014/01/15 22:53:53, James Cook wrote:
> Is animator ever null?  I don't think so, and probably the code before would
> have problems if it was.  Probably you don't need this if.

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:234:
settings.SetTweenType(gfx::Tween::EASE_IN_OUT);
On 2014/01/15 22:53:53, James Cook wrote:
> You might want to use EASE_OUT here, or EASE_OUT as it becomes visible and
> EASE_IN as it goes away.  (I hate the names of these tween types; "in" and
"out"
> correspond to the beginning and end of the animation curve, not direction.)

Done. The mock is using the CSS animation transition type of 'ease', which
doesn't seem to have an equivalent in Aura, and EASE_IN_OUT seemed like the
closest approximation. With such a short animation duration, I don't really see
the difference either way.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:251: gfx::Transform
StickyKeysOverlay::CalculateOverlayTransform() {
On 2014/01/15 22:53:53, James Cook wrote:
> This doesn't seem quite right. It looks like the widget position is 0,0 and
> you're just moving the layer.  I think what you want to do is position the
> widget at spacing,spacing and then animate the transform X coordinate from
> -widget_size.width() to 0.  Generally we want the layer to match the
widget/view
> location unless you are in the middle of an animation.

Done. What about the hidden case? Is it okay to leave the transform at non-zero
when the animation is finished?

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:281: }  // ash
On 2014/01/15 22:53:53, James Cook wrote:
> "namespace ash"

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.h (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.h:26: // Controls the overlay UI for sticky
keys, a transparent overlay at the top
On 2014/01/15 22:53:53, James Cook wrote:
> nit: Might mention the word "accessibility" in this comment.  (I'm on a
> class-header-comment rampage right now, since I'm refactoring code I've never
> seen before and good class header descriptions make it much much easier.)

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.h:45: bool is_showing() { return
is_showing_; }
On 2014/01/15 22:53:53, James Cook wrote:
> optional: Would this be better as is_visible()?  I first thought
"is_showing()"
> meant that the show-animation was playing.

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.h:66: }  // ash
On 2014/01/15 22:53:53, James Cook wrote:
> "namespace ash"

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay_unittest.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay_unittest.cc:18: protected:
On 2014/01/15 22:53:53, James Cook wrote:
> public is OK here

Done.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay_unittest.cc:220: }
On 2014/01/15 22:53:53, James Cook wrote:
> Nice test suite! Very thorough.

Thanks!

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay_unittest.cc:222: }  // ash
On 2014/01/15 22:53:53, James Cook wrote:
> namespace ash

Done.

[James Cook, 2014-01-16 00:46:20]

Almost there. A couple more questions.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_controller.cc:27: switch (modifier) {
On 2014/01/16 00:01:59, Tim Song wrote:
> On 2014/01/15 22:53:53, James Cook wrote:
> > It's unfortunate you need to have your own set of modifier flags. I chatted
> with
> > sky@ -- he would be OK with adding ui::EF_SEARCH_DOWN (similar to how we
have
> > EF_COMMAND_DOWN for Mac OS).  Would that make things easier for you?
> > 
> > I'm not sure what implications that would have for the rest of the input
> stack,
> > but it might make eventual sticky-search support easier.
> 
> It would certainly be cleaner and I think a bit more consistent, as the
ChromeOS
> EventRewriter is using the Mod4Mask XEvent flag rather than a more platform
> independent one search flag.

So do you want to make that change in this patch?  Or a follow-up one?

Given that search doesn't work yet do you want to remove the STICKY_KEY_SEARCH
constant from this patch?

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:251: gfx::Transform
StickyKeysOverlay::CalculateOverlayTransform() {
On 2014/01/16 00:01:59, Tim Song wrote:
> On 2014/01/15 22:53:53, James Cook wrote:
> > This doesn't seem quite right. It looks like the widget position is 0,0 and
> > you're just moving the layer.  I think what you want to do is position the
> > widget at spacing,spacing and then animate the transform X coordinate from
> > -widget_size.width() to 0.  Generally we want the layer to match the
> widget/view
> > location unless you are in the middle of an animation.
> 
> Done. What about the hidden case? Is it okay to leave the transform at
non-zero
> when the animation is finished?

I think we generally set it back to the identity transform when we're done.

[Tim Song, 2014-01-16 01:59:44]

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_controller.cc:27: switch (modifier) {
On 2014/01/16 00:46:20, James Cook wrote:
> On 2014/01/16 00:01:59, Tim Song wrote:
> > On 2014/01/15 22:53:53, James Cook wrote:
> > > It's unfortunate you need to have your own set of modifier flags. I
chatted
> > with
> > > sky@ -- he would be OK with adding ui::EF_SEARCH_DOWN (similar to how we
> have
> > > EF_COMMAND_DOWN for Mac OS).  Would that make things easier for you?
> > > 
> > > I'm not sure what implications that would have for the rest of the input
> > stack,
> > > but it might make eventual sticky-search support easier.
> > 
> > It would certainly be cleaner and I think a bit more consistent, as the
> ChromeOS
> > EventRewriter is using the Mod4Mask XEvent flag rather than a more platform
> > independent one search flag.
> 
> So do you want to make that change in this patch?  Or a follow-up one?
> 
> Given that search doesn't work yet do you want to remove the STICKY_KEY_SEARCH
> constant from this patch?

Let's do this in a separate patch. I removed the modifier enum and the search
key.

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/220001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:251: gfx::Transform
StickyKeysOverlay::CalculateOverlayTransform() {
On 2014/01/16 00:46:20, James Cook wrote:
> On 2014/01/16 00:01:59, Tim Song wrote:
> > On 2014/01/15 22:53:53, James Cook wrote:
> > > This doesn't seem quite right. It looks like the widget position is 0,0
and
> > > you're just moving the layer.  I think what you want to do is position the
> > > widget at spacing,spacing and then animate the transform X coordinate from
> > > -widget_size.width() to 0.  Generally we want the layer to match the
> > widget/view
> > > location unless you are in the middle of an animation.
> > 
> > Done. What about the hidden case? Is it okay to leave the transform at
> non-zero
> > when the animation is finished?
> 
> I think we generally set it back to the identity transform when we're done.

Done. The widget bounds is now updated whenever we show/hide.

[James Cook, 2014-01-16 17:45:59]

LGTM with nits, assuming the X position thing is either OK or fixed.

Nice patch!  It took a few rounds of review, but that's  typical for a 700 line
diff. :-)

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_constants.h (right):

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_constants.h:5: #ifndef
ASH_STICKY_KEYS_STICKY_KEYS_CONSTANTS_H_
If you think this file will only contain the single enum I suggest naming it
sticky_keys_state.h.  If you think it will gain more constants leave it as is.

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:194: widget_size_ =
overlay_view_->GetPreferredSize();
nit: this could go in the member initializers above if you reordered the
variables in the header

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:235: sign * (widget_size_.width() +
kHorizontalOverlayOffset), 0);
Nice use of |sign| to keep the math readable!

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:262: - widget_size_.width() -
kHorizontalOverlayOffset;
Should this just be "-widget_size.width()"?  Isn't the target to put it just off
the screen, rather than off the screen + more padding?

[Tim Song, 2014-01-16 19:11:55]

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_constants.h (right):

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_constants.h:5: #ifndef
ASH_STICKY_KEYS_STICKY_KEYS_CONSTANTS_H_
On 2014/01/16 17:45:59, James Cook wrote:
> If you think this file will only contain the single enum I suggest naming it
> sticky_keys_state.h.  If you think it will gain more constants leave it as is.

Done. Renamed to sticky_keys_state.h

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:194: widget_size_ =
overlay_view_->GetPreferredSize();
On 2014/01/16 17:45:59, James Cook wrote:
> nit: this could go in the member initializers above if you reordered the
> variables in the header

Done.

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:235: sign * (widget_size_.width() +
kHorizontalOverlayOffset), 0);
On 2014/01/16 17:45:59, James Cook wrote:
> Nice use of |sign| to keep the math readable!

Thanks!

https://codereview.chromium.org/137373003/diff/540001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:262: - widget_size_.width() -
kHorizontalOverlayOffset;
On 2014/01/16 17:45:59, James Cook wrote:
> Should this just be "-widget_size.width()"?  Isn't the target to put it just
off
> the screen, rather than off the screen + more padding?

Done. You're right, I was using the calculation for the translation a couple of
patch sets ago.

[commit-bot: I haz the power, 2014-01-16 21:20:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/137373003/710001

[commit-bot: I haz the power, 2014-01-16 23:51:30]

Change committed as 245382