Bug 1402846 - Wait for TextEditor updates in inspector codebase. r=pbro draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Mon, 25 Sep 2017 15:17:10 +0200
changeset 670519 10cfc4f36c4adc1722c9ece6baa362a84b833166
parent 670182 e6b3498a39b94616ba36798fe0b71a3090b1b14c
child 733250 f71aec5e2fcaf18e610ffecbc5dc06f84b2d3f7a
push id81648
push userbmo:poirot.alex@gmail.com
push dateTue, 26 Sep 2017 14:17:19 +0000
reviewerspbro
bugs1402846
milestone58.0a1
Bug 1402846 - Wait for TextEditor updates in inspector codebase. r=pbro MozReview-Commit-ID: 37zLrrbqFtT
devtools/client/inspector/markup/markup.js
devtools/client/inspector/markup/test/browser_markup_load_01.js
devtools/client/inspector/markup/views/markup-container.js
devtools/client/inspector/markup/views/text-editor.js
devtools/client/inspector/test/browser_inspector_addNode_03.js
devtools/client/inspector/test/browser_inspector_startup.js
--- a/devtools/client/inspector/markup/markup.js
+++ b/devtools/client/inspector/markup/markup.js
@@ -941,17 +941,17 @@ MarkupView.prototype = {
    * Make sure a node is included in the markup tool.
    *
    * @param  {NodeFront} node
    *         The node in the content document.
    * @param  {Boolean} flashNode
    *         Whether the newly imported node should be flashed
    * @return {MarkupContainer} The MarkupContainer object for this element.
    */
-  importNode: function (node, flashNode) {
+  async importNode(node, flashNode) {
     if (!node) {
       return null;
     }
 
     if (this._containers.has(node)) {
       return this.getContainer(node);
     }
 
@@ -964,36 +964,42 @@ MarkupView.prototype = {
     } else if (nodeType == nodeConstants.ELEMENT_NODE && !isPseudoElement) {
       container = new MarkupElementContainer(this, node, this.inspector);
     } else if (nodeType == nodeConstants.COMMENT_NODE ||
                nodeType == nodeConstants.TEXT_NODE) {
       container = new MarkupTextContainer(this, node, this.inspector);
     } else {
       container = new MarkupReadOnlyContainer(this, node, this.inspector);
     }
+    this._containers.set(node, container);
+    await container.update();
 
     if (flashNode) {
       container.flashMutation();
     }
 
-    this._containers.set(node, container);
     container.childrenDirty = true;
 
     this._updateChildren(container);
 
     this.inspector.emit("container-created", container);
 
     return container;
   },
 
   /**
    * Mutation observer used for included nodes.
    */
-  _mutationObserver: function (mutations) {
+  async _mutationObserver(mutations) {
     for (let mutation of mutations) {
+      // As this for loop contains async calls, markup may be destroyed in middle
+      // of the iteration. If that happens, just stop.
+      if (this._destroyer) {
+        return;
+      }
       let type = mutation.type;
       let target = mutation.target;
 
       if (mutation.type === "documentUnload") {
         // Treat this as a childList change of the child (maybe the protocol
         // should do this).
         type = "childList";
         target = mutation.targetParent;
@@ -1009,46 +1015,45 @@ MarkupView.prototype = {
         continue;
       }
 
       if (type === "attributes" && mutation.attributeName === "class") {
         container.updateIsDisplayed();
       }
       if (type === "attributes" || type === "characterData"
         || type === "events" || type === "pseudoClassLock") {
-        container.update();
+        await container.update();
       } else if (type === "childList" || type === "nativeAnonymousChildList") {
         container.childrenDirty = true;
         // Update the children to take care of changes in the markup view DOM
         // and update container (and its subtree) DOM tree depth level for
         // accessibility where necessary.
-        this._updateChildren(container, {flash: true}).then(() =>
-          container.updateLevel());
+        await this._updateChildren(container, {flash: true});
+        container.updateLevel();
       } else if (type === "inlineTextChild") {
         container.childrenDirty = true;
-        this._updateChildren(container, {flash: true});
-        container.update();
+        await this._updateChildren(container, {flash: true});
+        await container.update();
       }
     }
 
-    this._waitForChildren().then(() => {
-      if (this._destroyer) {
-        // Could not fully update after markup mutations, the markup-view was destroyed
-        // while waiting for children. Bail out silently.
-        return;
-      }
-      this._flashMutatedNodes(mutations);
-      this.inspector.emit("markupmutation", mutations);
+    await this._waitForChildren();
+    if (this._destroyer) {
+      // Could not fully update after markup mutations, the markup-view was destroyed
+      // while waiting for children. Bail out silently.
+      return;
+    }
+    this._flashMutatedNodes(mutations);
+    this.inspector.emit("markupmutation", mutations);
 
-      // Since the htmlEditor is absolutely positioned, a mutation may change
-      // the location in which it should be shown.
-      if (this.htmlEditor) {
-        this.htmlEditor.refresh();
-      }
-    });
+    // Since the htmlEditor is absolutely positioned, a mutation may change
+    // the location in which it should be shown.
+    if (this.htmlEditor) {
+      this.htmlEditor.refresh();
+    }
   },
 
   /**
    * React to display-change events from the walker
    *
    * @param  {Array} nodes
    *         An array of nodeFronts
    */
@@ -1111,34 +1116,36 @@ MarkupView.prototype = {
       container.flashMutation();
     }
   },
 
   /**
    * Make sure the given node's parents are expanded and the
    * node is scrolled on to screen.
    */
-  showNode: function (node, centered = true) {
-    let parent = node;
+  async showNode(node, centered = true) {
+    try {
+      let parent = node;
+
+      await this.importNode(node);
 
-    this.importNode(node);
+      while ((parent = parent.parentNode())) {
+        await this.importNode(parent);
+        await this.expandNode(parent);
+      }
 
-    while ((parent = parent.parentNode())) {
-      this.importNode(parent);
-      this.expandNode(parent);
+      await this._waitForChildren();
+      if (this._destroyer) {
+        return;
+      }
+      await this._ensureVisible(node);
+      scrollIntoViewIfNeeded(this.getContainer(node).editor.elt, centered);
+    } catch (e) {
+      this._handleRejectionIfNotDestroyed(e);
     }
-
-    return this._waitForChildren().then(() => {
-      if (this._destroyer) {
-        return promise.reject("markupview destroyed");
-      }
-      return this._ensureVisible(node);
-    }).then(() => {
-      scrollIntoViewIfNeeded(this.getContainer(node).editor.elt, centered);
-    }, this._handleRejectionIfNotDestroyed);
   },
 
   /**
    * Expand the container's children.
    */
   _expandContainer: function (container) {
     return this._updateChildren(container, {expand: true}).then(() => {
       if (this._destroyer) {
@@ -1256,17 +1263,17 @@ MarkupView.prototype = {
     this.cancelReselectOnRemoved();
 
     // Get the removedNode index in its parent node to reselect the right node.
     let isHTMLTag = removedNode.tagName.toLowerCase() === "html";
     let oldContainer = this.getContainer(removedNode);
     let parentContainer = this.getContainer(removedNode.parentNode());
     let childIndex = parentContainer.getChildContainers().indexOf(oldContainer);
 
-    let onMutations = this._removedNodeObserver = (e, mutations) => {
+    let onMutations = this._removedNodeObserver = async (e, mutations) => {
       let isNodeRemovalMutation = false;
       for (let mutation of mutations) {
         let containsRemovedNode = mutation.removed &&
                                   mutation.removed.some(n => n === removedNode);
         if (mutation.type === "childList" &&
             (containsRemovedNode || isHTMLTag)) {
           isNodeRemovalMutation = true;
           break;
@@ -1282,17 +1289,17 @@ MarkupView.prototype = {
       // Don't select the new node if the user has already changed the current
       // selection.
       if (this.inspector.selection.nodeFront === parentContainer.node ||
           (this.inspector.selection.nodeFront === removedNode && isHTMLTag)) {
         let childContainers = parentContainer.getChildContainers();
         if (childContainers && childContainers[childIndex]) {
           this.markNodeAsSelected(childContainers[childIndex].node, reason);
           if (childContainers[childIndex].hasChildren) {
-            this.expandNode(childContainers[childIndex].node);
+            await this.expandNode(childContainers[childIndex].node);
           }
           this.emit("reselectedonremoved");
         }
       }
     };
 
     // Start listening for mutations until we find a childList change that has
     // removedNode removed.
@@ -1561,17 +1568,17 @@ MarkupView.prototype = {
    *
    * @param  {MarkupContainer} container
    *         The markup container whose children need updating
    * @param  {Object} options
    *         Options are {expand:boolean,flash:boolean}
    * @return {Promise} that will be resolved when the children are ready
    *         (which may be immediately).
    */
-  _updateChildren: function (container, options) {
+  async _updateChildren(container, options) {
     let expand = options && options.expand;
     let flash = options && options.flash;
 
     container.hasChildren = container.node.hasChildren;
     // Accessibility should either ignore empty children or semantically
     // consider them a group.
     container.setChildrenRole();
 
@@ -1579,17 +1586,17 @@ MarkupView.prototype = {
       this._queuedChildUpdates = new Map();
     }
 
     if (this._queuedChildUpdates.has(container)) {
       return this._queuedChildUpdates.get(container);
     }
 
     if (!container.childrenDirty) {
-      return promise.resolve(container);
+      return container;
     }
 
     if (container.inlineTextChild
         && container.inlineTextChild != container.node.inlineTextChild) {
       // This container was doing double duty as a container for a single
       // text child, back that out.
       this._containers.delete(container.inlineTextChild);
       container.clearInlineTextChild();
@@ -1606,81 +1613,84 @@ MarkupView.prototype = {
       while (container.children.firstChild) {
         container.children.firstChild.remove();
       }
 
       container.setInlineTextChild(container.node.inlineTextChild);
 
       this._containers.set(container.node.inlineTextChild, container);
       container.childrenDirty = false;
-      return promise.resolve(container);
+      return container;
     }
 
     if (!container.hasChildren) {
       while (container.children.firstChild) {
         container.children.firstChild.remove();
       }
       container.childrenDirty = false;
       container.setExpanded(false);
-      return promise.resolve(container);
+      return container;
     }
 
     // If we're not expanded (or asked to update anyway), we're done for
     // now.  Note that this will leave the childrenDirty flag set, so when
     // expanded we'll refresh the child list.
     if (!(container.expanded || expand)) {
-      return promise.resolve(container);
+      return container;
     }
 
     // We're going to issue a children request, make sure it includes the
     // centered node.
     let centered = this._checkSelectionVisible(container);
 
     // Children aren't updated yet, but clear the childrenDirty flag anyway.
     // If the dirty flag is re-set while we're fetching we'll need to fetch
     // again.
     container.childrenDirty = false;
-    let updatePromise =
-      this._getVisibleChildren(container, centered).then(children => {
-        if (!this._containers) {
-          return promise.reject("markup view destroyed");
-        }
-        this._queuedChildUpdates.delete(container);
+    let updatePromise = this._getVisibleChildren(container, centered);
+    this._queuedChildUpdates.set(container, updatePromise);
+    try {
+      let children = await updatePromise;
+      if (!this._containers) {
+        throw new Error("markup view destroyed");
+      }
+      this._queuedChildUpdates.delete(container);
 
-        // If children are dirty, we got a change notification for this node
-        // while the request was in progress, we need to do it again.
-        if (container.childrenDirty) {
-          return this._updateChildren(container, {expand: centered || expand});
-        }
+      // If children are dirty, we got a change notification for this node
+      // while the request was in progress, we need to do it again.
+      if (container.childrenDirty) {
+        return this._updateChildren(container, {expand: centered || expand});
+      }
 
-        let fragment = this.doc.createDocumentFragment();
+      let fragment = this.doc.createDocumentFragment();
 
-        for (let child of children.nodes) {
-          let childContainer = this.importNode(child, flash);
-          fragment.appendChild(childContainer.elt);
-        }
+      for (let child of children.nodes) {
+        let childContainer = await this.importNode(child, flash);
+        fragment.appendChild(childContainer.elt);
+      }
 
-        while (container.children.firstChild) {
-          container.children.firstChild.remove();
-        }
+      while (container.children.firstChild) {
+        container.children.firstChild.remove();
+      }
 
-        if (!children.hasFirst) {
-          let topItem = this.buildMoreNodesButtonMarkup(container);
-          fragment.insertBefore(topItem, fragment.firstChild);
-        }
-        if (!children.hasLast) {
-          let bottomItem = this.buildMoreNodesButtonMarkup(container);
-          fragment.appendChild(bottomItem);
-        }
+      if (!children.hasFirst) {
+        let topItem = this.buildMoreNodesButtonMarkup(container);
+        fragment.insertBefore(topItem, fragment.firstChild);
+      }
+      if (!children.hasLast) {
+        let bottomItem = this.buildMoreNodesButtonMarkup(container);
+        fragment.appendChild(bottomItem);
+      }
 
-        container.children.appendChild(fragment);
-        return container;
-      }).catch(this._handleRejectionIfNotDestroyed);
-    this._queuedChildUpdates.set(container, updatePromise);
-    return updatePromise;
+      container.children.appendChild(fragment);
+      return container;
+    } catch (e) {
+      this._handleRejectionIfNotDestroyed(e);
+    }
+    return null;
   },
 
   buildMoreNodesButtonMarkup: function (container) {
     let elt = this.doc.createElement("li");
     elt.classList.add("more-nodes", "devtools-class-comment");
 
     let label = this.doc.createElement("span");
     label.textContent = INSPECTOR_L10N.getStr("markupView.more.showing");
--- a/devtools/client/inspector/markup/test/browser_markup_load_01.js
+++ b/devtools/client/inspector/markup/test/browser_markup_load_01.js
@@ -52,16 +52,20 @@ add_task(function* () {
   let multipleChildrenUpdates = waitForMultipleChildrenUpdates(inspector);
   yield chooseWithInspectElementContextMenu("img", tab);
 
   info("Wait for load");
   yield pageLoaded;
 
   info("Wait for markup-loaded after element inspection");
   yield markupLoaded;
+
+  info("Wait for inspector-updated");
+  yield inspector.once("inspector-updated");
+
   info("Wait for multiple children updates after element inspection");
   yield multipleChildrenUpdates;
 
   ok(inspector.markup, "There is a markup view");
   is(inspector.markup._elt.children.length, 1, "The markup view is rendering");
 });
 
 function* chooseWithInspectElementContextMenu(selector, tab) {
--- a/devtools/client/inspector/markup/views/markup-container.js
+++ b/devtools/client/inspector/markup/views/markup-container.js
@@ -671,25 +671,25 @@ MarkupContainer.prototype = {
       this.tagState.classList.remove("theme-selected");
     }
   },
 
   /**
    * Update the container's editor to the current state of the
    * viewed node.
    */
-  update: function () {
+  async update() {
     if (this.node.pseudoClassLocks.length) {
       this.elt.classList.add("pseudoclass-locked");
     } else {
       this.elt.classList.remove("pseudoclass-locked");
     }
 
     if (this.editor.update) {
-      this.editor.update();
+      await this.editor.update();
     }
   },
 
   /**
    * Try to put keyboard focus on the current editor.
    */
   focus: function () {
     // Elements with tabindex of -1 are not focusable.
--- a/devtools/client/inspector/markup/views/text-editor.js
+++ b/devtools/client/inspector/markup/views/text-editor.js
@@ -51,18 +51,16 @@ function TextEditor(container, node, typ
           }, () => {
             this.node.setNodeValue(oldValue);
           });
         });
       });
     },
     cssProperties: getCssProperties(this.markup.toolbox),
   });
-
-  this.update();
 }
 
 TextEditor.prototype = {
   buildMarkup: function (type) {
     let doc = this.markup.doc;
 
     this.elt = doc.createElement("span");
     this.elt.classList.add("editor", type);
@@ -92,35 +90,32 @@ TextEditor.prototype = {
   set selected(value) {
     if (value === this._selected) {
       return;
     }
     this._selected = value;
     this.update();
   },
 
-  update: function () {
-    let longstr = null;
-    this.node.getNodeValue().then(ret => {
-      longstr = ret;
-      return longstr.string();
-    }).then(str => {
-      longstr.release().catch(console.error);
-      this.value.textContent = str;
+  async update() {
+    let longstr = await this.node.getNodeValue();
+    let str = await longstr.string();
+    longstr.release().catch(console.error);
+
+    this.value.textContent = str;
 
-      let isWhitespace = !/[^\s]/.exec(str);
-      this.value.classList.toggle("whitespace", isWhitespace);
+    let isWhitespace = !/[^\s]/.exec(str);
+    this.value.classList.toggle("whitespace", isWhitespace);
 
-      let chars = str.replace(/\n/g, "⏎")
-                     .replace(/\t/g, "⇥")
-                     .replace(/ /g, "◦");
-      this.value.setAttribute("title", isWhitespace
-        ? INSPECTOR_L10N.getFormatStr("markupView.whitespaceOnly", chars)
-        : "");
-    }).catch(console.error);
+    let chars = str.replace(/\n/g, "⏎")
+                   .replace(/\t/g, "⇥")
+                   .replace(/ /g, "◦");
+    this.value.setAttribute("title", isWhitespace
+      ? INSPECTOR_L10N.getFormatStr("markupView.whitespaceOnly", chars)
+      : "");
   },
 
   destroy: function () {},
 
   /**
    * Stub method for consistency with ElementEditor.
    */
   getInfoAtNode: function () {
--- a/devtools/client/inspector/test/browser_inspector_addNode_03.js
+++ b/devtools/client/inspector/test/browser_inspector_addNode_03.js
@@ -16,41 +16,53 @@ add_task(function* () {
   info("Adding a node in an element that has no children and is collapsed");
   let parentNode = yield getNodeFront("#foo", inspector);
   yield selectNode(parentNode, inspector);
   yield testAddNode(parentNode, inspector);
 
   info("Adding a node in an element with children but that has not been expanded yet");
   parentNode = yield getNodeFront("#bar", inspector);
   yield selectNode(parentNode, inspector);
-  yield testAddNode(parentNode, inspector);
+  yield testAddNode(parentNode, inspector, 2);
 
   info("Adding a node in an element with children that has been expanded then collapsed");
   // Select again #bar and collapse it.
   parentNode = yield getNodeFront("#bar", inspector);
   yield selectNode(parentNode, inspector);
   collapseNode(parentNode, inspector);
   yield testAddNode(parentNode, inspector);
 
   info("Adding a node in an element with children that is expanded");
   parentNode = yield getNodeFront("#bar", inspector);
   yield selectNode(parentNode, inspector);
   yield testAddNode(parentNode, inspector);
 });
 
-function* testAddNode(parentNode, inspector) {
+function* testAddNode(parentNode, inspector, expectedContainers = 1) {
   let btn = inspector.panelDoc.querySelector("#inspector-element-add-button");
   let parentContainer = inspector.markup.getContainer(parentNode);
 
   is(parentContainer.tagLine.getAttribute("aria-level"), PARENT_TREE_LEVEL,
      "The parent aria-level is up to date.");
 
   info("Clicking 'add node' and expecting a markup mutation and a new container");
+
   let onMutation = inspector.once("markupmutation");
-  let onNewContainer = inspector.once("container-created");
+  // In addition to the added node, we should also wait for the number of children
+  // we will expand on the parent node.
+  let onNewContainer = new Promise(done => {
+    let created = 0;
+    let onContainer = (a,c) => {
+      if (++created == expectedContainers) {
+        inspector.off("container-created", onContainer);
+        done();
+      }
+    }
+    inspector.on("container-created", onContainer);
+  });
   btn.click();
   let mutations = yield onMutation;
   yield onNewContainer;
 
   is(mutations.length, 1, "There is one mutation only");
   is(mutations[0].added.length, 1, "There is one new node only");
 
   let newNode = mutations[0].added[0];
--- a/devtools/client/inspector/test/browser_inspector_startup.js
+++ b/devtools/client/inspector/test/browser_inspector_startup.js
@@ -50,16 +50,19 @@ add_task(function* () {
   // The request made to the image shouldn't block the DOMContentLoaded event
   info("Wait for DOMContentLoaded");
   yield domContentLoaded;
 
   // Nor does it prevent the inspector from loading
   info("Wait for markup-loaded");
   yield markupLoaded;
 
+  info("Wait for inspector-updated");
+  yield inspector.once("inspector-updated");
+
   ok(inspector.markup, "There is a markup view");
   is(inspector.markup._elt.children.length, 1, "The markup view is rendering");
   is(yield contentReadyState(tab), "interactive",
      "Page is still loading but the inspector is ready");
 
   // Ends page load by unblocking the image request
   response.finish();