Bug 1467399 - Part 2: Guard from removed animation during updating the animation state. r?pbro draft
authorDaisuke Akatsuka <dakatsuka@mozilla.com>
Wed, 20 Jun 2018 20:11:42 +0900
changeset 808669 308538e0e17082fc97c10a1d090a15bb79d27ff4
parent 808668 f70539ffc07ded7e63aa97029038428ffced2c2d
child 808670 fda717eec0291e81b52bcbc7b982523146ed3e1c
push id113460
push userbmo:dakatsuka@mozilla.com
push dateWed, 20 Jun 2018 11:31:27 +0000
reviewerspbro
bugs1467399
milestone62.0a1
Bug 1467399 - Part 2: Guard from removed animation during updating the animation state. r?pbro MozReview-Commit-ID: 9Ocb2tQuvoE
devtools/client/inspector/animation/animation.js
--- a/devtools/client/inspector/animation/animation.js
+++ b/devtools/client/inspector/animation/animation.js
@@ -321,20 +321,17 @@ class AnimationInspector {
         animation.off("changed", this.onAnimationStateChanged);
       }
     }
 
     // Update existing other animations as well since the currentTime would be proceeded
     // sice the scrubber position is related the currentTime.
     // Also, don't update the state of removed animations since React components
     // may refer to the same instance still.
-    await this.updateAnimations(animations);
-
-    // Get rid of animations that were removed during async updateAnimations().
-    animations = animations.filter(animation => !!animation.state.type);
+    animations = await this.updateAnimations(animations);
 
     this.updateState(animations.concat(addedAnimations));
   }
 
   onElementPickerStarted() {
     this.inspector.store.dispatch(updateElementPickerEnabled(true));
   }
 
@@ -398,65 +395,65 @@ class AnimationInspector {
   async setAnimationsCurrentTime(currentTime, shouldRefresh) {
     this.stopAnimationsCurrentTimeTimer();
     this.onAnimationsCurrentTimeUpdated(currentTime);
 
     if (!shouldRefresh && this.isCurrentTimeSet) {
       return;
     }
 
-    const { animations } = this.state;
+    let animations = this.state.animations;
     this.isCurrentTimeSet = true;
 
     try {
       await this.doSetCurrentTimes(currentTime);
-      await this.updateAnimations(animations);
+      animations = await this.updateAnimations(animations);
     } catch (e) {
       // Expected if we've already been destroyed or other node have been selected
       // in the meantime.
       console.error(e);
       return;
     }
 
     this.isCurrentTimeSet = false;
 
     if (shouldRefresh) {
-      this.updateState([...animations]);
+      this.updateState(animations);
     }
   }
 
   async setAnimationsPlaybackRate(playbackRate) {
-    const animations = this.state.animations;
+    let animations = this.state.animations;
     // "changed" event on each animation will fire respectively when the playback
     // rate changed. Since for each occurrence of event, change of UI is urged.
     // To avoid this, disable the listeners once in order to not capture the event.
     this.setAnimationStateChangedListenerEnabled(false);
 
     try {
       await this.animationsFront.setPlaybackRates(animations, playbackRate);
-      await this.updateAnimations(animations);
+      animations = await this.updateAnimations(animations);
     } catch (e) {
       // Expected if we've already been destroyed or other node have been selected
       // in the meantime.
       console.error(e);
       return;
     } finally {
       this.setAnimationStateChangedListenerEnabled(true);
     }
 
-    await this.updateState([...animations]);
+    await this.updateState(animations);
   }
 
   async setAnimationsPlayState(doPlay) {
     if (typeof this.hasPausePlaySome === "undefined") {
       this.hasPausePlaySome =
         await this.inspector.target.actorHasMethod("animations", "pauseSome");
     }
 
-    const { animations, timeScale } = this.state;
+    let { animations, timeScale } = this.state;
 
     try {
       if (doPlay && animations.every(animation =>
                       timeScale.getEndTime(animation) <= animation.state.currentTime)) {
         await this.doSetCurrentTimes(0);
       }
 
       // If the server does not support pauseSome/playSome function, (which happens
@@ -469,25 +466,25 @@ class AnimationInspector {
           await this.animationsFront.pauseSome(animations);
         }
       } else if (doPlay) {
         await this.animationsFront.playAll();
       } else {
         await this.animationsFront.pauseAll();
       }
 
-      await this.updateAnimations(animations);
+      animations = await this.updateAnimations(animations);
     } catch (e) {
       // Expected if we've already been destroyed or other node have been selected
       // in the meantime.
       console.error(e);
       return;
     }
 
-    await this.updateState([...animations]);
+    await this.updateState(animations);
   }
 
   /**
    * Enable/disable the animation state change listener.
    * If set true, observe "changed" event on current animations.
    * Otherwise, quit observing the "changed" event.
    *
    * @param {Bool} isEnabled
@@ -624,41 +621,39 @@ class AnimationInspector {
       ? await this.animationsFront.getAnimationPlayersForNode(selection.nodeFront)
       : [];
     this.updateState(animations);
     this.setAnimationStateChangedListenerEnabled(true);
 
     done();
   }
 
-  updateAnimations(animations) {
-    if (!animations.length) {
-      return Promise.resolve();
-    }
+  async updateAnimations(animations) {
+    let error = null;
 
-    return new Promise((resolve, reject) => {
-      let count = 0;
-      let error = null;
-
-      for (const animation of animations) {
+    const promises = animations.map(animation => {
+      return new Promise(resolve => {
         animation.refreshState().catch(e => {
           error = e;
         }).finally(() => {
-          count += 1;
+          resolve();
+        });
+      });
+    });
+    await Promise.all(promises);
 
-          if (count === animations.length) {
-            if (error) {
-              reject(error);
-            } else {
-              resolve();
-            }
-          }
-        });
-      }
-    });
+    if (error) {
+      throw new Error(error);
+    }
+
+    // Even when removal animation on inspected document, updateAnimations
+    // might be called before onAnimationsMutation due to the async timing.
+    // Return the animations as result of updateAnimations after getting rid of
+    // the animations since they should not display.
+    return animations.filter(anim => !!anim.state.type);
   }
 
   updateState(animations) {
     // Animation inspector already destroyed
     if (!this.inspector) {
       return;
     }