Issue 3334 - Fix warnings spotted during testing before release

Issue 3334 - Fix warnings spotted during testing before release

[kzar, 2015-11-17 16:46:17]

Patch Set 1

[Sebastian Noack, 2015-11-17 17:02:58]

https://codereview.adblockplus.org/29330321/diff/29330322/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29330321/diff/29330322/lib/whitelisting.js...
lib/whitelisting.js:81: var getKey = exports.getKey = function(page, frame)
This will break jsdoc. Instead use:

let getKey =
/*
 *
 */
exports.getKey = function () {};

Also note that the let statement is more appropriate here.

[Wladimir Palant, 2015-11-17 18:58:43]

https://codereview.adblockplus.org/29330321/diff/29330322/webrequest.js
File webrequest.js (right):

https://codereview.adblockplus.org/29330321/diff/29330322/webrequest.js#newco...
webrequest.js:26: var extractHostFromFrame = url.extractHostFromFrame;
I don't think that changes to this file are a good idea a week before a release.
As I explained in the issue, all these variables are declared and defined, in
background.js. Structuring the imports in a better way might be a good thing to
do but unless you have an actual issue that needs to be fixed - after the
release please.

[kzar, 2015-11-17 19:06:27]

Patch Set 2 : Avoid breaking jsdoc
Patch Set 3 : Remove changes to webrequest.js

https://codereview.adblockplus.org/29330321/diff/29330322/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29330321/diff/29330322/lib/whitelisting.js...
lib/whitelisting.js:81: var getKey = exports.getKey = function(page, frame)
On 2015/11/17 17:02:58, Sebastian Noack wrote:
> This will break jsdoc. Instead use:
> 
> let getKey =
> /*
>  *
>  */
> exports.getKey = function () {};
> 
> Also note that the let statement is more appropriate here.

Done.

https://codereview.adblockplus.org/29330321/diff/29330322/webrequest.js
File webrequest.js (right):

https://codereview.adblockplus.org/29330321/diff/29330322/webrequest.js#newco...
webrequest.js:26: var extractHostFromFrame = url.extractHostFromFrame;
On 2015/11/17 18:58:42, Wladimir Palant wrote:
> I don't think that changes to this file are a good idea a week before a
release.
> As I explained in the issue, all these variables are declared and defined, in
> background.js. Structuring the imports in a better way might be a good thing
to
> do but unless you have an actual issue that needs to be fixed - after the
> release please.

Done.

[Sebastian Noack, 2015-11-17 20:39:44]

https://codereview.adblockplus.org/29330321/diff/29330339/metadata.common
File metadata.common (right):

https://codereview.adblockplus.org/29330321/diff/29330339/metadata.common#new...
metadata.common:48: skin/abp-128.png = adblockplusui/skin/abp-128.png
We have following code in firstRun.js specifically to avoid bundling this image
twice.

    logo.src = "skin/abp-128.png";
    var errorCallback = function()
    {
      logo.removeEventListener("error", errorCallback, false);
      // We are probably in Chrome/Opera/Safari, the image has a different path.
      logo.src = "icons/detailed/abp-128.png";
    };
    logo.addEventListener("error", errorCallback, false);

Apparently, Chrome is still showing an error in the console, for failed requests
regardless whether the error has been handled. But that doesn't justify bundling
resources redundantly.

[kzar, 2015-11-17 21:04:15]

Patch Set 4 : Remove redundant mapping for the large icon

https://codereview.adblockplus.org/29330321/diff/29330339/metadata.common
File metadata.common (right):

https://codereview.adblockplus.org/29330321/diff/29330339/metadata.common#new...
metadata.common:48: skin/abp-128.png = adblockplusui/skin/abp-128.png
On 2015/11/17 20:39:43, Sebastian Noack wrote:
> We have following code in firstRun.js specifically to avoid bundling this
image
> twice.
> 
>     logo.src = "skin/abp-128.png";
>     var errorCallback = function()
>     {
>       logo.removeEventListener("error", errorCallback, false);
>       // We are probably in Chrome/Opera/Safari, the image has a different
path.
>       logo.src = "icons/detailed/abp-128.png";
>     };
>     logo.addEventListener("error", errorCallback, false);
> 
> Apparently, Chrome is still showing an error in the console, for failed
requests
> regardless whether the error has been handled. But that doesn't justify
bundling
> resources redundantly.

Whoops OK, that makes sense. Done.

[Sebastian Noack, 2015-11-17 21:11:38]

LGTM and good to merge before the release.

[Wladimir Palant, 2015-11-18 09:49:20]

LGTM