Issue 3880 - Improve behavior of Safari content blocker option

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -718,7 +718,14 @@
     function(features)
     {
       hidePref("show_devtools_panel", !features.devToolsPanel);
-      hidePref("safari_contentblocker", !features.safariContentBlocker);
+
+      // Only show option to switch between Safari Content Blockers
+      // and event based blocking if both are available.
+      hidePref("safari_contentblocker", !(
+        features.safariContentBlocker &&
+        "canLoad" in safari.self.tab &&
+        "onbeforeload" in Element.prototype
+      ));
     });
 
     var filterTextbox = document.querySelector("#custom-filters-add input");
@@ -1080,13 +1087,18 @@
     switch (message.type)
     {
       case "app.respond":
-        if (message.action == "addSubscription")
+        switch (message.action)
         {
-          var subscription = message.args[0];
-          var dialog = E("dialog-content-predefined");
-          dialog.querySelector("h3").textContent = subscription.title || "";
-          dialog.querySelector(".url").textContent = subscription.url;
-          openDialog("predefined");
+          case "addSubscription":
+            var subscription = message.args[0];
+            var dialog = E("dialog-content-predefined");
+            dialog.querySelector("h3").textContent = subscription.title || "";
+            dialog.querySelector(".url").textContent = subscription.url;
+            openDialog("predefined");
+            break;
+          case "safariRestartRequired":
+            E("restart-safari").setAttribute("aria-hidden", !message.args[0]);

[Thomas Greiner, 2016/03/31 14:24:57]

Why do we even need the background page for that? The options page already gets
notified whenever the preference changes so it can do the check itself.

[Sebastian Noack, 2016/03/31 14:45:37]

We should only indicate that restarting Safari is necessary, if Content Blockers
got disabled after the extension got loaded. However, without any persistence in
the background page we cannot distinguish whether it has been disabled since the
last restart, or whether it was already disabled when the extension was loaded.

[Sebastian Noack, 2016/03/31 15:14:31]

Frankly, I don't have a strong opinion here. Patch set #3 removed the persistent
logic. Hence when reloading or opening a new instance of the options page we
don't indicate to restart Safari, anymore. I'm not sure which is better. Let me
know what you think.

[Thomas Greiner, 2016/03/31 16:29:17]

I do agree that we should somehow indicate to the user that Adblock Plus is
broken until they restart Safari since that's quite significant. It might be
more effective to do that elsewhere though (e.g. in the icon popup?). What do
you think?

Not sure how common or helpful it might be considering that the user already
ignored the message and closed the options page but, in general, I see your
point.

[Sebastian Noack, 2016/03/31 17:09:24]

FWIW, I just begun to prefer the new patch set without the persistence, only
showing a message to restart Safari once after Content Blockers got disabled, if
it's just because it's way simpler. Also, AFAIK, that is what Dave has
implemented for the old options page.

Please let's not overthink the problem for now. We are talking about an edge
case, that is Safari 9 users, that use the options page to disable an
experimental feature they have previously turned on. And even those will restart
Safari sooner or later anyway, and everything will be fine again. Not to mention
that even before the new options page is release, Safari 10 might be released
and might not have the old mechanism anyway anymore.

[Thomas Greiner, 2016/03/31 17:17:06]

You're right.

+            break;
         }
         break;
       case "filters.respond":
@@ -1104,7 +1116,7 @@
   ext.backgroundPage.sendMessage(
   {
     type: "app.listen",
-    filter: ["addSubscription"]
+    filter: ["addSubscription", "safariRestartRequired"]
   });
   ext.backgroundPage.sendMessage(
   {