Pass accessible location change messages to the automation API.

Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=alokp@chromium.org

Committed: https://crrev.com/2400c46083215ca8dcaecb6393397d13eedc4e2f
Cr-Commit-Position: refs/heads/master@{#413738}

[dmazzoni, 2016-08-19 21:24:48]

Description was changed from

==========
Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
==========

to

==========
Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-08-19 21:25:04]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[dmazzoni, 2016-08-19 21:25:47]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, tsepez@chromium.org

[dmazzoni, 2016-08-19 21:25:48]

aboxhall: primary review (see comments, only two files are
nontrivial)

tsepez: owners approval for messages

[commit-bot: I haz the power, 2016-08-19 21:26:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-08-19 21:43:11]

On 2016/08/19 21:25:48, dmazzoni wrote:
> aboxhall: primary review (see comments, only two files are
> nontrivial)
> 
> tsepez: owners approval for messages

messages LGTM, and it looks like the tree_id lookups are all checked for
non-existence.

[commit-bot: I haz the power, 2016-08-19 21:58:03]

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

[commit-bot: I haz the power, 2016-08-19 21:58:04]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2016-08-22 19:48:38]

LGTM 🎉

One minor question but mostly awesome.

https://codereview.chromium.org/2255093005/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/2255093005/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:206:
for (const auto& event : details) {
yay

https://codereview.chromium.org/2255093005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2255093005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1895: // TODO(aboxhall):
send location change events to web contents observers too
yay

https://codereview.chromium.org/2255093005/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/render_accessibility_impl.cc (left):

https://codereview.chromium.org/2255093005/diff/1/content/renderer/accessibil...
content/renderer/accessibility/render_accessibility_impl.cc:398: if (iter !=
locations_.end() && iter->second != new_location) {
Why is this no longer necessary?

[dmazzoni, 2016-08-22 21:29:14]

Thanks

https://codereview.chromium.org/2255093005/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/render_accessibility_impl.cc (left):

https://codereview.chromium.org/2255093005/diff/1/content/renderer/accessibil...
content/renderer/accessibility/render_accessibility_impl.cc:398: if (iter !=
locations_.end() && iter->second != new_location) {
On 2016/08/22 at 19:48:38, aboxhall wrote:
> Why is this no longer necessary?

A few lines above, we have:

if (iter == locations_.end())
  continue;

So it doesn't seem to be necessary to check it again.

[dmazzoni, 2016-08-22 23:21:43]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:22:19]

Dry run: 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-08-22 23:59:30]

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

[commit-bot: I haz the power, 2016-08-22 23:59:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dmazzoni, 2016-08-23 07:26:55]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 07:27:09]

Dry run: 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-08-23 08:50:48]

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

[commit-bot: I haz the power, 2016-08-23 08:50:49]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-08-23 15:01:06]

Description was changed from

==========
Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=alokp@chromium.org
==========

[dmazzoni, 2016-08-23 15:02:20]

+TBR=alokp for chromecast/ - I needed to add a dependency
on //skia because I added a Skia type to
content/public/common/common_param_traits_macros.h - if this
is a bad idea we can try to fix it another way

[dmazzoni, 2016-08-23 15:02:20]

+TBR=alokp for chromecast/ - I needed to add a dependency
on //skia because I added a Skia type to
content/public/common/common_param_traits_macros.h - if this
is a bad idea we can try to fix it another way

[dmazzoni, 2016-08-23 15:02:24]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-08-23 15:02:24]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2255093005/#ps40001
(title: "Speculative fix to cast_shell")

[commit-bot: I haz the power, 2016-08-23 15:02:47]

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-08-23 15:07:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-23 15:09:32]

Description was changed from

==========
Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=alokp@chromium.org
==========

to

==========
Pass accessible location change messages to the automation API.

The only "new" code is in
RenderFrameHostImpl::OnAccessibilityLocationChanges and
AutomationInternalCustomBindings::OnAccessibilityLocationChange, and they're
very straightforward. Everything else is just plumbing to get the message
from point A to point B.

It'd be nice if we didn't require so much duplicate code to accomplish this,
but it's not clear what we could do.

BUG=638701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=alokp@chromium.org

Committed: https://crrev.com/2400c46083215ca8dcaecb6393397d13eedc4e2f
Cr-Commit-Position: refs/heads/master@{#413738}
==========

[commit-bot: I haz the power, 2016-08-23 15:09:33]

Patchset 3 (id:??) landed as
https://crrev.com/2400c46083215ca8dcaecb6393397d13eedc4e2f
Cr-Commit-Position: refs/heads/master@{#413738}