Support focus highlight in Android window

Support focus highlight in Android window

BUG=b/31788292
TEST=Enable focus highlight in Chrome accessibility settings, and see it
     works in Android window.

Review-Url: https://codereview.chromium.org/2559663002
Cr-Commit-Position: refs/heads/master@{#445916}
Committed: https://chromium.googlesource.com/chromium/src/+/ca05a1afa26a0fbb45063b3975f575805dbf4ab2

[yawano, 2016-12-07 07:46:47]

yawano@chromium.org changed reviewers:
+ dmazzoni@chromium.org, kinaba@chromium.org, sky@chromium.org

[yawano, 2016-12-07 07:46:47]

PTAL

sky@: components/arc/focus_highlight/DEPS: adding dependency from
components/arc/focus_highlight to ui/aura
dmazzoni@:
  - files under chrome/browser/chromeos/accessibility
  - components/arc/focus_highlight/DEPS: adding dependency from
components/arc/focus_highlight to chrome/browser/chromeos/accessibility.
kinaba@: the rest of the files (chrome/browser/chromeos/arc, components/arc).

Thank you!

[kinaba, 2016-12-07 07:54:34]

I have no time to take a look today (sorry), but one thing.

https://codereview.chromium.org/2559663002/diff/1/components/arc/focus_highli...
File components/arc/focus_highlight/DEPS (right):

https://codereview.chromium.org/2559663002/diff/1/components/arc/focus_highli...
components/arc/focus_highlight/DEPS:2: "+chrome/browser/chromeos/accessibility",
I thought it's disallowed to depend chrome/ from component/.

If the dependency is necessary, some of the code needs to reside in
chrome/browser/chromeos/arc/.

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

yawano@chromium.org changed reviewers:
- sky@chromium.org

[yawano, 2016-12-07 09:18:31]

Moving sky@ from reviewer to CC as we no longer add dependency to ui/aura in
this CL.

Moved focus_highlight from components/arc to chrome/browser/chromeos/arc.

https://codereview.chromium.org/2559663002/diff/1/components/arc/focus_highli...
File components/arc/focus_highlight/DEPS (right):

https://codereview.chromium.org/2559663002/diff/1/components/arc/focus_highli...
components/arc/focus_highlight/DEPS:2: "+chrome/browser/chromeos/accessibility",
I've moved focus_highlight from components/arc to chrome/browser/chromeos/arc as
we are doing for c/b/chromeos/arc/tts. I believe codes under focus_highlight
should be small enough as it can be under c/b/chromeos/arc.

[stevemonjohn, 2016-12-07 09:20:09]

it is very helpful website.The chrome browser easy to browsing.thank you for
sharing this information.This is my first time i visit here. I exposed such a
variety of charming stuff in your blog Truly its
strange piece of writing
http://www.similima.com/

[Luis Héctor Chávez, 2016-12-07 17:58:56]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2016-12-07 17:58:57]

drive-by

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc:32:
void ArcFocusHighlightBridge::OnInstanceClosed() {}
You don't seem to need this. Let's remove it.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc:45: if
(!focused_window) {
nit: elide the braces.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h:25: //
InstanceHolder::Observer::OnInstanceReady overrides.
nit:

// InstanceHolder<mojom::FocusHighlightInstance>::Observer overrides:
void OnInstanceReady() override;
void OnInstanceClosed() override;

you just need to list the name of the interface, not the individual methods you
are overriding.

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
Can you think of anything else you might want to have this service do? Maybe it
would be helpful to have an accessibility bridge instead of just sending focus
highlights.

[dmazzoni, 2016-12-07 20:23:52]

lgtm for chrome/browser/chromeos/accessibility

[kinaba, 2016-12-08 04:07:44]

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc:52:
bounds_in_screen.Offset(window_bounds.x(), window_bounds.y());
Does this work as expected on Samus (a device with non-1 scale factor)?

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
On 2016/12/07 17:58:56, Luis Héctor Chávez wrote:
> Can you think of anything else you might want to have this service do? Maybe
it
> would be helpful to have an accessibility bridge instead of just sending focus
> highlights.

+1

[yawano, 2016-12-08 09:08:38]

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
No, I don't come up with other things which we want to bridge about
accessibility at this time. I think other things would be sent in different
channels. Accessibility setting sync would be done via intent helper bridge,
spoken feedback would be done via Android accessibility API.

[hidehiko, 2016-12-08 14:54:58]

hidehiko@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hidehiko, 2016-12-08 14:54:59]

