Implement High Contrast mode for Chrome OS

Implement High Contrast mode for Chrome OS.

-Add controller to ash for managing the state of High Contrast mode
-Add API to shell to allow Chromium to turn the mode on and off
-Add the option to the Chromium OS accessibility settings page.
-Add functions to layer.cc/h to enable an inversion filter.

BUG=chromium-os:30678
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138212

[Zachary Kuznia, 2012-05-08 08:31:33]

Hi Dominic,

This patch enables High contrast mode for Chrome OS.  Please have a look at:
https://chromiumcodereview.appspot.com/10201014

Thanks,
-Zach

[dmazzoni, 2012-05-16 04:44:17]

lgtm

Looks good! At some point I'd like to modify it to do a more sophisticated
mapping than just inverting (e.g. gamma-aware inversion, increase contrast), but
this is a good first step.

http://codereview.chromium.org/10201014/diff/1005/ash/high_contrast/high_cont...
File ash/high_contrast/high_contrast_controller.h (right):

http://codereview.chromium.org/10201014/diff/1005/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:29: int enable_;
int -> bool

[Zachary Kuznia, 2012-05-16 08:49:28]

http://codereview.chromium.org/10201014/diff/1005/ash/high_contrast/high_cont...
File ash/high_contrast/high_contrast_controller.h (right):

http://codereview.chromium.org/10201014/diff/1005/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:29: int enable_;
On 2012/05/16 04:44:17, Dominic Mazzoni wrote:
> int -> bool

Done.

[Zachary Kuznia, 2012-05-16 08:50:16]

Hello sky,

Could you review this at:
http://codereview.chromium.org/10201014

Thanks,
-Zach

[sky, 2012-05-16 21:07:48]

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
File ash/high_contrast/high_contrast_controller.h (right):

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:22: virtual
~HighContrastController() {}
Does this really need to be virtual?

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:24: void SetEnable(bool enable);
SetEnabled

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:29: bool enable_;
enabled

http://codereview.chromium.org/10201014/diff/5006/ash/shell_delegate.h
File ash/shell_delegate.h (right):

http://codereview.chromium.org/10201014/diff/5006/ash/shell_delegate.h#newcode92
ash/shell_delegate.h:92: virtual bool IsHighContrastEnabled() = 0;
Do we really need this? Can't we set it when chrome creates the shell?

http://codereview.chromium.org/10201014/diff/5006/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/accessibility_util.cc (right):

