Bug 1265796 - Use queueAsyncCalls in inspector draft
authorGreg Tatum <tatum.creative@gmail.com>
Wed, 17 Aug 2016 10:10:32 -0500
changeset 409217 6e210d44672518d38ed41772d5b0359d70923135
parent 409216 f055a214952efa973575f8b207fe0b884ca60228
child 530306 c51da5f78122c7390e4cfa33560840dfeddbcfac
push id28433
push userbmo:gtatum@mozilla.com
push dateFri, 02 Sep 2016 15:08:28 +0000
bugs1265796
milestone51.0a1
Bug 1265796 - Use queueAsyncCalls in inspector MozReview-Commit-ID: AiDqpr7cS0P
devtools/client/inspector/rules/models/element-style.js
devtools/client/inspector/rules/models/rule.js
devtools/client/inspector/rules/models/text-property.js
devtools/client/inspector/rules/rules.js
--- a/devtools/client/inspector/rules/models/element-style.js
+++ b/devtools/client/inspector/rules/models/element-style.js
@@ -7,16 +7,17 @@
 "use strict";
 
 const promise = require("promise");
 const {Rule} = require("devtools/client/inspector/rules/models/rule");
 const {promiseWarn} = require("devtools/client/inspector/shared/utils");
 const {ELEMENT_STYLE} = require("devtools/shared/specs/styles");
 const {getCssProperties} = require("devtools/shared/fronts/css-properties");
 const {Task} = require("devtools/shared/task");
+const {queueAsyncCalls} = require("devtools/shared/DevToolsUtils");;
 
 /**
  * ElementStyle is responsible for the following:
  *   Keeps track of which properties are overridden.
  *   Maintains a list of Rule objects for a given element.
  *
  * @param {Element} element
  *        The element whose style we are viewing.
@@ -37,16 +38,20 @@ function ElementStyle(element, ruleView,
   this.element = element;
   this.ruleView = ruleView;
   this.store = store || {};
   this.pageStyle = pageStyle;
   this.showUserAgentStyles = showUserAgentStyles;
   this.rules = [];
   this.cssProperties = getCssProperties(this.ruleView.inspector.toolbox);
 
+  // Make sure that the populate calls wait in a queue so they are processed in the order
+  // they were received.
+  this.populate = queueAsyncCalls(this.populate, this);
+
   // We don't want to overwrite this.store.userProperties so we only create it
   // if it doesn't already exist.
   if (!("userProperties" in this.store)) {
     this.store.userProperties = new UserProperties();
   }
 
   if (!("disabled" in this.store)) {
     this.store.disabled = new WeakMap();
@@ -82,66 +87,62 @@ ElementStyle.prototype = {
 
   /**
    * Refresh the list of rules to be displayed for the active element.
    * Upon completion, this.rules[] will hold a list of Rule objects.
    *
    * Returns a promise that will be resolved when the elementStyle is
    * ready.
    */
