Add a11y virtual keyboard icon when a11y VK is enabled

Add a11y virtual keyboard icon when a11y VK is enabled

This CL adds a virtual keyboard icon when a11y VK is enabled. Click or tap on
the icon will force the VK to show and lock.

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

[bshe, 2014-02-28 22:08:04]

Hi Kevin.

Do you mind to take a first look at this CL?

Thanks

[kevers, 2014-03-03 16:58:45]

https://codereview.chromium.org/177123016/diff/20001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/177123016/diff/20001/ash/ash.gyp#newcode454
ash/ash.gyp:454: 'system/virtual_keyboard/virtual_keyboard_tray.cc',
Might be more appropriate to move to system/chromeos/virtual_keyboard.  We now
have Ash for Windows and this variant of the virtual keyboard is ChromeOS only.

https://codereview.chromium.org/177123016/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.h (left):

https://codereview.chromium.org/177123016/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.h:41: void set_size(const gfx::Size& size)
{ size_ = size; }
Is this change intentional?  If this is dead code, it should probably be moved
to a separate CL as it is not specific to the a11y keyboard.

[bshe, 2014-03-03 20:24:03]

Thanks. Do you mind to take another look?

+sadrul for ash/system/*
+oshima for ash/resources/*

Thanks!

https://codereview.chromium.org/177123016/diff/20001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/177123016/diff/20001/ash/ash.gyp#newcode454
ash/ash.gyp:454: 'system/virtual_keyboard/virtual_keyboard_tray.cc',
On 2014/03/03 16:58:45, kevers wrote:
> Might be more appropriate to move to system/chromeos/virtual_keyboard.  We now
> have Ash for Windows and this variant of the virtual keyboard is ChromeOS
only.

Done.

https://codereview.chromium.org/177123016/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.h (left):

https://codereview.chromium.org/177123016/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.h:41: void set_size(const gfx::Size& size)
{ size_ = size; }
On 2014/03/03 16:58:45, kevers wrote:
> Is this change intentional?  If this is dead code, it should probably be moved
> to a separate CL as it is not specific to the a11y keyboard.

Done.

[kevers, 2014-03-03 21:41:09]

lgtm.

Suggest looping in the a11y team.  dtseng reviewed the earlier patches for the
a11y VK.

[bshe, 2014-03-03 21:53:12]

thanks.

+dtseng for a11y

[oshima, 2014-03-03 22:58:28]

ash/resources lgtm

[sadrul, 2014-03-04 07:00:32]

https://codereview.chromium.org/177123016/diff/40001/ash/system/chromeos/virt...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/177123016/diff/40001/ash/system/chromeos/virt...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:40:
VirtualKeyboardButton::VirtualKeyboardButton(views::ButtonListener* listener)
Shouldn't these be inside the anonymous namespace too?

https://codereview.chromium.org/177123016/diff/40001/ash/system/status_area_w...
File ash/system/status_area_widget.h (right):

https://codereview.chromium.org/177123016/diff/40001/ash/system/status_area_w...
ash/system/status_area_widget.h:22: class VirtualKeyboardTray;
sort

[bshe, 2014-03-04 16:11:53]

sadrul@ I have addressed your comments. Also, it looks like my patch failed on
win platform. I wrapped everything I added in chromeos ifdef guard in
status_area_widget.*. I am not sure if this is the preferred/correct way. Do you
have any comment?

Thanks!

https://codereview.chromium.org/177123016/diff/40001/ash/system/chromeos/virt...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/177123016/diff/40001/ash/system/chromeos/virt...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:40:
VirtualKeyboardButton::VirtualKeyboardButton(views::ButtonListener* listener)
doh. Sorry.

On 2014/03/04 07:00:32, sadrul wrote:
> Shouldn't these be inside the anonymous namespace too?

https://codereview.chromium.org/177123016/diff/40001/ash/system/status_area_w...
File ash/system/status_area_widget.h (right):

https://codereview.chromium.org/177123016/diff/40001/ash/system/status_area_w...
ash/system/status_area_widget.h:22: class VirtualKeyboardTray;
On 2014/03/04 07:00:32, sadrul wrote:
> sort

Done.

[sadrul, 2014-03-04 16:25:22]

On 2014/03/04 16:11:53, bshe wrote:
> sadrul@ I have addressed your comments. Also, it looks like my patch failed on
> win platform. I wrapped everything I added in chromeos ifdef guard in
> status_area_widget.*. I am not sure if this is the preferred/correct way. Do
you
> have any comment?

I think this is correct. A few nits below, otherwise LGTM

https://codereview.chromium.org/177123016/diff/60001/ash/system/chromeos/virt...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right):

https://codereview.chromium.org/177123016/diff/60001/ash/system/chromeos/virt...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:42: // Overridden
from AccessibilityObserver.
The comment should be consistent, so '// AccessibilityObserver:'

https://codereview.chromium.org/177123016/diff/60001/ash/system/status_area_w...
File ash/system/status_area_widget.h (right):

https://codereview.chromium.org/177123016/diff/60001/ash/system/status_area_w...
ash/system/status_area_widget.h:25: #endif  // OS_CHROMEOS
I think the comment should be 'defined(OS_CHROMEOS)'

But for really small blocks like this, I don't think you need the comments at
all (for all changes in this file).

[bshe, 2014-03-04 18:56:14]

thanks for review

https://codereview.chromium.org/177123016/diff/60001/ash/system/chromeos/virt...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right):

https://codereview.chromium.org/177123016/diff/60001/ash/system/chromeos/virt...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:42: // Overridden
from AccessibilityObserver.
On 2014/03/04 16:25:22, sadrul wrote:
> The comment should be consistent, so '// AccessibilityObserver:'

Done.

https://codereview.chromium.org/177123016/diff/60001/ash/system/status_area_w...
File ash/system/status_area_widget.h (right):

https://codereview.chromium.org/177123016/diff/60001/ash/system/status_area_w...
ash/system/status_area_widget.h:25: #endif  // OS_CHROMEOS
On 2014/03/04 16:25:22, sadrul wrote:
> I think the comment should be 'defined(OS_CHROMEOS)'
> 
> But for really small blocks like this, I don't think you need the comments at
> all (for all changes in this file).

Done.

[bshe, 2014-03-04 23:33:21]

On 2014/03/04 18:56:14, bshe wrote:
> thanks for review
> 
>
https://codereview.chromium.org/177123016/diff/60001/ash/system/chromeos/virt...
> File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right):
> 
>
https://codereview.chromium.org/177123016/diff/60001/ash/system/chromeos/virt...
> ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:42: // Overridden
> from AccessibilityObserver.
> On 2014/03/04 16:25:22, sadrul wrote:
> > The comment should be consistent, so '// AccessibilityObserver:'
> 
> Done.
> 
>
https://codereview.chromium.org/177123016/diff/60001/ash/system/status_area_w...
> File ash/system/status_area_widget.h (right):
> 
>
https://codereview.chromium.org/177123016/diff/60001/ash/system/status_area_w...
> ash/system/status_area_widget.h:25: #endif  // OS_CHROMEOS
> On 2014/03/04 16:25:22, sadrul wrote:
> > I think the comment should be 'defined(OS_CHROMEOS)'
> > 
> > But for really small blocks like this, I don't think you need the comments
at
> > all (for all changes in this file).
> 
> Done.

Hey Sadrul. Do you mind to take another look at the patchset 6. Steve gave me
two new resources and
I need to change the GetPreferredSize logic to make it match the new mock here:
https://drive.google.com/a/google.com/file/d/0B33i9-DcAoN_UUFDYnJlUXNCQlE/edi...

Thanks!

[sadrul, 2014-03-05 18:34:10]

slgtm

[bshe, 2014-03-06 00:42:13]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:45:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/177123016/100001

[commit-bot: I haz the power, 2014-03-06 02:52:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/177123016/100001

[commit-bot: I haz the power, 2014-03-06 03:17:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/177123016/100001

[commit-bot: I haz the power, 2014-03-06 10:54:21]

Change committed as 255307