Ensure only one instance of AutomationInternalCustomBindings for background page extensions

Ensure only one instance of AutomationInternalCustomBindings for background page extensions

Previously, if an extension requesting automation has more than one page it opens in its extension process (e.g. background.html and panel.html, in ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each page).

This fix makes it so we only ever make "active" the instance that is associated with the extension's background page.

Note that as a result, any calls to chrome.automation from non-background page contexts will result in an no-op (e.g. getDesktop will not trigger the callback).

In the case that the extension has no background page, we revert to the previous behavior of having one instance per page.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we only process one event and only output one alert.
BUG=691165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[David Tseng, 2017-03-28 23:39:36]

Description was changed from

==========
Ensure there is only ever one AutomationInternalCustomBindings active per
automation extension

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from the context of
non-background page contexts will result in an no-op as well as events delivered
to the extension process.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=
==========

to

==========
Ensure there is only ever one AutomationInternalCustomBindings active per
automation extension

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from the context of
non-background page contexts will result in an no-op as well as events delivered
to the extension process.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-03-28 23:40:12]

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

[David Tseng, 2017-03-28 23:40:21]

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

[commit-bot: I haz the power, 2017-03-28 23:41:03]

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, 2017-03-29 01:04:46]

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

[commit-bot: I haz the power, 2017-03-29 01:04:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[David Tseng, 2017-03-29 01:55:38]

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

[commit-bot: I haz the power, 2017-03-29 01:56:27]

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, 2017-03-29 03:34:07]

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

[commit-bot: I haz the power, 2017-03-29 03:34:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[David Tseng, 2017-03-29 14:08:14]

Description was changed from

==========
Ensure there is only ever one AutomationInternalCustomBindings active per
automation extension

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from the context of
non-background page contexts will result in an no-op as well as events delivered
to the extension process.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure only one instance of AutomationInternalCustomBindings for background page
extensions

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from the context of
non-background page contexts will result in an no-op (e.g. getDesktop will not
trigger the callback).

In the case that the extension has no background page, we revert to the previous
behavior of having one instance per page.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=691165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-03-29 14:10:50]

Description was changed from

==========
Ensure only one instance of AutomationInternalCustomBindings for background page
extensions

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from the context of
non-background page contexts will result in an no-op (e.g. getDesktop will not
trigger the callback).

In the case that the extension has no background page, we revert to the previous
behavior of having one instance per page.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=691165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure only one instance of AutomationInternalCustomBindings for background page
extensions

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from non-background page
contexts will result in an no-op (e.g. getDesktop will not trigger the
callback).

In the case that the extension has no background page, we revert to the previous
behavior of having one instance per page.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=691165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-03-29 14:11:31]

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

[commit-bot: I haz the power, 2017-03-29 14:11:42]

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, 2017-03-29 15:13:53]

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

[commit-bot: I haz the power, 2017-03-29 15:13:55]

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

[dmazzoni, 2017-03-29 15:22:58]

lgtm

lgtm, but could you also file a bug for the
underlying issue that the bindings are being instantiated
on non-extension pages and assign to Devlin?

[David Tseng, 2017-03-29 17:50:42]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-03-29 17:50:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2782753002/#ps40001
(title: "Fix ChromeVox tests.")

[commit-bot: I haz the power, 2017-03-29 17:51:19]

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-03-29 17:54:45]

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

[commit-bot: I haz the power, 2017-03-29 17:54:47]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[chromium-reviews, 2017-03-29 18:00:04]

dmazzoni@chromium.org writes:

> lgtm
>
> lgtm, but could you also file a bug for the
> underlying issue that the bindings are being instantiated
> on non-extension pages and assign to Devlin?
>

Yes, that's an issue though not the root issue of this bug. Basically, I
think there's nothing wrong with having an instance of the bindings per
context since that's the way the DOM works (each context has a unique
instance of window). It just doesn't make sense
for automation since the bindings each get their own state which I don't
think these native bindings were meant to do. We should think about
perhaps sharing global state across all automation bindings and or
having a non-extension based ipc to shuttle data to an AXTree in the
extension process.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-03-29 18:06:16]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2017-03-29 18:06:46]

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-03-29 18:11:00]

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

[commit-bot: I haz the power, 2017-03-29 18:11:01]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[David Tseng, 2017-03-29 18:27:47]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-03-29 18:27:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2782753002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-29 18:28:54]

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-03-29 20:27:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490812067621440,
"parent_rev": "78c7038fe6f981a9045d7d32d9f531ebb098fd80", "commit_rev":
"d64d9854a1d22208db2d97b7f441696ed0ca8fa0"}

[commit-bot: I haz the power, 2017-03-29 20:28:16]

Description was changed from

==========
Ensure only one instance of AutomationInternalCustomBindings for background page
extensions

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from non-background page
contexts will result in an no-op (e.g. getDesktop will not trigger the
callback).

In the case that the extension has no background page, we revert to the previous
behavior of having one instance per page.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=691165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure only one instance of AutomationInternalCustomBindings for background page
extensions

Previously, if an extension requesting automation has more than one page it
opens in its extension process (e.g. background.html and panel.html, in
ChromeVox), we instantiated two AutomationInternalCustomBindings (one for each
page).

This fix makes it so we only ever make "active" the instance that is associated
with the extension's background page.

Note that as a result, any calls to chrome.automation from non-background page
contexts will result in an no-op (e.g. getDesktop will not trigger the
callback).

In the case that the extension has no background page, we revert to the previous
behavior of having one instance per page.

TEST=open the ChromeVox Panel, and close it again. Press caps lock. Verify we
only process one event and only output one alert.
BUG=691165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-03-29 20:28:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d64d9854a1d22208db2d97b7f441...