Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 07 Jan 2019 16:20:21 +0000
changeset 509820 b9a24fd4b40bc79930265fb09c4fbe2656b9a6f3
parent 509819 653b89c61e4946a8ec12f33c9b16d4f751e0b956
child 509821 5b395d1d3585f20579c098cd000adfe26eb84831
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersewright
bugs1495207
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright 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
@@ -818,23 +818,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.
@@ -848,16 +841,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);
 
@@ -870,31 +883,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.strings.disabledTooltipText);
       this.shieldHistogramAdd(1);
     } else if (anyBlocking) {
       this.iconBox.setAttribute("tooltiptext", this.strings.activeTooltipText);
       this.shieldHistogramAdd(2);
     } else {
       this.iconBox.removeAttribute("tooltiptext");