yet another drive-by.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h:31:
void OnViewFocused(const gfx::Rect& rect) override;
nit: "// mojom::FocusHighlightHost" too?

https://codereview.chromium.org/2559663002/diff/20001/components/arc/arc_brid...
File components/arc/arc_bridge_host_impl.cc (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/arc_brid...
components/arc/arc_bridge_host_impl.cc:133:
OnInstanceReady(ArcBridgeService::Get()->focus_highlight(),
I landed a CL to remove ArcBridgeService::Get() from this file, and the function
is being removed.
Could you rebase and follow the new way?

[Luis Héctor Chávez, 2016-12-08 17:16:41]

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
On 2016/12/08 09:08:38, yawano wrote:
> No, I don't come up with other things which we want to bridge about
> accessibility at this time. I think other things would be sent in different
> channels. Accessibility setting sync would be done via intent helper bridge,
> spoken feedback would be done via Android accessibility API.

The thing is that spinning off new channels does add a bit of overhead, let
alone increases cognitive burden to figure out where things live.

Android's android.accessibilityservice.AccessibilityService exposes a lot more
interesting stuff. Would any of that be useful in the future and be a good
candidate of including in this interface? Would it hurt to officially make this
interface a little broader so we can plumb other AccessibilityService stuff in
the future without adding too much overhead?

[yawano, 2016-12-09 09:21:58]

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
One thing might be braille output (https://crbug.com/635579). If we compose
braille output in Android, we will need to send it from Android to Chrome.

Is it difficult to rename .mojom file later? Is this just naming issue as we can
add more methods later? If so, I'm comfortable to name this as
accessibility_helper.mojom or like that.

[Luis Héctor Chávez, 2016-12-09 16:25:33]

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
On 2016/12/09 09:21:58, yawano wrote:
> One thing might be braille output (https://crbug.com/635579). If we compose
> braille output in Android, we will need to send it from Android to Chrome.
> 
> Is it difficult to rename .mojom file later? Is this just naming issue as we
can
> add more methods later? If so, I'm comfortable to name this as
> accessibility_helper.mojom or like that.

Renaming has a bit more overhead (since you effectively need to go through all
the files you just modified right now and do it all over again, in both Android
and Chrome), but it is possible. We can definitely add more methods later (look
at all the [MinVersion] tags in all the other .mojom files).

[hidehiko, 2016-12-12 01:58:30]

https://codereview.chromium.org/2559663002/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_host_impl.cc (right):

https://codereview.chromium.org/2559663002/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_host_impl.cc:135:
OnInstanceReady(ArcBridgeService::Get()->focus_highlight(),
ArcBridgeService::Get() is being removed very soon. So please use
arc_bridge_service_ member instead.

[yawano, 2017-01-11 10:31:33]

Thank you for the review! PTAL.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc:32:
void ArcFocusHighlightBridge::OnInstanceClosed() {}
On 2016/12/07 17:58:56, Luis Héctor Chávez wrote:
> You don't seem to need this. Let's remove it.

Done.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.cc:45: if
(!focused_window) {
On 2016/12/07 17:58:56, Luis Héctor Chávez wrote:
> nit: elide the braces.

Done.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h:25: //
InstanceHolder::Observer::OnInstanceReady overrides.
On 2016/12/07 17:58:56, Luis Héctor Chávez wrote:
> nit:
> 
> // InstanceHolder<mojom::FocusHighlightInstance>::Observer overrides:
> void OnInstanceReady() override;
> void OnInstanceClosed() override;
> 
> you just need to list the name of the interface, not the individual methods
you
> are overriding.

Done.

https://codereview.chromium.org/2559663002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/focus_highlight/arc_focus_highlight_bridge.h:31:
void OnViewFocused(const gfx::Rect& rect) override;
On 2016/12/08 14:54:59, hidehiko wrote:
> nit: "// mojom::FocusHighlightHost" too?

Done.

https://codereview.chromium.org/2559663002/diff/20001/components/arc/arc_brid...
File components/arc/arc_bridge_host_impl.cc (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/arc_brid...
components/arc/arc_bridge_host_impl.cc:133:
OnInstanceReady(ArcBridgeService::Get()->focus_highlight(),
On 2016/12/08 14:54:59, hidehiko wrote:
> I landed a CL to remove ArcBridgeService::Get() from this file, and the
function
> is being removed.
> Could you rebase and follow the new way?

Done.

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
File components/arc/common/focus_highlight.mojom (right):

https://codereview.chromium.org/2559663002/diff/20001/components/arc/common/f...
components/arc/common/focus_highlight.mojom:17: interface FocusHighlightInstance
{
On 2016/12/09 16:25:33, Luis Héctor Chávez wrote:
> On 2016/12/09 09:21:58, yawano wrote:
> > One thing might be braille output (https://crbug.com/635579). If we compose
> > braille output in Android, we will need to send it from Android to Chrome.
> > 
> > Is it difficult to rename .mojom file later? Is this just naming issue as we
> can
> > add more methods later? If so, I'm comfortable to name this as
> > accessibility_helper.mojom or like that.
> 
> Renaming has a bit more overhead (since you effectively need to go through all
> the files you just modified right now and do it all over again, in both
Android
> and Chrome), but it is possible. We can definitely add more methods later
(look
> at all the [MinVersion] tags in all the other .mojom files).

Renamed to AccessibilityHelper.

https://codereview.chromium.org/2559663002/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_host_impl.cc (right):

https://codereview.chromium.org/2559663002/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_host_impl.cc:135:
OnInstanceReady(ArcBridgeService::Get()->focus_highlight(),
On 2016/12/12 01:58:30, hidehiko wrote:
> ArcBridgeService::Get() is being removed very soon. So please use
> arc_bridge_service_ member instead.

Done.

[David Tseng, 2017-01-11 19:26:52]

dtseng@chromium.org changed reviewers:
+ dtseng@chromium.org

[David Tseng, 2017-01-11 19:26:54]

https://codereview.chromium.org/2559663002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc
(right):

https://codereview.chromium.org/2559663002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc:12:
#include "ui/gfx/geometry/rect.h"
nit: remove; already included in header

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: OnViewFocused@0(ScreenRect
rect);
Just FYI. We will eventually want to pass through not only the rectangle of
focus events, but all of the data contained in theAccessibilityNodeInfo on the
Android side for all event types.

With that in mind, would you mind making this into
OnAccessibilityEvent(EventType type, AccessibilityNodeInfoData data);

where
EventType is an enum of all Android events (you can just put focus at this
point)
and
AccessibilityNodeInfoData is a struct containing data about the
AccessibilityNodeInfo
(you can just put 
boundsInScreen
 for now).
https://developer.android.com/reference/android/view/accessibility/Accessibil...

[David Tseng, 2017-01-11 19:34:55]

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: OnViewFocused@0(ScreenRect
rect);
On 2017/01/11 19:26:53, David Tseng wrote:
> Just FYI. We will eventually want to pass through not only the rectangle of
> focus events, but all of the data contained in theAccessibilityNodeInfo on the
> Android side for all event types.
> 
> With that in mind, would you mind making this into
> OnAccessibilityEvent(EventType type, AccessibilityNodeInfoData data);
> 
> where
> EventType is an enum of all Android events (you can just put focus at this
> point)
> and
> AccessibilityNodeInfoData is a struct containing data about the
> AccessibilityNodeInfo
> (you can just put 
> boundsInScreen
>  for now).
>
https://developer.android.com/reference/android/view/accessibility/Accessibil...
> 

Quick correction. We want an array<AccessibilityNodeInfoData> above. It will be
a serialized collection of data.

[yawano, 2017-01-17 02:29:21]

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: OnViewFocused@0(ScreenRect
rect);
Sorry, why we need to send it as an array? I thought the accessibility node info
data in the second parameter is about event source.

Is it for send accessibility node info tree of a screen? If so, I prefer to add
another method for it as we don't always need accessibility node info tree.

[David Tseng, 2017-01-17 18:46:32]

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: OnViewFocused@0(ScreenRect
rect);
On 2017/01/17 02:29:21, yawano wrote:
> Sorry, why we need to send it as an array? I thought the accessibility node
info
> data in the second parameter is about event source.
> 
> Is it for send accessibility node info tree of a screen? If so, I prefer to
add
> another method for it as we don't always need accessibility node info tree.

It would be nice to have just one api.

Yes, an array is needed because it will be used for serialization of the tree.
It also wouldn't prevent us from simply placing one node in the array whenever
focus highlight is on as opposed to ChromeVox.

However, this seems fine for now and I can make any necessary changes.

[yawano, 2017-01-18 01:33:38]

Uploaded a new patch set, and we've changed a lot from the previous review
especially about interfaces. PTAL again. Thank you!

kinaba@: components/arc, chrome/browser/chromeos/arc, and
chrome/browser/chromeos/BUILD.gn
dmazzoni@: PTAL c/b/chromeos/accessibility again. We haven't changed much in the
folder, but just to confirm whether you are okay with implementing focus
highlight with the new interface.

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: OnViewFocused@0(ScreenRect
rect);
I feel it costly to serialize accessibility node info tree every time an event
has dispatched.

Yes, using array doesn't mean that we always need to pass node info tree and we
can pass single node info, but it would be confusing as sometimes tree is passed
and sometimes not.

Anyway, let's review this CL with current interface, the single accessibility
node info version. We can change this later if we need to pass accessibility
node info tree in onAccessibilityEvent.

[David Tseng, 2017-01-18 18:54:09]

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/120001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: OnViewFocused@0(ScreenRect
rect);
On 2017/01/18 01:33:38, yawano wrote:
> I feel it costly to serialize accessibility node info tree every time an event
> has dispatched.

Yes clearly we'd prefer to send over diffs of the tree minimizing the ipc call
-- which I think would be the bulk of the cost; also note that the framework
already serializes the ANI tree to send to the accessibility service. However,
serialization of the whole tree would only be for the initial implementation
(see the design doc I sent that discusses optimizations).

> 
> Yes, using array doesn't mean that we always need to pass node info tree and
we
> can pass single node info, but it would be confusing as sometimes tree is
passed
> and sometimes not.

Could be a root with one child or just the ancestry chain of the focused item.
Depending on the client, we would / can prune unneeded nodes. At this point
though, I'd like to see something working before trying to tackle optimizing.
Hope you agree with this especially since ChromeVox is the primary consumer of
focus highlight anyway.

> 
> Anyway, let's review this CL with current interface, the single accessibility
> node info version. We can change this later if we need to pass accessibility
> node info tree in onAccessibilityEvent.

Agreed; let's land this cl first and I'll make the above changes and yes it will
be needed. Thanks for the preemptive changes!

[kinaba, 2017-01-19 03:50:03]

lgtm

https://codereview.chromium.org/2559663002/diff/200001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017? (same for other new files)

https://codereview.chromium.org/2559663002/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:11: // AccessibilityEventType
is a enum which corresponds to event types of AccessibilityEvent in
I'm not sure about the coding style for mojom so it may be ok as-is,
but how about splitting the lines with 80-char width?
(To make it look better on codereview?)

[yawano, 2017-01-19 06:30:35]

https://codereview.chromium.org/2559663002/diff/200001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/19 03:50:02, kinaba wrote:
> 2017? (same for other new files)

Done.

https://codereview.chromium.org/2559663002/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:11: // AccessibilityEventType
is a enum which corresponds to event types of AccessibilityEvent in
On 2017/01/19 03:50:02, kinaba wrote:
> I'm not sure about the coding style for mojom so it may be ok as-is,
> but how about splitting the lines with 80-char width?
> (To make it look better on codereview?)

Done.

Also fixed other lines in this file as they wrap in 80 characters.

[Luis Héctor Chávez, 2017-01-19 17:17:56]

lgtm with a few nits.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc
(right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc:12:
#include "ui/gfx/geometry/rect.h"
nit: Remove

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:85: #include
"ui/gfx/geometry/rect.h"
nit: not needed. This file only deals with references and can depend on either
an #include or fwd-declaration from the .h

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.h:28: #include
"ui/gfx/geometry/rect.h"
nit: It's better if you remove this and fwd-declare gfx::Rect since you only use
references in this file.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc:55:
gfx::Rect bounds_in_screen = gfx::ScaleToEnclosingRect(
nit: #include "ui/gfx/geometry/rect.h"

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc:57:
1 / toplevel_window->layer()->device_scale_factor());
nit: maybe 1.0f to avoid the implicit conversion?

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h:28:
// mojom::AccessibilityHelperHost
nit: be consistent with the override comment style with L25.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:199:
AddPrefToObserve(prefs::kAccessibilityFocusHighlightEnabled);
nit: Can you sort these lexicographically?

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:240:
SyncFocusHighlightEnabled();
nit: sort lexicographically

https://codereview.chromium.org/2559663002/diff/220001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/220001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: [extensible]
nit: [Extensible]

[yawano, 2017-01-20 06:52:31]

Thank you for the review!

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc
(right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc:12:
#include "ui/gfx/geometry/rect.h"
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: Remove

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:85: #include
"ui/gfx/geometry/rect.h"
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: not needed. This file only deals with references and can depend on either
> an #include or fwd-declaration from the .h

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.h:28: #include
"ui/gfx/geometry/rect.h"
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: It's better if you remove this and fwd-declare gfx::Rect since you only
use
> references in this file.

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc:55:
gfx::Rect bounds_in_screen = gfx::ScaleToEnclosingRect(
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: #include "ui/gfx/geometry/rect.h"

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc:57:
1 / toplevel_window->layer()->device_scale_factor());
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: maybe 1.0f to avoid the implicit conversion?

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h:28:
// mojom::AccessibilityHelperHost
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: be consistent with the override comment style with L25.

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right):

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:199:
AddPrefToObserve(prefs::kAccessibilityFocusHighlightEnabled);
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: Can you sort these lexicographically?

Done.

https://codereview.chromium.org/2559663002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:240:
SyncFocusHighlightEnabled();
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: sort lexicographically

Done.

https://codereview.chromium.org/2559663002/diff/220001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/220001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:13: [extensible]
On 2017/01/19 17:17:56, Luis Héctor Chávez wrote:
> nit: [Extensible]

Done.

[yawano, 2017-01-20 07:11:45]

Uploaded a new patch set to remove blank lines around the forward declaration.

[hidehiko, 2017-01-20 13:24:15]

lgtm with more minor review comments. All of mine are optional.

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/BUILD.gn:211:
"arc/accessibility_helper/arc_accessibility_helper_bridge.cc",
For consistency with c/b/c/accessibility, maybe
arc/accessibility/arc_accessibility_helper_bridge?

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h
(right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h:39: void
OnViewFocusedInArc(const gfx::Rect& bounds);
Comment when this is actually called? and/or what's the coordinate system of
|bounds| etc.?

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.h:258: void
OnViewFocusedInArc(const gfx::Rect& bounds);
Similary, what's |bounds|'s coordinate system etc.?

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc:18:
bridge_service->accessibility_helper()->AddObserver(this);
Totally nit: for consistency, s/bridge_service/arc_bridge_service()/

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h:17:
class ArcAccessibilityHelperBridge
Document what is the responsibility of this class?

https://codereview.chromium.org/2559663002/diff/280001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/280001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:26:
OnAccessibilityEvent@0(AccessibilityEventType eventType,
This is an interface between ARC and Chrome. So it is more important to
explicitly add documents.
Could you document when this is called (even if it looks clear, at the moment?).

[yawano, 2017-01-23 09:04:52]

kinaba@: PTAL. Addressed comments from hidehiko@ and changed to send bounds in
screen coordinate.

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/BUILD.gn:211:
"arc/accessibility_helper/arc_accessibility_helper_bridge.cc",
On 2017/01/20 13:24:15, hidehiko wrote:
> For consistency with c/b/c/accessibility, maybe
> arc/accessibility/arc_accessibility_helper_bridge?

Done.

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h
(right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h:39: void
OnViewFocusedInArc(const gfx::Rect& bounds);
On 2017/01/20 13:24:15, hidehiko wrote:
> Comment when this is actually called? and/or what's the coordinate system of
> |bounds| etc.?

Done.

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.h:258: void
OnViewFocusedInArc(const gfx::Rect& bounds);
On 2017/01/20 13:24:15, hidehiko wrote:
> Similary, what's |bounds|'s coordinate system etc.?

Done.

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.cc:18:
bridge_service->accessibility_helper()->AddObserver(this);
On 2017/01/20 13:24:15, hidehiko wrote:
> Totally nit: for consistency, s/bridge_service/arc_bridge_service()/

Done.

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h
(right):

https://codereview.chromium.org/2559663002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility_helper/arc_accessibility_helper_bridge.h:17:
class ArcAccessibilityHelperBridge
On 2017/01/20 13:24:15, hidehiko wrote:
> Document what is the responsibility of this class?

Done.

https://codereview.chromium.org/2559663002/diff/280001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/280001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:26:
OnAccessibilityEvent@0(AccessibilityEventType eventType,
On 2017/01/20 13:24:15, hidehiko wrote:
> This is an interface between ARC and Chrome. So it is more important to
> explicitly add documents.
> Could you document when this is called (even if it looks clear, at the
moment?).

Done.

[yawano, 2017-01-23 09:08:16]

Patch Set 19 is to fix a comment in accessibility_helper.mojom file.

[dmazzoni, 2017-01-24 03:31:51]

lgtm

The changes to make the mojo interface more general look great.

[yawano, 2017-01-24 03:49:56]

Thank you for your review!

[kinaba, 2017-01-24 06:27:43]

lgtm

[yawano, 2017-01-24 06:28:30]

On 2017/01/24 06:27:43, kinaba wrote:
> lgtm

Thank you!

[yawano, 2017-01-24 06:28:54]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-01-24 06:28:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2559663002/#ps360001
(title: "Fix a comment.")

[commit-bot: I haz the power, 2017-01-24 06:29:12]

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-01-24 06:36:48]

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

[commit-bot: I haz the power, 2017-01-24 06:36:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yawano, 2017-01-24 06:50:55]

yawano@chromium.org changed reviewers:
+ dcheng@chromium.org

[yawano, 2017-01-24 06:50:56]

dcheng@: PTAL components/arc/common/*.mojom files. Thank you!

[dcheng, 2017-01-24 09:36:57]

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h
(right):

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h:44: void
OnViewFocusedInArc(const gfx::Rect& boundsInScreen);
Nit: name_like_this, here and elsewhere in this CL, rather than lowerCamelCase.

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:36:
eventSource.is_null()) {
Is |event_source| actually nullable? The IDL doesn't define as optional.

https://codereview.chromium.org/2559663002/diff/360001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/360001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:19: // AccessibilityNodeInfo in
Android.
Nit: I'm not sure how stable Android documentation is, but it might be nice to
link to it for this (and the enum above)

[yawano, 2017-01-24 11:18:09]

Thank you for your review! PTAL.

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h
(right):

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.h:44: void
OnViewFocusedInArc(const gfx::Rect& boundsInScreen);
On 2017/01/24 09:36:56, dcheng wrote:
> Nit: name_like_this, here and elsewhere in this CL, rather than
lowerCamelCase.

Done. Thank you for catching this!

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2559663002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:36:
eventSource.is_null()) {
Yes, this can be nullable. For example TYPE_WINDOWS_CHANGED accessibility event
doesn't provide getSource(). (See TYPE_WINDOWS_CHANGED section in
https://developer.android.com/reference/android/view/accessibility/Accessibil...)
Updated the mojom file.

https://codereview.chromium.org/2559663002/diff/360001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2559663002/diff/360001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:19: // AccessibilityNodeInfo in
Android.
On 2017/01/24 09:36:56, dcheng wrote:
> Nit: I'm not sure how stable Android documentation is, but it might be nice to
> link to it for this (and the enum above)

Done.

[dcheng, 2017-01-24 16:23:18]

lgtm

[yawano, 2017-01-25 01:21:10]

Thank you!

[yawano, 2017-01-25 01:21:17]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-01-25 01:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, dmazzoni@chromium.org, hidehiko@chromium.org,
kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/2559663002/#ps380001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-01-25 01:21:53]

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-01-25 02:38:25]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1485307277551960,
"parent_rev": "7bba05ed69f2a2fdf3aca587dec0c9f148bdc469", "commit_rev":
"ca05a1afa26a0fbb45063b3975f575805dbf4ab2"}

[commit-bot: I haz the power, 2017-01-25 02:39:36]

Description was changed from

==========
Support focus highlight in Android window

BUG=b/31788292
TEST=Enable focus highlight in Chrome accessibility settings, and see it
     works in Android window.
==========

to

==========
Support focus highlight in Android window

BUG=b/31788292
TEST=Enable focus highlight in Chrome accessibility settings, and see it
     works in Android window.

Review-Url: https://codereview.chromium.org/2559663002
Cr-Commit-Position: refs/heads/master@{#445916}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca05a1afa26a0fbb45063b3975f5...
==========

[commit-bot: I haz the power, 2017-01-25 02:39:38]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/ca05a1afa26a0fbb45063b3975f5...

[johnhonay93, 2017-03-22 06:52:49]

Here include the program is based on the overriding the data based program.this
program is very simple and easy to understand all.this program is very useful
for me.i really like to visit this site as well as read this
program.http://campextremezone.com/outing-trips/

[trackermaster6, 2017-03-22 11:54:12]

The information about the array is help me to solve some errors in my website-
https://www.vouchermedia.com/  .Yes you are right the array is used for the
serialization of tree.

[johnhonay93, 2017-03-22 12:01:53]

On 2017/03/22 06:52:49, johnhonay93 wrote:
> Here include the program is based on the overriding the data based
program.this
> program is very simple and easy to understand all.this program is very useful
> for me.i really like to visit this site as well as read this
> program. http://campextremezone.com/outing-trips/

[bubbleray30, 2017-03-23 04:39:02]

This is an nice site all of them can see it and use it very effectively all the
age groups can understand it. http://supremepackaginguae.com/

[stevemonjohn, 2017-03-23 05:26:33]

This article is related to technical field but i from medical field like this
article so much.Technical field has very good oppurtunities in every
sector.http://supremepackaginguae.com/

[stevemonjohn, 2017-03-23 05:29:23]

On 2017/03/23 05:26:33, stevemonjohn wrote:
> This article is related to technical field but i from medical field like this
> article so much.Technical field has very good oppurtunities in every
> sector. www.similima.com/tag/psc-homoeopathy-question-paper/

[bubbleray92, 2017-03-23 06:00:48]

Every field needs computer and the programming part is very very
important.Computer science has great scope in coming years and it is growing in
the last few years.codereview is a very nice site and i am most interested in
it. http://coloursproductions.com/

[stuartlittlemarch, 2017-03-23 06:16:05]

IT field is a very good and most job providing site in the World.Without
computer no one can do anything,it is a very nice article and the codereview is
a very nice site. http://keralatravelcabs.com/