From cea0dcd04daefeeab410a71787390da4ac28cf72 Mon Sep 17 00:00:00 2001 From: Yura Zenevich Date: Fri, 3 Jul 2020 23:52:12 +0000 Subject: [PATCH] Bug 1640338 - simplify accessibility proxy startup since we no longer need to initialize it until the panel is opened. r=mtigley Differential Revision: https://phabricator.services.mozilla.com/D77056 --- .../accessibility/accessibility-proxy.js | 138 ++++++++---------- devtools/client/accessibility/panel.js | 23 ++- devtools/client/fronts/accessibility.js | 17 +-- devtools/server/tests/browser/head.js | 1 - 4 files changed, 75 insertions(+), 104 deletions(-) diff --git a/devtools/client/accessibility/accessibility-proxy.js b/devtools/client/accessibility/accessibility-proxy.js index c1412bc82082..dbcf6075babc 100644 --- a/devtools/client/accessibility/accessibility-proxy.js +++ b/devtools/client/accessibility/accessibility-proxy.js @@ -5,6 +5,7 @@ "use strict"; const Services = require("Services"); +const EventEmitter = require("devtools/shared/event-emitter"); loader.lazyRequireGetter( this, @@ -29,6 +30,7 @@ class AccessibilityProxy { this._accessibilityWalkerFronts = new Set(); this.lifecycleEvents = new Map(); this.accessibilityEvents = new Map(); + this._updateTargetListeners = new EventEmitter(); this.supports = {}; this.audit = this.audit.bind(this); @@ -45,6 +47,9 @@ class AccessibilityProxy { this.startListeningForParentLifecycleEvents = this.startListeningForParentLifecycleEvents.bind( this ); + this.startListeningForTargetUpdated = this.startListeningForTargetUpdated.bind( + this + ); this.stopListeningForAccessibilityEvents = this.stopListeningForAccessibilityEvents.bind( this ); @@ -54,6 +59,9 @@ class AccessibilityProxy { this.stopListeningForParentLifecycleEvents = this.stopListeningForParentLifecycleEvents.bind( this ); + this.stopListeningForTargetUpdated = this.stopListeningForTargetUpdated.bind( + this + ); this.highlightAccessible = this.highlightAccessible.bind(this); this.unhighlightAccessible = this.unhighlightAccessible.bind(this); this.onTargetAvailable = this.onTargetAvailable.bind(this); @@ -134,6 +142,14 @@ class AccessibilityProxy { return combinedAudit; } + startListeningForTargetUpdated(onTargetUpdated) { + this._updateTargetListeners.on("target-updated", onTargetUpdated); + } + + stopListeningForTargetUpdated(onTargetUpdated) { + this._updateTargetListeners.off("target-updated", onTargetUpdated); + } + async disableAccessibility() { // Accessibility service is shut down using the parent accessibility front. // That, in turn, shuts down accessibility service in all content processes. @@ -280,8 +296,7 @@ class AccessibilityProxy { async resetAccessiblity() { const { enabled } = this.accessibilityFront; - const { canBeEnabled, canBeDisabled } = - this.parentAccessibilityFront || this.accessibilityFront; + const { canBeEnabled, canBeDisabled } = this.parentAccessibilityFront; return { enabled, canBeDisabled, canBeEnabled }; } @@ -387,58 +402,22 @@ class AccessibilityProxy { }); } - /** - * Part of the proxy initialization only needs to be done when the accessibility panel starts. - * To avoid performance issues, the panel will explicitly call this method every time a new - * target becomes available. - */ - async initializeProxyForPanel(targetFront) { - await this.onTargetAvailable({ targetFront }); - - // No need to retrieve parent accessibility front since root front does not - // change. - if (!this.parentAccessibilityFront) { - this.parentAccessibilityFront = await this._currentTarget.client.mainRoot.getFront( - "parentaccessibility" - ); - } - - this.simulatorFront = this.accessibilityFront.simulatorFront; - if (this.simulatorFront) { - this.simulate = types => this.simulatorFront.simulate({ types }); - } else { - this.simulate = null; - } - - // Move accessibility front lifecycle event listeners to a new top level - // front. - for (const [type, listeners] of this.lifecycleEvents.entries()) { - for (const listener of listeners.values()) { - this.accessibilityFront.on(type, listener); - } - } - } - async initialize() { - try { - await this.toolbox.targetList.watchTargets( - [this.toolbox.targetList.TYPES.FRAME], - this.onTargetAvailable, - this.onTargetDestroyed - ); - // Bug 1602075: auto init feature definition is used for an experiment to - // determine if we can automatically enable accessibility panel when it - // opens. - this.supports.autoInit = Services.prefs.getBoolPref( - "devtools.accessibility.auto-init.enabled", - false - ); - - return true; - } catch (e) { - // toolbox may be destroyed during this step. - return false; - } + await this.toolbox.targetList.watchTargets( + [this.toolbox.targetList.TYPES.FRAME], + this.onTargetAvailable, + this.onTargetDestroyed + ); + this.parentAccessibilityFront = await this._currentTarget.client.mainRoot.getFront( + "parentaccessibility" + ); + // Bug 1602075: auto init feature definition is used for an experiment to + // determine if we can automatically enable accessibility panel when it + // opens. + this.supports.autoInit = Services.prefs.getBoolPref( + "devtools.accessibility.auto-init.enabled", + false + ); } destroy() { @@ -450,6 +429,7 @@ class AccessibilityProxy { this.lifecycleEvents.clear(); this.accessibilityEvents.clear(); + this._updateTargetListeners = null; this.accessibilityFront = null; this.parentAccessibilityFront = null; @@ -532,7 +512,7 @@ class AccessibilityProxy { } } - async onTargetAvailable({ targetFront }) { + async onTargetAvailable({ targetFront, isTargetSwitching }) { targetFront.watchFronts( "accessibility", this.onAccessibilityFrontAvailable, @@ -540,33 +520,41 @@ class AccessibilityProxy { ); if (!targetFront.isTopLevel) { - return null; - } - - if (this._updatePromise && this._currentTarget === targetFront) { - return this._updatePromise; + return; } this._currentTarget = targetFront; this._accessibilityWalkerFronts.clear(); - this._updatePromise = (async () => { - this.accessibilityFront = await this._currentTarget.getFront( - "accessibility" - ); - // Finalize accessibility front initialization. See accessibility front - // bootstrap method description. - await this.accessibilityFront.bootstrap(); - // To add a check for backward compatibility add something similar to the - // example below: - // - // [this.supports.simulation] = await Promise.all([ - // // Please specify the version of Firefox when the feature was added. - // this._currentTarget.actorHasMethod("accessibility", "getSimulator"), - // ]); - })(); + this.accessibilityFront = await this._currentTarget.getFront( + "accessibility" + ); + // To add a check for backward compatibility add something similar to the + // example below: + // + // [this.supports.simulation] = await Promise.all([ + // // Please specify the version of Firefox when the feature was added. + // this._currentTarget.actorHasMethod("accessibility", "getSimulator"), + // ]); + this.simulatorFront = this.accessibilityFront.simulatorFront; + if (this.simulatorFront) { + this.simulate = types => this.simulatorFront.simulate({ types }); + } else { + this.simulate = null; + } - return this._updatePromise; + // Move accessibility front lifecycle event listeners to a new top level + // front. + for (const [type, listeners] of this.lifecycleEvents.entries()) { + for (const listener of listeners.values()) { + this.accessibilityFront.on(type, listener); + } + } + + this._updateTargetListeners.emit("target-updated", { + targetFront: this._currentTarget, + isTargetSwitching, + }); } async onTargetDestroyed({ targetFront }) { diff --git a/devtools/client/accessibility/panel.js b/devtools/client/accessibility/panel.js index 38955c2d9c4a..efb52ec4420f 100644 --- a/devtools/client/accessibility/panel.js +++ b/devtools/client/accessibility/panel.js @@ -55,7 +55,7 @@ function AccessibilityPanel(iframeWindow, toolbox) { this._toolbox = toolbox; this.onTabNavigated = this.onTabNavigated.bind(this); - this.onTargetAvailable = this.onTargetAvailable.bind(this); + this.onTargetUpdated = this.onTargetUpdated.bind(this); this.onPanelVisibilityChange = this.onPanelVisibilityChange.bind(this); this.onNewAccessibleFrontSelected = this.onNewAccessibleFrontSelected.bind( this @@ -106,12 +106,10 @@ AccessibilityPanel.prototype = { this.shouldRefresh = true; this.accessibilityProxy = new AccessibilityProxy(this._toolbox); - await this.accessibilityProxy.initialize(); - - await this._toolbox.targetList.watchTargets( - [this._toolbox.targetList.TYPES.FRAME], - this.onTargetAvailable + this.accessibilityProxy.startListeningForTargetUpdated( + this.onTargetUpdated ); + await this.accessibilityProxy.initialize(); // Bug 1602075: if auto init feature is enabled, enable accessibility // service if necessary. @@ -181,10 +179,9 @@ AccessibilityPanel.prototype = { this._opening.then(() => this.refresh()); }, - async onTargetAvailable({ targetFront, isTargetSwitching }) { + onTargetUpdated({ targetFront, isTargetSwitching }) { if (targetFront.isTopLevel) { - await this.accessibilityProxy.initializeProxyForPanel(targetFront); - this.accessibilityProxy.currentTarget.on("navigate", this.onTabNavigated); + targetFront.on("navigate", this.onTabNavigated); } if (isTargetSwitching) { @@ -329,14 +326,12 @@ AccessibilityPanel.prototype = { } this._destroyed = true; - this._toolbox.targetList.unwatchTargets( - [this._toolbox.targetList.TYPES.FRAME], - this.onTargetAvailable - ); - this.postContentMessage("destroy"); if (this.accessibilityProxy) { + this.accessibilityProxy.stopListeningForTargetUpdated( + this.onTargetUpdated + ); this.accessibilityProxy.currentTarget.off( "navigate", this.onTabNavigated diff --git a/devtools/client/fronts/accessibility.js b/devtools/client/fronts/accessibility.js index 0a40d5876c19..edeca9d1a46e 100644 --- a/devtools/client/fronts/accessibility.js +++ b/devtools/client/fronts/accessibility.js @@ -192,7 +192,6 @@ class AccessibleFront extends FrontClassWithSpec(accessibleSpec) { const accessibilityFront = await documentNodeFront.targetFront.getFront( "accessibility" ); - await accessibilityFront.bootstrap(); return accessibilityFront.accessibleWalkerFront.children(); } @@ -330,7 +329,6 @@ class AccessibleWalkerFront extends FrontClassWithSpec(accessibleWalkerSpec) { ); const frameNodeFront = (await domWalkerFront.getRootNode()).parentNode(); const accessibilityFront = await parentTarget.getFront("accessibility"); - await accessibilityFront.bootstrap(); const { accessibleWalkerFront } = accessibilityFront; const frameAccessibleFront = await accessibleWalkerFront.getAccessibleFor( frameNodeFront @@ -443,20 +441,11 @@ class AccessibilityFront extends FrontClassWithSpec(accessibilitySpec) { this.formAttributeName = "accessibilityActor"; } - // We purposefully do not use initialize here and separate accessiblity - // front/actor initialization into two parts: getting the front from target - // and then separately bootstrapping the front. The reason for that is because - // accessibility front is always created as part of the accessibility panel - // startup when the toolbox is opened. If initialize was used, in rare cases, - // when the toolbox is destroyed before the accessibility tool startup is - // complete, the toolbox destruction would hang because the accessibility - // front will indefinitely wait for its initialize method to complete before - // being destroyed. With custom bootstrapping the front will be destroyed - // correctly. - async bootstrap() { + async initialize() { this.accessibleWalkerFront = await super.getWalker(); this.simulatorFront = await super.getSimulator(); - ({ enabled: this.enabled } = await super.bootstrap()); + const { enabled } = await super.bootstrap(); + this.enabled = enabled; } init() { diff --git a/devtools/server/tests/browser/head.js b/devtools/server/tests/browser/head.js index c581ddfb30b8..2de1b34c910f 100644 --- a/devtools/server/tests/browser/head.js +++ b/devtools/server/tests/browser/head.js @@ -88,7 +88,6 @@ async function initAccessibilityFrontsForUrl( "parentaccessibility" ); const accessibility = await target.getFront("accessibility"); - await accessibility.bootstrap(); const a11yWalker = accessibility.accessibleWalkerFront; if (enableByDefault) { await parentAccessibility.enable();