Issue 3687 - Add experimental support for Safari content blockers

options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 123 matching lines @@
   // Display jQuery UI elements
   $("#tabs").tabs();
   $("button").button();
   $(".refreshButton").button("option", "icons", {primary: "ui-icon-refresh"});
   $(".addButton").button("option", "icons", {primary: "ui-icon-plus"});
   $(".removeButton").button("option", "icons", {primary: "ui-icon-minus"});
 
   // Popuplate option checkboxes
   initCheckbox("shouldShowBlockElementMenu");
   initCheckbox("show_devtools_panel");
-  initCheckbox("shouldShowNotifications", {
-    key: "notifications_ignoredcategories",
-    get: function(ignoredcategories)
-    {
-      return ignoredcategories.indexOf("*") == -1;
-    }
-  });
-  initCheckbox("safariContentBlocker", {onChange: function(checkbox)
-  {
-    var restartMessage = document.getElementById("restart-safari");
-    restartMessage.hidden = true;
-
-    // When the user has chosen to use the legacy APIs but Safari has disabled
-    // them we need to show a "Please restart Safari" message.
-    if (!checkbox.checked)

[Sebastian Noack, 2016/05/12 11:12:22]

As discussed with Thomas on the review for the related change to the new options
page, we explictily decided against persistent logic and agreed that it's
sufficient to notify the user only once when he disabled content blockers.

If we want to persistently notify the user about this state, the options page is
the wrong place to do so. But given that disabling an experimental feature that
have previously be enabled on Safari can be considered edge case, notifying the
user once should be sufficient.

[kzar, 2016/05/17 15:15:38]

I don't understand what you mean by "persistent logic" here.

The behaviour I've implemented displays a red "Please restart Safari" message
beside the content blocking option when necessary and it handles the various
edge-cases pretty well. (For example what if the user clicks to enable content
blocking but there's an exception, the old API isn't disabled and the content
blocking feature is automatically disabled.)

[Sebastian Noack, 2016/05/17 18:35:24]

At the very least this logic is inconsistent with the new options page, where we
agreed to ignore edge cases. But fine with me, as long as you keep the new
options page in sync with the logic here.

[kzar, 2016/05/17 19:20:21]

Acknowledged.

-    {
-      ext.backgroundPage.sendMessage({type: "safari.contentBlockingActive"},
-      function (contentBlockingActive)
-      {
-        if (contentBlockingActive)
-          restartMessage.hidden = false;
-      });
-    }
-  }});
+  initCheckbox("shouldShowNotifications", "notifications_ignoredcategories");
+  initCheckbox("safariContentBlocker");
 
   getInfo("features", function(features)
   {
     if (!features.devToolsPanel)
       document.getElementById("showDevtoolsPanelContainer").hidden = true;
 
     // Only show the option for Safari content blocking API if the user is
     // running Safari and both the legacy and content blocking APIs are
     // available.
     document.getElementById("safariContentBlockerContainer").hidden = !(
@@ skipping 80 matching lines @@
   getSubscriptions(false, true, function(subscriptions)
   {
     clearListBox("userFiltersBox");
     clearListBox("excludedDomainsBox");
 
     for (var i = 0; i < subscriptions.length; i++)
       convertSpecialSubscription(subscriptions[i]);
   });
 }
 
-function initCheckbox(id, descriptor)
-{
+function initCheckbox(id, key)
+{
+  key = key || id;
   var checkbox = document.getElementById(id);
-  var key = descriptor && descriptor.key || id;
-  var onChange;
-  if (descriptor && descriptor.onChange)
-    onChange = descriptor.onChange.bind(undefined, checkbox);
+
   getPref(key, function(value)
   {
-    if (descriptor && descriptor.get)
-      checkbox.checked = descriptor.get(value);
-    else
-      checkbox.checked = value;
-
-    if (onChange)
-      onChange();
+    onPrefMessage(key, value);
   });
 
   checkbox.addEventListener("click", function()
   {
     togglePref(key);
   }, false);
-
-  if (onChange)
-    checkbox.addEventListener("change", onChange);
 }
 
 function loadRecommendations()
 {
   fetch("subscriptions.xml")
     .then(function(response)
     {
       return response.text();
     })
     .then(function(text)
@@ skipping 225 matching lines @@
 {
   switch (key)
   {
     case "notifications_showui":
       document.getElementById("shouldShowNotificationsContainer").hidden = !value;
       return;
     case "notifications_ignoredcategories":
       key = "shouldShowNotifications";
       value = value.indexOf("*") == -1;
       break;
-  }
-
+    case "safariContentBlocker":
+      var restartMessage = document.getElementById("restart-safari");
+      restartMessage.hidden = true;
+      // When the user has chosen to use the legacy APIs but Safari has disabled
+      // them we need to show a "Please restart Safari" message.
+      if (!value)
+      {
+        ext.backgroundPage.sendMessage({type: "safari.contentBlockingActive"},
+        function(contentBlockingActive)
+        {
+          if (contentBlockingActive)
+            restartMessage.hidden = false;
+        });
+      }
+  }
   var checkbox = document.getElementById(key);
   if (checkbox)
-  {
     checkbox.checked = value;
-    // Apparently modifying the checked attribute for a checkbox does not

[Sebastian Noack, 2016/05/12 11:12:23]

If you want to emulate user actions that's what the click() method is for.
However, it would be better to call the change handler explicitly here.

[kzar, 2016/05/17 15:15:37]

Calling .click() would also toggle the checkbox right? (I guess I could move the
checkbox.checked assignment below to force the correct state but it seems kind
of pointless.)

As for calling the change handler directly here, sounds good to me except I
can't see an easy way to get to the handler here. Maybe I'm missing something?
(I guess we could assign an extra property to the element, or assign onchange
instead of using addEventListener but both of those seem maybe worse than just
dispatching the change event.) 

[Sebastian Noack, 2016/05/17 18:35:25]

I wonder whether the change listener is even necessary. Why don't you simply
move the related logic here? If the preference is changed the code here is
triggered anyway.

[kzar, 2016/05/17 19:20:21]

We also need to call the logic when the checkbox is first initialized. Even if
we moved the logic into a separate function and called it here we would struggle
to call it when the checkbox is initialized. (We would have to add a key like
`afterInit` to initCheckbox and pass the function there too. Hardly seems less
work than the existing approach.)

[Sebastian Noack, 2016/05/18 07:03:29]

Well, in the new options page we call the same function that is called on
preference change also when initially retrieving the preferences in order to
initialize the checkboxes. Couldn't you do the same here?

[kzar, 2016/05/18 08:02:48]

Yes, I already do that here.

[Sebastian Noack, 2016/05/18 08:13:37]

But why can't you put the logic simply here then instead of passing a callback
to initCheckbox() and emulating a "change" even here? Note that we already have
logic that depends on the preference name above.

[kzar, 2016/05/18 09:28:47]

Oh I finally understand, I think you mean that we should call `onPrefMessage`
when initializing the checkboxes. I misunderstood, I thought by "the same
function" you meant `onChange`. We don't call the `onPrefMessage` when the
checkboxes are initialized so far.

Well I really think the change you suggest here would make the code more
confusing without any real benefit. It would be less clear in edge cases what
should happen in `onPrefMessage`, for example with normalizing the key and value
for the "notifications_ignoredcategories" preference. (Note that logic that
depends on the preference name as it stands adjusts the preference name in some
cases, so we can't rely on the name until afterwards.)

It would also add slightly to the delay after clicking the checkbox before the
"safari.contentBlockingActive" message comes back and the "Please restart
Safari" message is shown. As it is we immediately send that message when the
user clicks, instead of waiting for the `prefs.toggle` message to be sent and
then for the `prefs.respond` message to come back in.

[Sebastian Noack, 2016/05/18 10:07:24]

I don't see how that makes anything more confusing. In fact, I think the way you
have it implemented now is quite confusing. You introduced yet another method to
respond to preference changes, which however is by design only triggered by user
gesture, and then you work around that by emulating user gesture. That is
certainly backwards.

[kzar, 2016/05/18 10:43:40]

Done.

-    // dispatch the change event automatically...
-    checkbox.dispatchEvent(new Event("change"));
-  }
 }
 
 function onFilterMessage(action, filter)
 {
   switch (action)
   {
     case "loaded":
       reloadFilters();
       break;
     case "added":
@@ skipping 229 matching lines @@
       onFilterMessage(message.action, message.args[0]);
       break;
     case "prefs.respond":
       onPrefMessage(message.action, message.args[0]);
       break;
     case "subscriptions.respond":
       onSubscriptionMessage(message.action, message.args[0]);
       break;
   }
 });