mus: Introduce high-contrast mode in mus.

mus: Introduce high-contrast mode in mus.

BUG=626439
Committed: https://crrev.com/05ee8278913d7012f5216505e243d1b5b2728ba6
Cr-Commit-Position: refs/heads/master@{#404985}

[sadrul, 2016-07-08 16:22:17]

sadrul@chromium.org changed reviewers:
+ fsamuel@chromium.org, sky@chromium.org

[sadrul, 2016-07-08 16:22:19]

fsamuel@ Mind looking at the frame_generator change to see if that's the right
way of doing things?

sky@ for all of it.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/accessibility_manager.mojom (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/accessibility_manager.mojom:12:
SetHighContrastMode(bool enabled);
Would it make sense to have "enum ServiceState { ENABLE, DISABLE, TOGGLE };
SetHighContrastMode(ServiceState);" instead?

[sadrul, 2016-07-08 16:24:07]

Description was changed from

==========
mus: Introduce high-contrast mode in mus.

...
==========

to

==========
mus: Introduce high-contrast mode in mus.

BUG=626439
==========

[sky, 2016-07-08 16:45:01]

https://codereview.chromium.org/2127383003/diff/40001/ash/sysui/manifest.json
File ash/sysui/manifest.json (right):

https://codereview.chromium.org/2127383003/diff/40001/ash/sysui/manifest.json...
ash/sysui/manifest.json:7: "*": { "classes": [ "app", "a11y" ] },
nit: I realize accessibility is long to type, but I would rather we have the
whole thing written out.

https://codereview.chromium.org/2127383003/diff/40001/ash/sysui/shell_delegat...
File ash/sysui/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2127383003/diff/40001/ash/sysui/shell_delegat...
ash/sysui/shell_delegate_mus.cc:31: class AccessibilityDelegateMus : public
DefaultAccessibilityDelegate {
To further my previous nit see how accessibility is spelled out in its entirety
here and not a11y.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/manifest.json
File services/ui/manifest.json (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/manifest.js...
services/ui/manifest.json:17: "a11y": [
accessibility

https://codereview.chromium.org/2127383003/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/accessibility_manager.mojom (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/accessibility_manager.mojom:12:
SetHighContrastMode(bool enabled);
On 2016/07/08 16:22:18, sadrul wrote:
> Would it make sense to have "enum ServiceState { ENABLE, DISABLE, TOGGLE };
> SetHighContrastMode(ServiceState);" instead?

Seems like the caller is going to know what state they want, right? When would
you envision TOGGLE being used?

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
File services/ui/ws/accessibility_manager.cc (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
services/ui/ws/accessibility_manager.cc:20: if (binding_.is_bound())
nit: I'm pretty sure you can always call binding_.Close(). It use to be the case
you couldn't, but I believe 618488 fixed it.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
services/ui/ws/accessibility_manager.cc:24:
base::Bind(&mojo::Binding<mojom::AccessibilityManager>::Close,
Do you really need this? Seems like it doesn't matter if there is a connection
error.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
services/ui/ws/accessibility_manager.cc:29:
window_server_->SetHighContrastMode(enabled);
Isn't high contrast mode specific to a user and not all users?

[sadrul, 2016-07-08 17:33:25]

https://codereview.chromium.org/2127383003/diff/40001/ash/sysui/manifest.json
File ash/sysui/manifest.json (right):

https://codereview.chromium.org/2127383003/diff/40001/ash/sysui/manifest.json...
ash/sysui/manifest.json:7: "*": { "classes": [ "app", "a11y" ] },
On 2016/07/08 16:45:00, sky wrote:
> nit: I realize accessibility is long to type, but I would rather we have the
> whole thing written out.

Done.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/manifest.json
File services/ui/manifest.json (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/manifest.js...
services/ui/manifest.json:17: "a11y": [
On 2016/07/08 16:45:00, sky wrote:
> accessibility

Done.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/accessibility_manager.mojom (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/accessibility_manager.mojom:12:
SetHighContrastMode(bool enabled);
On 2016/07/08 16:45:00, sky wrote:
> On 2016/07/08 16:22:18, sadrul wrote:
> > Would it make sense to have "enum ServiceState { ENABLE, DISABLE, TOGGLE };
> > SetHighContrastMode(ServiceState);" instead?
> 
> Seems like the caller is going to know what state they want, right? When would
> you envision TOGGLE being used?

The system-tray gets an entry to toggle the flag, but it knows whether it is
turned on or not, so yeah, toggle doesn't actually need to be in the API.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
File services/ui/ws/accessibility_manager.cc (right):

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
services/ui/ws/accessibility_manager.cc:20: if (binding_.is_bound())
On 2016/07/08 16:45:00, sky wrote:
> nit: I'm pretty sure you can always call binding_.Close(). It use to be the
case
> you couldn't, but I believe 618488 fixed it.

Done.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
services/ui/ws/accessibility_manager.cc:24:
base::Bind(&mojo::Binding<mojom::AccessibilityManager>::Close,
On 2016/07/08 16:45:00, sky wrote:
> Do you really need this? Seems like it doesn't matter if there is a connection
> error.

Cool. Removed.

https://codereview.chromium.org/2127383003/diff/40001/services/ui/ws/accessib...
services/ui/ws/accessibility_manager.cc:29:
window_server_->SetHighContrastMode(enabled);
On 2016/07/08 16:45:00, sky wrote:
> Isn't high contrast mode specific to a user and not all users?

Oh yeah, good point. Fixed.

[sadrul, 2016-07-08 17:49:52]

sadrul@chromium.org changed reviewers:
+ danakj@chromium.org, kenrb@chromium.org

[sadrul, 2016-07-08 17:49:53]

+kenrb@ for the mojom change
(services/ui/public/interfaces/accessibility_manager.mojom).

+danakj@ for the code in FrameGenerator (services/ui/ws/frame_generator.cc)

[kenrb, 2016-07-08 18:15:25]

ipc lgtm

[danakj, 2016-07-08 19:59:23]

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:108: std::unique_ptr<cc::RenderPass>
invert_pass = cc::RenderPass::Create();
pls call SetNew on the pass, rather than setting a few fields. The constructor
isn't meant to be useful. SetNew is the constructor.

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:114: shared_state->SetAll(gfx::Transform(),
root->bounds().size(),
Why are you using these root bounds. Are they not in DIP? Why not use the
(previously) root renderpass' output_rect?

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:122: 0,  // mask_resource_id
if you use /* */ then it can pack parameters better

[sadrul, 2016-07-08 20:10:27]

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:108: std::unique_ptr<cc::RenderPass>
invert_pass = cc::RenderPass::Create();
On 2016/07/08 19:59:23, danakj wrote:
> pls call SetNew on the pass, rather than setting a few fields. The constructor
> isn't meant to be useful. SetNew is the constructor.

Done.

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:114: shared_state->SetAll(gfx::Transform(),
root->bounds().size(),
On 2016/07/08 19:59:23, danakj wrote:
> Why are you using these root bounds. Are they not in DIP? Why not use the
> (previously) root renderpass' output_rect?

Done.

https://codereview.chromium.org/2127383003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:122: 0,  // mask_resource_id
On 2016/07/08 19:59:23, danakj wrote:
> if you use /* */ then it can pack parameters better

Done.

[danakj, 2016-07-08 20:41:40]

That LGTM

[sky, 2016-07-08 20:53:33]

https://codereview.chromium.org/2127383003/diff/100001/ash/sysui/shell_delega...
File ash/sysui/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2127383003/diff/100001/ash/sysui/shell_delega...
ash/sysui/shell_delegate_mus.cc:31: class AccessibilityDelegateMus : public
DefaultAccessibilityDelegate {
Can this be wired up for the ash/mus side? We're trying to get rid of sysui.

[sadrul, 2016-07-08 22:20:30]

https://codereview.chromium.org/2127383003/diff/100001/ash/sysui/shell_delega...
File ash/sysui/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2127383003/diff/100001/ash/sysui/shell_delega...
ash/sysui/shell_delegate_mus.cc:31: class AccessibilityDelegateMus : public
DefaultAccessibilityDelegate {
On 2016/07/08 20:53:32, sky (OOO until 7-18) wrote:
> Can this be wired up for the ash/mus side? We're trying to get rid of sysui.

Done (removed all changes from sysui).

[sadrul, 2016-07-09 14:21:18]

sadrul@chromium.org changed reviewers:
+ ben@chromium.org, jamescook@chromium.org

[sadrul, 2016-07-09 14:21:19]

Since sky@ is OOO, +jamescook@ for ash/mus, +ben@ for services/ui changes.

[James Cook, 2016-07-10 02:31:36]

ash/mus lgtm

I'm about to head out of town, so I didn't look at the services/ui code at all.

[James Cook, 2016-07-10 02:31:37]

ash/mus lgtm

I'm about to head out of town, so I didn't look at the services/ui code at all.

[sadrul, 2016-07-12 14:42:48]

sadrul@chromium.org changed reviewers:
+ msw@chromium.org

[sadrul, 2016-07-12 14:42:49]

+msw@ too for services/ui owner (since Ben also seems ooo at the moment)

[msw, 2016-07-12 18:30:09]

Just a few minor comments/questions.

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:102: ui::mojom::AccessibilityManager*
accessibility_manager() {
nit: GetAccessibilityManager? Or ConnectToInterface separately (on Toggle)?

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/manifest.json
File ash/mus/manifest.json (right):

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/manifest.json#...
ash/mus/manifest.json:10: "*": { "classes": [ "accessibility", "app" ] },
Wouldn't it make more sense to list ui::mojom::AccessibilityManager in the
interfaces required of mojo:ui instead? Will we soon add other interfaces to the
accessibility 'class'?

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:93: cc::CompositorFrame
FrameGenerator::GenerateCompositorFrame() {
I presume danakj looked here; I don't understand this code well enough to do
more than rubber stamp your changes here. Does CreateInvertFilter really suffice
for a high contrast mode? Is that how Chrome OS currently does this? Seems odd.

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:118:
filters.Append(cc::FilterOperation::CreateInvertFilter(1.0));
nit: 1.f?

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/test_ut...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/test_ut...
services/ui/ws/test_utils.h:255: bool IsInHighContrastMode() override;
nit: order after OnCompositorFrameDrawn to match FrameGeneratorDelegate decl.

[sadrul, 2016-07-13 01:01:22]

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:102: ui::mojom::AccessibilityManager*
accessibility_manager() {
On 2016/07/12 18:30:09, msw wrote:
> nit: GetAccessibilityManager? Or ConnectToInterface separately (on Toggle)?

Done (changed to GetAccessibilityManager())

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/manifest.json
File ash/mus/manifest.json (right):

https://codereview.chromium.org/2127383003/diff/140001/ash/mus/manifest.json#...
ash/mus/manifest.json:10: "*": { "classes": [ "accessibility", "app" ] },
On 2016/07/12 18:30:09, msw wrote:
> Wouldn't it make more sense to list ui::mojom::AccessibilityManager in the
> interfaces required of mojo:ui instead? Will we soon add other interfaces to
the
> accessibility 'class'?

I expect we will add additional interfaces for accessibility. Although it's not
clear if we will connect to those via shell, or if AccessibilityManager will
provide those impls. Removed the 'accessibility' class from this CL.

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:93: cc::CompositorFrame
FrameGenerator::GenerateCompositorFrame() {
On 2016/07/12 18:30:09, msw wrote:
> I presume danakj looked here; I don't understand this code well enough to do
> more than rubber stamp your changes here. Does CreateInvertFilter really
suffice
> for a high contrast mode? Is that how Chrome OS currently does this? Seems
odd.

Yep. That's all we do (except we set the filter on a cc::Layer:
https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?sq=package:chromi...
whereas we set the filter on a cc surface)

danakj@ reviewed this change, yes.

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:118:
filters.Append(cc::FilterOperation::CreateInvertFilter(1.0));
On 2016/07/12 18:30:09, msw wrote:
> nit: 1.f?

Done.

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/test_ut...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2127383003/diff/140001/services/ui/ws/test_ut...
services/ui/ws/test_utils.h:255: bool IsInHighContrastMode() override;
On 2016/07/12 18:30:09, msw wrote:
> nit: order after OnCompositorFrameDrawn to match FrameGeneratorDelegate decl.

Done.

[msw, 2016-07-13 01:07:58]

lgtm

[sadrul, 2016-07-13 02:03:54]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-07-13 02:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
danakj@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2127383003/#ps160001
(title: ".")

[commit-bot: I haz the power, 2016-07-13 02:05:22]

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, 2016-07-13 03:52:24]

Description was changed from

==========
mus: Introduce high-contrast mode in mus.

BUG=626439
==========

to

==========
mus: Introduce high-contrast mode in mus.

BUG=626439
==========

[commit-bot: I haz the power, 2016-07-13 03:52:26]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-13 03:54:40]

Description was changed from

==========
mus: Introduce high-contrast mode in mus.

BUG=626439
==========

to

==========
mus: Introduce high-contrast mode in mus.

BUG=626439

Committed: https://crrev.com/05ee8278913d7012f5216505e243d1b5b2728ba6
Cr-Commit-Position: refs/heads/master@{#404985}
==========

[commit-bot: I haz the power, 2016-07-13 03:54:42]

Patchset 9 (id:??) landed as
https://crrev.com/05ee8278913d7012f5216505e243d1b5b2728ba6
Cr-Commit-Position: refs/heads/master@{#404985}