Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright, a=RyanVM
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 07 Jan 2019 16:20:21 +0000
changeset 506592 c777b4b5cd07b6575bacf5a2309b384c0c7c9c6f
parent 506591 39fe709c6cf8de41c2a739ad456877eb5fa1e7c3
child 506593 274d224478736b7ff659c3d1f38dc8c2b2285a8a
push id10476
push userryanvm@gmail.com
push dateWed, 09 Jan 2019 15:36:04 +0000
treeherdermozilla-beta@40a7055f712b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersewright, RyanVM
bugs1495207
milestone65.0
Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright, a=RyanVM Previously this code was using webProgress.isTopLevel to set and reset the shield animation, which is just plain nonsense and was based on false assumptions about it being something like webProgress.isLoadingDocument. In reality this attribute just reflects whether the source of the event is the top-level window or a frame, not the load type. The new code uses the "blocking" state to set and reset the animation and uses the "active" attribute as a guard to ensure that we only set the "animate" attribute once per page. This works because the "active" attribute is guaranteed to be reset on a top-level document load. Differential Revision: https://phabricator.services.mozilla.com/D15654
browser/base/content/browser-contentblocking.js
--- a/browser/base/content/browser-contentblocking.js
+++ b/browser/base/content/browser-contentblocking.js
@@ -771,23 +771,16 @@ var ContentBlocking = {
     // Don't deal with about:, file: etc.
     if (!baseURI) {
       this.iconBox.removeAttribute("animate");
       this.iconBox.removeAttribute("active");
       this.iconBox.removeAttribute("hasException");
       return;
     }
 
-    // The user might have navigated before the shield animation
-    // finished. In this case, reset the animation to be able to
-    // play it in full again and avoid choppiness.
-    if (webProgress.isTopLevel) {
-      this.iconBox.removeAttribute("animate");
-    }
-
     let anyDetected = false;
     let anyBlocking = false;
 
     for (let blocker of this.blockers) {
       // Store data on whether the blocker is activated in the current document for
       // reporting it using the "report breakage" dialog. Under normal circumstances this
       // dialog should only be able to open in the currently selected tab and onSecurityChange
       // runs on tab switch, so we can avoid associating the data with the document directly.
@@ -801,16 +794,36 @@ var ContentBlocking = {
 
     let isBrowserPrivate = PrivateBrowsingUtils.isBrowserPrivate(gBrowser.selectedBrowser);
 
     // Check whether the user has added an exception for this site.
     let type =  isBrowserPrivate ? "trackingprotection-pb" : "trackingprotection";
     let hasException = Services.perms.testExactPermission(baseURI, type) ==
       Services.perms.ALLOW_ACTION;
 
+    // Reset the animation in case the user is switching tabs or if no blockers were detected
+    // (this is most likely happening because the user navigated on to a different site). This
+    // allows us to play it from the start without choppiness next time.
+    if (isSimulated || !anyBlocking) {
+      this.iconBox.removeAttribute("animate");
+    // Only play the animation when the shield is not already shown on the page (the visibility
+    // of the shield based on this onSecurityChange be determined afterwards).
+    } else if (anyBlocking && !this.iconBox.hasAttribute("active")) {
+      this.iconBox.setAttribute("animate", "true");
+
+      if (!isBrowserPrivate) {
+        let introCount = Services.prefs.getIntPref(this.prefIntroCount);
+        if (introCount < this.MAX_INTROS) {
+          Services.prefs.setIntPref(this.prefIntroCount, ++introCount);
+          Services.prefs.savePrefFile(null);
+          this.showIntroPanel();
+        }
+      }
+    }
+
     // We consider the shield state "active" when some kind of blocking activity
     // occurs on the page.  Note that merely allowing the loading of content that
     // we could have blocked does not trigger the appearance of the shield.
     // This state will be overriden later if there's an exception set for this site.
     this.content.toggleAttribute("detected", anyDetected);
     this.content.toggleAttribute("blocking", anyBlocking);
     this.content.toggleAttribute("hasException", hasException);
 
@@ -823,31 +836,16 @@ var ContentBlocking = {
         (ThirdPartyCookies.reportBreakageEnabled &&
          ThirdPartyCookies.activated &&
          !TrackingProtection.activated)) {
       this.reportBreakageButton.removeAttribute("hidden");
     } else {
       this.reportBreakageButton.setAttribute("hidden", "true");
     }
 
-    if (isSimulated) {
-      this.iconBox.removeAttribute("animate");
-    } else if (anyBlocking && webProgress.isTopLevel) {
-      this.iconBox.setAttribute("animate", "true");
-
-      if (!isBrowserPrivate) {
-        let introCount = Services.prefs.getIntPref(this.prefIntroCount);
-        if (introCount < this.MAX_INTROS) {
-          Services.prefs.setIntPref(this.prefIntroCount, ++introCount);
-          Services.prefs.savePrefFile(null);
-          this.showIntroPanel();
-        }
-      }
-    }
-
     if (hasException) {
       this.iconBox.setAttribute("tooltiptext", this.disabledTooltipText);
       this.shieldHistogramAdd(1);
     } else if (anyBlocking) {
       this.iconBox.setAttribute("tooltiptext", this.activeTooltipText);
       this.shieldHistogramAdd(2);
     } else {
       this.iconBox.removeAttribute("tooltiptext");