-  populate: function () {
-    let self = this;
-    let populated = this.pageStyle.getApplied(this.element, {
+  populate: Task.async(function* () {
+    let entries = yield this.pageStyle.getApplied(this.element, {
       inherited: true,
       matchedSelectors: true,
       filter: this.showUserAgentStyles ? "ua" : undefined,
-    }).then(Task.async(function* (entries) {
-      if (self.destroyed) {
-        return promise.resolve(undefined);
+    }).then(null, e => {
+      // Don't warn if the element style is already destroyed as populate is often
+      // called after a setTimeout and the connection may already be closed.
+      if (!this.destroyed) {
+        promiseWarn(e);
       }
-
-      if (self.populated !== populated) {
-        // Don't care anymore.
-        return promise.resolve(undefined);
-      }
+    });
 
-      // Store the current list of rules (if any) during the population
-      // process.  They will be reused if possible.
-      let existingRules = self.rules;
+    if (!entries || this.destroyed) {
+      return;
+    }
 
-      self.rules = [];
+    // Store the current list of rules (if any) during the population
+    // process.  They will be reused if possible.
+    let existingRules = this.rules;
 
-      for (let entry of entries) {
-        yield self._maybeAddRule(entry, existingRules);
-      }
+    this.rules = [];
 
-      // Mark overridden computed styles.
-      self.markOverriddenAll();
-
-      self._sortRulesForPseudoElement();
+    for (let entry of entries) {
+      // Since this is async, check to make sure this component is still around before
+      // doing anything with it.
+      yield this._maybeAddRule(entry, existingRules);
+      if (this.destroyed) {
+        return;
+      }
+    }
 
-      // We're done with the previous list of rules.
-      for (let r of existingRules) {
-        if (r && r.editor) {
-          r.editor.destroy();
-        }
-      }
+    // Mark overridden computed styles.
+    this.markOverriddenAll();
 
-      return undefined;
-    })).then(null, e => {
-      // populate is often called after a setTimeout,
-      // the connection may already be closed.
-      if (this.destroyed) {
-        return promise.resolve(undefined);
+    this._sortRulesForPseudoElement();
+
+    // We're done with the previous list of rules.
+    for (let r of existingRules) {
+      if (r && r.editor) {
+        r.editor.destroy();
       }
-      return promiseWarn(e);
-    });
-    this.populated = populated;
-    return this.populated;
-  },
+    }
+
+    return;
+  }),
 
   /**
    * Put pseudo elements in front of others.
    */
   _sortRulesForPseudoElement: function () {
     this.rules = this.rules.sort((a, b) => {
       return (a.pseudoElement || "z") > (b.pseudoElement || "z");
     });
--- a/devtools/client/inspector/rules/models/rule.js
+++ b/devtools/client/inspector/rules/models/rule.js
@@ -10,16 +10,17 @@ const promise = require("promise");
 const CssLogic = require("devtools/shared/inspector/css-logic");
 const {ELEMENT_STYLE} = require("devtools/shared/specs/styles");
 const {TextProperty} =
       require("devtools/client/inspector/rules/models/text-property");
 const {promiseWarn} = require("devtools/client/inspector/shared/utils");
 const {parseDeclarations} = require("devtools/shared/css-parsing-utils");
 const Services = require("Services");
 const {Task} = require("devtools/shared/task");
+const {queueAsyncCalls} = require("devtools/shared/DevToolsUtils");;
 
 /**
  * Rule is responsible for the following:
  *   Manages a single style declaration or rule.
  *   Applies changes to the properties in a rule.
  *   Maintains a list of TextProperty objects.
  *
  * @param {ElementStyle} elementStyle
@@ -41,16 +42,19 @@ function Rule(elementStyle, options) {
   this.pseudoElement = options.pseudoElement || "";
 
   this.isSystem = options.isSystem;
   this.isUnmatched = options.isUnmatched || false;
   this.inherited = options.inherited || null;
   this.keyframes = options.keyframes || null;
   this._modificationDepth = 0;
 
+  // Make sure that the async refresh calls are queued and execute in order.
+  this.refresh = queueAsyncCalls(this.refresh, this);
+
   if (this.domRule && this.domRule.mediaText) {
     this.mediaText = this.domRule.mediaText;
   }
 
   this.cssProperties = this.elementStyle.ruleView.cssProperties;
 
   // Populate the text properties with the style's current authoredText
   // value, and add in any disabled properties from the store.
@@ -199,17 +203,16 @@ Rule.prototype = {
 
   /**
    * Helper function for applyProperties that is called when the actor
    * does not support as-authored styles.  Store disabled properties
    * in the element style's store.
    */
   _applyPropertiesNoAuthored: function (modifications) {
     this.elementStyle.markOverriddenAll();
-
     let disabledProps = [];
 
     for (let prop of this.textProps) {
       if (prop.invisible) {
         continue;
       }
       if (!prop.enabled) {
         disabledProps.push({
@@ -494,41 +497,40 @@ Rule.prototype = {
 
   /**
    * Reread the current state of the rules and rebuild text
    * properties as needed.
    */
   refresh: Task.async(function* (options) {
     this.matchedSelectors = options.matchedSelectors || [];
     let newTextProps = this._getTextProperties();
+    let visited = new Map();
 
-    // Update current properties for each property present on the style.
-    // This will mark any touched properties with _visited so we
-    // can detect properties that weren't touched (because they were
-    // removed from the style).
-    // Also keep track of properties that didn't exist in the current set
-    // of properties.
+    // Update current properties for each property present on the style. Any touched
+    // properties will be set to true in the `visited` map so we can detect properties
+    // that weren't touched (because they were removed from the style). Also keep track
+    // of properties that didn't exist in the current set of properties.
     let brandNewProps = [];
     for (let newProp of newTextProps) {
       yield newProp.updateComputed();
-      if (!this._updateTextProperty(newProp)) {
+      if (!this._updateTextProperty(newProp, visited)) {
         brandNewProps.push(newProp);
       }
     }
 
     // Refresh editors and disabled state for all the properties that
     // were updated.
     for (let prop of this.textProps) {
       // Properties that weren't touched during the update
       // process must no longer exist on the node.  Mark them disabled.
-      if (!prop._visited) {
+      if (!visited.get(prop)) {
         prop.enabled = false;
         prop.updateEditor();
       } else {
-        delete prop._visited;
+        visited.delete(prop);
       }
     }
 
     // Add brand new properties.
     this.textProps = this.textProps.concat(brandNewProps);
 
     // Refresh the editor if one already exists.
     if (this.editor) {
@@ -553,26 +555,26 @@ Rule.prototype = {
    * If no existing properties match the property, nothing happens.
    *
    * @param {TextProperty} newProp
    *        The current version of the property, as parsed from the
    *        authoredText in Rule._getTextProperties().
    * @return {Boolean} true if a property was updated, false if no properties
    *         were updated.
    */
-  _updateTextProperty: function (newProp) {
+  _updateTextProperty: function (newProp, visited) {
     let match = { rank: 0, prop: null };
 
     for (let prop of this.textProps) {
       if (prop.name !== newProp.name) {
         continue;
       }
 
       // Mark this property visited.
-      prop._visited = true;
+      visited.set(prop, true);
 
       // Start at rank 1 for matching name.
       let rank = 1;
 
       // Value and Priority matches add 2 to the rank.
       // Being enabled adds 1.  This ranks better matches higher,
       // with priority breaking ties.
       if (prop.value === newProp.value) {
--- a/devtools/client/inspector/rules/models/text-property.js
+++ b/devtools/client/inspector/rules/models/text-property.js
@@ -54,16 +54,17 @@ function TextProperty(rule, name, value,
   this.enabled = !!enabled;
   this.invisible = invisible;
   const toolbox = this.rule.elementStyle.ruleView.inspector.toolbox;
   this.cssProperties = getCssProperties(toolbox);
   this.pageStyle = this.rule.elementStyle.pageStyle;
 }
 
 TextProperty.prototype = {
+
   /**
    * Update the editor associated with this text property,
    * if any.
    */
   updateEditor: function () {
     if (this.editor) {
       this.editor.update();
     }
--- a/devtools/client/inspector/rules/rules.js
+++ b/devtools/client/inspector/rules/rules.js
@@ -805,16 +805,21 @@ CssRuleView.prototype = {
           this.element.scrollTop = 0;
         }
         this._stopSelectingElement();
         this._elementStyle.onChanged = () => {
           this._changed();
         };
       }
     }).then(null, e => {
+      if (this.isDestroyed) {
+        // Selecting the element will fail if this element is destroyed midway through,
+        // which is expected.
+        return;
+      }
       if (this._elementStyle === elementStyle) {
         this._stopSelectingElement();
         this._clearRules();
       }
       console.error(e);
     });
   },
 
@@ -1577,17 +1582,17 @@ RuleViewTool.prototype = {
       this.view.selectElement(this.inspector.selection.nodeFront)
         .then(done, done);
     }
   },
 
   refresh: function () {
     if (this.isSidebarActive()) {
       this.view.refreshPanel().then(() => {
-        if(this.view.element) {
+        if(this.view && this.view.element) {
           // If there is an editor, make sure it has retained focus.
           const el = this.view.element.querySelector('input, textarea');
           if (el) {
             el.focus();
           }
         }
       });
     }