http://codereview.chromium.org/10201014/diff/5006/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/accessibility_util.cc:169:
ash::Shell::GetInstance()->high_contrast_controller()->SetEnable(enabled);
Since you're exposing HighContrastController outside of shell it shouldn't be in
the internal namespace.

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:210: {
{ on previous line

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.h
File ui/compositor/layer.h (right):

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.h#newco...
ui/compositor/layer.h:139: void SetInvert(bool invert);
SetInverted

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.h#newco...
ui/compositor/layer.h:295: bool invert_;
inverted

[Zachary Kuznia, 2012-05-17 08:25:25]

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
File ash/high_contrast/high_contrast_controller.h (right):

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:22: virtual
~HighContrastController() {}
On 2012/05/16 21:07:48, sky wrote:
> Does this really need to be virtual?

Done.

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:24: void SetEnable(bool enable);
On 2012/05/16 21:07:48, sky wrote:
> SetEnabled

Done.

http://codereview.chromium.org/10201014/diff/5006/ash/high_contrast/high_cont...
ash/high_contrast/high_contrast_controller.h:29: bool enable_;
On 2012/05/16 21:07:48, sky wrote:
> enabled

Done.

http://codereview.chromium.org/10201014/diff/5006/ash/shell_delegate.h
File ash/shell_delegate.h (right):

http://codereview.chromium.org/10201014/diff/5006/ash/shell_delegate.h#newcode92
ash/shell_delegate.h:92: virtual bool IsHighContrastEnabled() = 0;
On 2012/05/16 21:07:48, sky wrote:
> Do we really need this? Can't we set it when chrome creates the shell?

Removed.

http://codereview.chromium.org/10201014/diff/5006/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/accessibility_util.cc (right):

http://codereview.chromium.org/10201014/diff/5006/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/accessibility_util.cc:169:
ash::Shell::GetInstance()->high_contrast_controller()->SetEnable(enabled);
On 2012/05/16 21:07:48, sky wrote:
> Since you're exposing HighContrastController outside of shell it shouldn't be
in
> the internal namespace. 

Done.

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:210: {
On 2012/05/16 21:07:48, sky wrote:
> { on previous line

Done.

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.h
File ui/compositor/layer.h (right):

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.h#newco...
ui/compositor/layer.h:139: void SetInvert(bool invert);
On 2012/05/16 21:07:48, sky wrote:
> SetInverted

Done.

http://codereview.chromium.org/10201014/diff/5006/ui/compositor/layer.h#newco...
ui/compositor/layer.h:295: bool invert_;
On 2012/05/16 21:07:48, sky wrote:
> inverted

Done.

[Zachary Kuznia, 2012-05-17 08:26:21]

Hello jhawkins,

Could you do an OWNERS review on:
chrome/browser/resources/options2/*

Thanks,
-Zach

[sky, 2012-05-17 15:54:14]

LGTM with the following changes.

http://codereview.chromium.org/10201014/diff/11005/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10201014/diff/11005/ash/shell.cc#newcode750
ash/shell.cc:750:
//high_contrast_controller_->SetEnabled(delegate_->IsHighContrastEnabled());
Remove this

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_extra_parts_ash.cc (right):

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_extra_parts_ash.cc:81: if
(chromeos::accessibility::IsHighContrastEnabled())
Can you remove the if, so that it's:

ash::Shell::GetInstance()->...->SetEnabled(chromeos::accessibility::IsHighContrastEnabled());

[James Hawkins, 2012-05-17 16:07:37]

LGTM with nit.

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.js:472:
[$('accessibility-high-contrast-check').checked]);
nit: The parameter indentation on this line and line 467 are wrong.  The start
of parameter rows should line up on the same column.  That's probably not
possible here so 'highContrast...', will need to be dropped a line and indented.

[Zachary Kuznia, 2012-05-21 05:50:04]

http://codereview.chromium.org/10201014/diff/11005/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10201014/diff/11005/ash/shell.cc#newcode750
ash/shell.cc:750:
//high_contrast_controller_->SetEnabled(delegate_->IsHighContrastEnabled());
On 2012/05/17 15:54:15, sky wrote:
> Remove this

Done.

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_extra_parts_ash.cc (right):

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_extra_parts_ash.cc:81: if
(chromeos::accessibility::IsHighContrastEnabled())
On 2012/05/17 15:54:15, sky wrote:
> Can you remove the if, so that it's:
> 
>
ash::Shell::GetInstance()->...->SetEnabled(chromeos::accessibility::IsHighContrastEnabled());

Done.

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10201014/diff/11005/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.js:472:
[$('accessibility-high-contrast-check').checked]);
On 2012/05/17 16:07:37, James Hawkins wrote:
> nit: The parameter indentation on this line and line 467 are wrong.  The start
> of parameter rows should line up on the same column.  That's probably not
> possible here so 'highContrast...', will need to be dropped a line and
indented.

Done.

[commit-bot: I haz the power, 2012-05-21 11:15:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10201014/11010

[commit-bot: I haz the power, 2012-05-21 14:00:11]

Try job failure for 10201014-11010 (retry) on win_rel for steps "base_unittests,
sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-21 14:11:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10201014/11010

[commit-bot: I haz the power, 2012-05-21 14:13:47]

Try job failure for 10201014-11010 on mac_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-21 14:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10201014/11010

[commit-bot: I haz the power, 2012-05-21 15:51:02]

Try job failure for 10201014-11010 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-21 21:32:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10201014/11010

[commit-bot: I haz the power, 2012-05-21 23:50:55]

Try job failure for 10201014-11010 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-21 23:56:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10201014/11010

[commit-bot: I haz the power, 2012-05-22 02:49:04]

Try job failure for 10201014-11010 (retry) on win_rel for steps "base_unittests,
sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 03:58:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10201014/11010