Issue 3208 - Don`t use numerical content types outside nsIContentPolicy.shouldLoad

Content policy and pop-up blocker tests pass with that change, blockable items work correctly.

[Wladimir Palant, 2015-10-29 12:26:36]

Uploaded new patchset which removes TYPE_POPUP and TYPE_ELEMHIDE constants -
these aren't actually being used (these were required in the big patch because
the split there was slightly different). I also fixed an unrelated issue in the
filter composer (bug 1202902 regression, inline event handler unable to access
"global" variables) which prevented me from verifying that the filter composer
indeed works correctly with these changes.

[Thomas Greiner, 2015-11-02 18:54:37]

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
File chrome/content/ui/composer.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:20: var advancedMode = false;
How is this change related to the issue?

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:123: types.push(type);
Detail: What about writing `let types =
Array.from(Policy.localizedDescr.keys());` instead? It more clearly communicates
the intent behind those three lines and squashes them into a single one.

https://codereview.adblockplus.org/29329450/diff/29329460/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/lib/contentPolicy.j...
lib/contentPolicy.js:60: * @type Map
Detail: Since we've already been specifying the types of arrays (e.g.
`string[]`), it would make sense doing the same for `Set` and `Map` as well
(e.g. `Map.<number, string>`).

https://codereview.adblockplus.org/29329450/diff/29329460/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/lib/requestNotifier...
lib/requestNotifier.js:218: RequestNotifier.addNodeData = function(/**Node*/
node, /**Window*/ topWnd, /**String*/ contentType, /**String*/ docDomain,
/**Boolean*/ thirdParty, /**String*/ location, /**Filter*/ filter)
Detail: The inline documentation here is highly redundant.

[Wladimir Palant, 2015-11-03 11:26:49]

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
File chrome/content/ui/composer.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:20: var advancedMode = false;
On 2015/11/02 18:54:36, Thomas Greiner wrote:
> How is this change related to the issue?

It is only related to testing here. We have an event handler in the dialog
expecting advancedMode to be global, switching to Advanced Mode isn't possible
if it isn't (that's https://bugzilla.mozilla.org/show_bug.cgi?id=1202902
striking again).

Let me know if you prefer this to be fixed separately.

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:123: types.push(type);
On 2015/11/02 18:54:36, Thomas Greiner wrote:
> Detail: What about writing `let types =
> Array.from(Policy.localizedDescr.keys());` instead? It more clearly
communicates
> the intent behind those three lines and squashes them into a single one.

Nice one, I didn't know about Array.from() yet. However, the way this code is
changed in https://codereview.adblockplus.org/29329473/ I don't think that this
makes sense.

https://codereview.adblockplus.org/29329450/diff/29329460/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/lib/contentPolicy.j...
lib/contentPolicy.js:60: * @type Map
On 2015/11/02 18:54:36, Thomas Greiner wrote:
> Detail: Since we've already been specifying the types of arrays (e.g.
> `string[]`), it would make sense doing the same for `Set` and `Map` as well
> (e.g. `Map.<number, string>`).

True, will improve JSDoc comments in a separate commit.

https://codereview.adblockplus.org/29329450/diff/29329460/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/lib/requestNotifier...
lib/requestNotifier.js:218: RequestNotifier.addNodeData = function(/**Node*/
node, /**Window*/ topWnd, /**String*/ contentType, /**String*/ docDomain,
/**Boolean*/ thirdParty, /**String*/ location, /**Filter*/ filter)
True, this aspect of my original patch got lost. Will improve JSDoc comments in
a separate commit.

[Thomas Greiner, 2015-11-03 11:54:54]

LGTM

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
File chrome/content/ui/composer.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:20: var advancedMode = false;
On 2015/11/03 11:26:49, Wladimir Palant wrote:
> On 2015/11/02 18:54:36, Thomas Greiner wrote:
> > How is this change related to the issue?
> 
> It is only related to testing here. We have an event handler in the dialog
> expecting advancedMode to be global, switching to Advanced Mode isn't possible
> if it isn't (that's https://bugzilla.mozilla.org/show_bug.cgi?id=1202902
> striking again).
> 
> Let me know if you prefer this to be fixed separately.

I see. In that case fine to fix it separately.

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:123: types.push(type);
On 2015/11/03 11:26:49, Wladimir Palant wrote:
> On 2015/11/02 18:54:36, Thomas Greiner wrote:
> > Detail: What about writing `let types =
> > Array.from(Policy.localizedDescr.keys());` instead? It more clearly
> communicates
> > the intent behind those three lines and squashes them into a single one.
> 
> Nice one, I didn't know about Array.from() yet. However, the way this code is
> changed in https://codereview.adblockplus.org/29329473/ I don't think that
this
> makes sense.

I agree that, with that change in mind, it's not necessary here.

https://codereview.adblockplus.org/29329450/diff/29329460/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/lib/contentPolicy.j...
lib/contentPolicy.js:60: * @type Map
On 2015/11/03 11:26:49, Wladimir Palant wrote:
> On 2015/11/02 18:54:36, Thomas Greiner wrote:
> > Detail: Since we've already been specifying the types of arrays (e.g.
> > `string[]`), it would make sense doing the same for `Set` and `Map` as well
> > (e.g. `Map.<number, string>`).
> 
> True, will improve JSDoc comments in a separate commit.

Great, thanks.

[Wladimir Palant, 2015-11-04 09:30:42]

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
File chrome/content/ui/composer.js (right):

https://codereview.adblockplus.org/29329450/diff/29329460/chrome/content/ui/c...
chrome/content/ui/composer.js:20: var advancedMode = false;
On 2015/11/03 11:54:54, Thomas Greiner wrote:
> I see. In that case fine to fix it separately.

Ok, I removed that change here and created
https://issues.adblockplus.org/ticket/3263. All the other "changes" displayed by
the interdiff are due to rebasing.