[adblockplusui] Issue 4915 - Define ext for background pages in antiadblockInit.js

lib/antiadblockInit.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 let {Utils} = require("utils");
 let {Prefs} = require("prefs");
 let {ActiveFilter} = require("filterClasses");
 let {FilterStorage} = require("filterStorage");
 let {FilterNotifier} = require("filterNotifier");
 let {Subscription} = require("subscriptionClasses");
 let {Notification} = require("notification");
 
-let ext = typeof window != "undefined" &&

[kzar, 2017/03/02 09:36:30]

I'm not sure this is right, since if ext is already declared using let  then
redeclaring it here will throw an exception. Also are we expecting this code to
run somewhere where window doesn't exist?

I also wasn't sure about the best way to do this, see my attempt here
https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js

Thomas what do you think?

[Wladimir Palant, 2017/03/02 11:14:39]

This part is unproblematic, it's a module which means that it always has its own
scope - this code isn't running in global scope and not conflicting with the
globally defined ext variable.
Yes, we do. Currently Firefox, in future maybe libadblockplus and more.
See my comment in this review as to why this approach is a bad one. Too bad that
Sebastian felt that he had to obfuscate my suggestion by misusing logical
operators.

[Thomas Greiner, 2017/03/02 11:41:16]

I assume the confusion here comes from the fact that only some files in the
adblockplusui repository are actual modules.

It'd probably help if we turn the other files in this repo into modules as well.
While I find it personally quite convenient to use `let foo = foo || "foo";` in
certain situations, I do agree with Wladimir that we're overdoing it here in
this case.

Is there any reason not to go with the below variant?

let ext;
if (typeof window != "undefined" && window.ext)
  ext = window.ext; // although ESLint probably forces us to use destructuring
here
else
  ext = require("ext_background");

[wspee, 2017/03/03 09:25:21]

Done. Any objections?

-  window.ext || require("ext_background");
+let ext;
+if (typeof window != "undefined" && window.ext)
+  ext = window.ext;
+else
+  ext = require("ext_background");
 
 exports.initAntiAdblockNotification = function initAntiAdblockNotification()
 {
   let notification = {
     id: "antiadblock",
     type: "question",
     title: ext.i18n.getMessage("notification_antiadblock_title"),
     message: ext.i18n.getMessage("notification_antiadblock_message"),
     urlFilters: []
   };
@@ skipping 44 matching lines @@
     if (url in FilterStorage.knownSubscriptions && !subscription.disabled)
       addAntiAdblockNotification(subscription);
     else
       removeAntiAdblockNotification();
   }
 
   FilterNotifier.on("subscription.updated", onSubscriptionChange);
   FilterNotifier.on("subscription.removed", onSubscriptionChange);
   FilterNotifier.on("subscription.disabled", onSubscriptionChange);
 }