Issue 6031 - Implement Acceptable Ads notification

desktop-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 10 matching lines @@
 "use strict";
 
 {
   let subscriptionsMap = Object.create(null);
   let filtersMap = Object.create(null);
   let collections = Object.create(null);
   let acceptableAdsUrl = null;
   let acceptableAdsPrivacyUrl = null;
   let isCustomFiltersLoaded = false;
   let {getMessage} = browser.i18n;
   let {setElementText} = ext.i18n;

[saroyanm, 2017/11/17 16:10:39]

Didin't we convert all ext. to browser. ?
I'm not sure if this is a correct use.

[Thomas Greiner, 2017/11/20 18:36:05]

No, we merely got rid of the abstraction layer for Web Extension APIs. This
method, however, is not part of that.

Alternatively, you could call it as `ext.i18n.setElementText()` if you don't
like doing this import.

[saroyanm, 2017/11/21 14:58:32]

I like, import here, as it's somehow consistent with other "{getMessage}"
import.

   let customFilters = [];
   let filterErrors = new Map([
     ["synchronize_invalid_url",
      "options_filterList_lastDownload_invalidURL"],
     ["synchronize_connection_error",
      "options_filterList_lastDownload_connectionError"],
     ["synchronize_invalid_data",
      "options_filterList_lastDownload_invalidData"],
     ["synchronize_checksum_mismatch",
      "options_filterList_lastDownload_checksumMismatch"]
@@ skipping 859 matching lines @@
 
     // General tab
     getDocLink("contribute", (link) =>
     {
       E("contribute").href = link;
     });
     getDocLink("acceptable_ads_criteria", (link) =>
     {
       setLinks("enable-acceptable-ads-description", link);
     });
     setElementText(E("tracking-warning-1"), "options_tracking_warning_1", 

[saroyanm, 2017/11/17 16:10:39]

Ideally I think we should be able to specify "translatable" arguments somehow in
the HTML data ?

Maybe <p data-i18n="options_tracking_warning_1"
data-args="common_feature_privacy_title,options_acceptableAds_ads_label"></p>

If you agree, I'll create another ticket to handle that, or can handle in this
ticket as well, depending how urgent are this changes.

[Thomas Greiner, 2017/11/20 18:36:05]

Let's wait for further cases where we'd need that. Because at this point it
wouldn't add any value since this is the only part in the code where we use it.

[saroyanm, 2017/11/21 14:58:32]

Agree.

       [getMessage("common_feature_privacy_title"),
       getMessage("options_acceptableAds_ads_label")]);
     setElementText(E("tracking-warning-3"), "options_tracking_warning_3", 
       [getMessage("options_acceptableAds_privacy_label")]);
 
     getDocLink("privacy_friendly_ads", (link) =>
     {
       E("enable-acceptable-ads-privacy-description").href = link;
     });
     getDocLink("adblock_plus_{browser}_dnt", url =>
@@ skipping 447 matching lines @@
     switch (key)
     {
       case "notifications_ignoredcategories":
         value = value.indexOf("*") == -1;
         break;
 
       case "notifications_showui":
         hidePref("notifications_ignoredcategories", !value);
         break;
       case "ui_warn_tracking":
-        if (!value)

[saroyanm, 2017/11/17 16:45:14]

Does it make sense to check against value == false ?
In case the preferences gets corrupted, or doesn't load ?

[Thomas Greiner, 2017/11/20 18:36:05]

I'd assume not. If we cannot load the setting, we should get its default value
as defined in lib/prefs.js.

It would help in case we accidentally set the preference value to `undefined`,
`null` or `""` but then we'd need to use `value === false` since `"" == false`
and `[] == false` both evaluate to `true`.

[saroyanm, 2017/11/21 14:58:32]

I see an issue with strict equation(not sure if that might be case), when the
Browser passes non Boolean value (ex. String) ?

I think we can leave it as it is right now.

[Thomas Greiner, 2017/11/21 17:17:29]

I don't understand what you mean but I'm fine with relying on truthy and falsy
values for now since we do the same for "notifications_showui" above.

-          E("acceptable-ads").classList.remove("show-warning");

[Thomas Greiner, 2017/11/20 18:36:05]

What if the value changes to `true`? We'd probably want to reuse
`hasPrivacyConflict()` here again in that case.

While this is currently not possible via the UI, it could be useful for testing
and it could help avoid issues with this in the future.

[saroyanm, 2017/11/21 14:58:32]

Agree. Done.

+        let showWarning = (value && hasPrivacyConflict());
+        E("acceptable-ads").classList.toggle("show-warning", showWarning);
         break;
     }
 
     let checkbox = document.querySelector(
       "[data-pref='" + key + "'] button[role='checkbox']"
     );
     if (checkbox)
       checkbox.setAttribute("aria-checked", value);
   }
 
@@ skipping 67 matching lines @@
   });
   browser.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }