Refactor event handler classes to make it easier to add new event type listeners.

Refactor event handler classes to make it easier to add new event type listeners.

A new event type, activedescendantchanged, was added.

Moving forward, it will be simpler to separate handlers (which contain groups of events), into separate classes. BaseAutomationHandler will provide basic utilities that are useful when daeling with a group of extension-styled event listeners (e.g. adding/removing, binding, etc).
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4b26b22de82af571edd4176547abead9f95ae200
Cr-Commit-Position: refs/heads/master@{#391395}

[David Tseng, 2016-05-02 22:07:43]

Description was changed from

==========
Refactor event handler classes to make it easier to add new event type
listeners.
==========

to

==========
Refactor event handler classes to make it easier to add new event type
listeners.

A new event type, activedescendantchanged, was added.

Moving forward, it will be simpler to separate handlers (which contain groups of
events), into separate classes. BaseAutomationHandler will provide basic
utilities that are useful when daeling with a group of extension-styled event
listeners (e.g. adding/removing, binding, etc).
==========

[David Tseng, 2016-05-02 22:07:43]

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

[dmazzoni, 2016-05-02 22:15:48]

lgtm

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_object_installer.js
(right):

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_object_installer.js:13:
goog.provide('AutomationObjectInstaller');
Nice hack! How about something like AutomationObjectConstructorInstaller?

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_object_installer.js:16:
* installs the AutomationNode and AutomationEvent classes based on an
nit: capital I at start of sentence

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:67:
new chrome.automation.AutomationEvent(EventType.loadComplete, root));
Is there a possible race condition if the chrome.automation.AutomationEvent
installer hasn't succeeded yet? Maybe check that
chrome.automation.AutomationEvent exists first?

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:73:
this.onFocus(new chrome.automation.AutomationEvent(EventType.focus, focus));
nit: indentation

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:76:
AutomationObjectInstaller.init(node);
Perhaps this init should just take a callback argument and that can trigger
actually doing the rest of the registration.

[David Tseng, 2016-05-03 20:18:27]

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_object_installer.js
(right):

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_object_installer.js:13:
goog.provide('AutomationObjectInstaller');
On 2016/05/02 22:15:48, dmazzoni wrote:
> Nice hack! How about something like AutomationObjectConstructorInstaller?

Done.

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_object_installer.js:16:
* installs the AutomationNode and AutomationEvent classes based on an
On 2016/05/02 22:15:48, dmazzoni wrote:
> nit: capital I at start of sentence

Done.

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:67:
new chrome.automation.AutomationEvent(EventType.loadComplete, root));
On 2016/05/02 22:15:48, dmazzoni wrote:
> Is there a possible race condition if the chrome.automation.AutomationEvent
> installer hasn't succeeded yet? Maybe check that
> chrome.automation.AutomationEvent exists first?
> 

Went with a callback as suggested below.

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:73:
this.onFocus(new chrome.automation.AutomationEvent(EventType.focus, focus));
On 2016/05/02 22:15:48, dmazzoni wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1942683005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:76:
AutomationObjectInstaller.init(node);
On 2016/05/02 22:15:48, dmazzoni wrote:
> Perhaps this init should just take a callback argument and that can trigger
> actually doing the rest of the registration.

Done.

[David Tseng, 2016-05-03 20:47:18]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-05-03 20:47:19]

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/1942683005/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-05-03 20:47:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942683005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942683005/20001

[commit-bot: I haz the power, 2016-05-03 21:28:54]

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

[commit-bot: I haz the power, 2016-05-03 21:28:55]

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

[David Tseng, 2016-05-03 21:41:26]

Description was changed from

==========
Refactor event handler classes to make it easier to add new event type
listeners.

A new event type, activedescendantchanged, was added.

Moving forward, it will be simpler to separate handlers (which contain groups of
events), into separate classes. BaseAutomationHandler will provide basic
utilities that are useful when daeling with a group of extension-styled event
listeners (e.g. adding/removing, binding, etc).
==========

to

==========
Refactor event handler classes to make it easier to add new event type
listeners.

A new event type, activedescendantchanged, was added.

Moving forward, it will be simpler to separate handlers (which contain groups of
events), into separate classes. BaseAutomationHandler will provide basic
utilities that are useful when daeling with a group of extension-styled event
listeners (e.g. adding/removing, binding, etc).
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-05-03 21:41:35]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-05-03 21:41:35]

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/1942683005/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-03 21:41:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942683005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942683005/40001

[commit-bot: I haz the power, 2016-05-03 23:33:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-03 23:35:25]

Description was changed from

==========
Refactor event handler classes to make it easier to add new event type
listeners.

A new event type, activedescendantchanged, was added.

Moving forward, it will be simpler to separate handlers (which contain groups of
events), into separate classes. BaseAutomationHandler will provide basic
utilities that are useful when daeling with a group of extension-styled event
listeners (e.g. adding/removing, binding, etc).
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Refactor event handler classes to make it easier to add new event type
listeners.

A new event type, activedescendantchanged, was added.

Moving forward, it will be simpler to separate handlers (which contain groups of
events), into separate classes. BaseAutomationHandler will provide basic
utilities that are useful when daeling with a group of extension-styled event
listeners (e.g. adding/removing, binding, etc).
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4b26b22de82af571edd4176547abead9f95ae200
Cr-Commit-Position: refs/heads/master@{#391395}
==========

[commit-bot: I haz the power, 2016-05-03 23:35:26]

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