Added Safari Support

Added Safari Support

[Felix Dahlke, 2013-10-24 16:30:34]

Not finished with the review yet, but I guess there are a few things to do
already. One thing I've seen in many places is that opening braces should go on
their own line (except for object literals).

http://codereview.adblockplus.org/16067002/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode116
background.js:116: if(!/^https?:/.test(tab.url))
What if tab is null or undefined? Can that still happen?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode122
background.js:122: if (window.safari || !excluded)
Why do we always show the normal icon on Safari? Definitely worth a comment.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode139
background.js:139: if (window.chrome) // TODO: Implement context menus for
Safari
The surrounding |if (excluded)| needs braces, otherwise the else below will be
for |if (window.chrome)|.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode370
background.js:370: ext.windows.getLastFocused(function(win) {
Opening braces always go on their own line, except for object literals, where
that won't work.

The same goes for all the code below, but it's easier if I only point this out
once I guess.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode421
background.js:421: function openOptions(callback)
Why remove the docs?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode482
background.js:482: tabs.forEach(this._updateIcon.bind(this));
Is the |bind(this)| actually necessary here?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode507
background.js:507: var n = windows.length;
I'd love a more descriptive variable name here. Something like |visibleWindows|
maybe?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode511
background.js:511: if (tabs.length == --n)
tabs.length will always be 0 here, won't it? In that case I'd prefer |if (--n ==
0)| for clarity.

Then again, it seems to me like this all this can be simplified. Comparing
tabs.length and n is, how I see it, the exit condition, so we might as well move
it after the loop. Once we do that, we don't need to count visible windows
anymore, so I guess this would do:

windows.forEach(function(window)
{
  if (window.visible)
    window.getActiveTab(tabs.push);
});
callback(tabs);

Or did I miss something?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode544
background.js:544: var iconAnimation = new IconAnimation();
If there's only one instance of IconAnimation, I'd prefer a JavaScript module.
Might be worth it's own file actually.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode594
background.js:594: if (!isFrameWhitelisted(sender.tab, frameId, "DOCUMENT"))
I prefer the old code (enabled variable) to nested ifs here.

http://codereview.adblockplus.org/16067002/diff/1/block.js
File block.js (right):

http://codereview.adblockplus.org/16067002/diff/1/block.js#newcode32
block.js:32: ext.backgroundPage.sendMessage({
Opening braces should go on their own line, likewise below.

http://codereview.adblockplus.org/16067002/diff/1/chrome/background.js
File chrome/background.js (right):

http://codereview.adblockplus.org/16067002/diff/1/chrome/background.js#newcode1
chrome/background.js:1: (function() {
Opening braces should go on their own line, likewise below.

http://codereview.adblockplus.org/16067002/diff/1/chrome/background.js#newcode60
chrome/background.js:60: // copy listeners before calling them, because of they
s/because of/because/

http://codereview.adblockplus.org/16067002/diff/1/chrome/background.js#newcode62
chrome/background.js:62: // taken into account before the next occurrance of the
event
s/occureace/occurence/

http://codereview.adblockplus.org/16067002/diff/60001/chrome/common.js
File chrome/common.js (right):

http://codereview.adblockplus.org/16067002/diff/60001/chrome/common.js#newcode19
chrome/common.js:19: /* Events */
As in other places, opening braces should go on their own line.

http://codereview.adblockplus.org/16067002/diff/60001/chrome/common.js#newcode31
chrome/common.js:31: var extaArgs = Array.prototype.slice.call(arguments, 1);
s/extaArgs/extraArgs/?

[Sebastian Noack, 2013-10-25 09:55:02]

http://codereview.adblockplus.org/16067002/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode116
background.js:116: if(!/^https?:/.test(tab.url))
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> What if tab is null or undefined? Can that still happen?

Did you had a look at the calling code? Maybe I missed something, but I think
refreshIconAndContextMenu() is always called with a tab.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode122
background.js:122: if (window.safari || !excluded)
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> Why do we always show the normal icon on Safari? Definitely worth a comment.

First of all because of in Safari icons are global and not per page. Page
actions like in Chrome doesn't exist in Safari. Also the "whitelisted" icon is
more or less just a grayscale version of the regular icon. And because of icons
in safari are always grayscale, it would be hard to recognize it anyway. Sure, I
could add a comment about that.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode139
background.js:139: if (window.chrome) // TODO: Implement context menus for
Safari
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> The surrounding |if (excluded)| needs braces, otherwise the else below will be
> for |if (window.chrome)|.

Oh, you are right.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode370
background.js:370: ext.windows.getLastFocused(function(win) {
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> Opening braces always go on their own line, except for object literals, where
> that won't work.
> 
> The same goes for all the code below, but it's easier if I only point this out
> once I guess.

I thought we go with Mozilla coding style?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode421
background.js:421: function openOptions(callback)
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> Why remove the docs?

The old docs are deprecated now (the callback is called with the tab, not its
window object, now), and I felt that they are unneeded, since the code below is
pretty simple and easy to understand. Should I add docs that describe the new
behavior of openOptions()?

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode482
background.js:482: tabs.forEach(this._updateIcon.bind(this));
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> Is the |bind(this)| actually necessary here?

Yes, it is. _updateIcon() needs this._step and this._severity. And if I don't
bind it, "this" will be the window, and not the IconAnimation object.

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode511
background.js:511: if (tabs.length == --n)
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> tabs.length will always be 0 here, won't it? In that case I'd prefer |if (--n
==
> 0)| for clarity.

Yes it is. When writing the code I felt it would be more clear that way. But now
I think checking against 0 would actually more clear.

> Then again, it seems to me like this all this can be simplified. Comparing
> tabs.length and n is, how I see it, the exit condition, so we might as well
move
> it after the loop. Once we do that, we don't need to count visible windows
> anymore, so I guess this would do:
> 
> windows.forEach(function(window)
> {
>   if (window.visible)
>     window.getActiveTab(tabs.push);
> });
> callback(tabs);
> 
> Or did I miss something?

That wouldn't work. The only reason I'm using callbacks here is that they are
called asynchronously (on Chrome). So tabs would always be an empty array in
your case, as callback(tabs) is called before the active tabs were actually
retrieved.

So the callback passed to _getVisibleTabs must actually be called from within
the callback passed getActiveTab(), but only if we know that it was called for
the last active window, and tabs is fully populated.

http://codereview.adblockplus.org/16067002/diff/1/block.js
File block.js (right):

http://codereview.adblockplus.org/16067002/diff/1/block.js#newcode32
block.js:32: ext.backgroundPage.sendMessage({
On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> Opening braces should go on their own line, likewise below.

I though except for object literals, as we have here?

[Felix Dahlke, 2013-10-25 14:43:08]

http://codereview.adblockplus.org/16067002/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode370
background.js:370: ext.windows.getLastFocused(function(win) {
On 2013/10/25 09:55:03, sebastian wrote:
> On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> > Opening braces always go on their own line, except for object literals,
where
> > that won't work.
> > 
> > The same goes for all the code below, but it's easier if I only point this
out
> > once I guess.
> 
> I thought we go with Mozilla coding style?

No, we have our own: https://adblockplus.org/en/coding-style

It's largely based on Mozilla's though. Personally not a fan of opening braces
on new lines in JS, but that's how it is.

http://codereview.adblockplus.org/16067002/diff/1/block.js
File block.js (right):

http://codereview.adblockplus.org/16067002/diff/1/block.js#newcode32
block.js:32: ext.backgroundPage.sendMessage({
On 2013/10/25 09:55:03, sebastian wrote:
> On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> > Opening braces should go on their own line, likewise below.
> 
> I though except for object literals, as we have here?

Um, yes, nevermind :)

[Wladimir Palant, 2013-10-31 14:07:06]

I've only looked at the locale changes so far.

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
File _locales/en_US/messages.json (right):

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
_locales/en_US/messages.json:32: "description_chrome": {
While we are at it, please add "Must not be more than 132 characters wide"
description comment here.

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
_locales/en_US/messages.json:39: "description": "Must not be more than 100
characters wide",
Wrong indentation here: tab used instead of spaces?

Could you check whether that's really the limitation? Getting translators to
observe it will be tricky.

[Sebastian Noack, 2013-10-31 14:40:37]

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
File _locales/en_US/messages.json (right):

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
_locales/en_US/messages.json:39: "description": "Must not be more than 100
characters wide",
On 2013/10/31 14:07:06, Wladimir Palant wrote:
> Wrong indentation here: tab used instead of spaces?
> 
> Could you check whether that's really the limitation? Getting translators to
> observe it will be tricky.

The extension builder shows a warning, saying that it will be truncated, if
longer than 100 chars. But at least under Preferences->Extensions, the
description isn't truncated. There might still be other places (I don't know any
at the moment) where the description is shown too, and will be truncated.
However I have already uploaded a new version that adds the full description
again and removes that comment, as discussed in IRC.

[Wladimir Palant, 2013-10-31 14:42:13]

LGTM on the messages.json changes, feel free to commit these along with your
buildtools changes.

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
File _locales/en_US/messages.json (right):

http://codereview.adblockplus.org/16067002/diff/300001/_locales/en_US/message...
_locales/en_US/messages.json:32: "description_chrome": {
On 2013/10/31 14:07:06, Wladimir Palant wrote:
> While we are at it, please add "Must not be more than 132 characters wide"
> description comment here.

Feel free to address this comment.

[Felix Dahlke, 2013-11-10 01:07:00]

Done with the review, mostly just style issues.

Note that I somewhat focused on the parts relevant for Chrome/Opera, less on the
Safari-specific stuff.

http://codereview.adblockplus.org/16067002/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode421
background.js:421: function openOptions(callback)
On 2013/10/25 09:55:03, sebastian wrote:
> On 2013/10/24 16:30:34, Felix H. Dahlke wrote:
> > Why remove the docs?
> 
> The old docs are deprecated now (the callback is called with the tab, not its
> window object, now), and I felt that they are unneeded, since the code below
is
> pretty simple and easy to understand. Should I add docs that describe the new
> behavior of openOptions()?

Yeah I guess it's fine, most functions here don't have docs.

http://codereview.adblockplus.org/16067002/diff/430001/block.js
File block.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/block.js#newcode32
block.js:32: ext.backgroundPage.sendMessage({
I know the coding style put this differently, but I guess we should put the
opening brace on a new line here even though it's an object literal, that's how
the code looked before.

I changed that in the coding style. The object literal exception was basically
only because putting the opening brace on a new line after = won't work.

http://codereview.adblockplus.org/16067002/diff/430001/block.js#newcode41
block.js:41: function(response) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js
File chrome/background.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:220: Tab = function(tab) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:265: return tab._id in this._map;
Wouldn't you need |this._map = this._map.bind(this);| in the constructor for
this to work? Sounded like that in your reply to one of my comments - but I'm
still wondering why this would be necessary for this to have the right value to
begin with.

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:273: TabMap.prototype["delete"] = function(tab) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:281: Window = function(win) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js
File lib/websql/io.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:34: {
Looks like indentation's off here.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:72: tx.executeSql(
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:76: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:78: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:85: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:102: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:117: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:122: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:137: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:142: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:157: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/lib/websql/io.js#newco...
lib/websql/io.js:171: {
Looks like the indentation is off here too.

http://codereview.adblockplus.org/16067002/diff/430001/metadata.safari
File metadata.safari (right):

http://codereview.adblockplus.org/16067002/diff/430001/metadata.safari#newcode32
metadata.safari:32: icons/abp-16.png                            =
icons/abp-16.png                                                          ->
contrastToAlpha
We typically don't align expressions on column.

http://codereview.adblockplus.org/16067002/diff/430001/popup.js
File popup.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/popup.js#newcode108
popup.js:108: function openOptions() {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
File safari/background.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:47: this.onCompleted      = new TabEventTarget(tab,
"navigate",       false);
We don't typically align things on column.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:135: TabMap.prototype["delete"] = function(tab) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:150: callback(this._win.tabs.map(function(tab) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:218: if (obj.constructor != RegExp)
How about && instead of nested ifs?

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:232: if (idx != -1) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:235: tab.page.dispatchMessage("proxyCallback", {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:328: try {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:341: try {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:358: try {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:408: if (this._listeners[i](message.url, message.type, tab,
0, -1) === false)
As above, I'd prefer a logical operator to nested ifs here.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:487: onCompleted:      new
TabEventTarget(safari.application, "navigate",       true),
We typically don't align things on column.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
File safari/common.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:71: return function(event) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:89: listener(event.message, sender, function(message) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:105: if (responseCallback) {
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:132: var bits, i, locale;
We typically declare variables where they are first used.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:134: var default_locale = "en_US";
camelCase please :)

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:175: // candidate, dont"t try to load it again
Typo: s/dont"t/don't/

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
File safari/content.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
safari/content.js:70: if (obj != null)
As before, why no logical operators instead of nested ifs?

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
safari/content.js:146: this.send({
Opening brace should go on its own line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/webrequest.js
File webrequest.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/webrequest.j...
webrequest.js:146: if ('chrome' in window)
We normally use ".

http://codereview.adblockplus.org/16067002/diff/5685265389584384/webrequest.j...
webrequest.js:172: var key = header.value.substr(0, index);
key and signature are already declared above. I know you just inherited the
problem, but I guess this is a good time as any to fix it.

[Sebastian Noack, 2013-11-10 11:59:55]

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js
File chrome/background.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:265: return tab._id in this._map;
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> Wouldn't you need |this._map = this._map.bind(this);| in the constructor for
> this to work? Sounded like that in your reply to one of my comments - but I'm
> still wondering why this would be necessary for this to have the right value
to
> begin with.

I don't get it. What exactly you are wondering about, whether it would be
necessary, to have what right value? All that the code in this line/function
does is checking whether the given tab, as uniquely identified by its id, has
already been added to the TabMap object. Also TabMap._map doesn't have a method
"bind". It's not a function. It's just a plain object, which is used as mapping.

[Felix Dahlke, 2013-11-10 13:00:42]

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js
File chrome/background.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:265: return tab._id in this._map;
On 2013/11/10 11:59:55, sebastian wrote:
> On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> > Wouldn't you need |this._map = this._map.bind(this);| in the constructor for
> > this to work? Sounded like that in your reply to one of my comments - but
I'm
> > still wondering why this would be necessary for this to have the right value
> to
> > begin with.
> 
> I don't get it. What exactly you are wondering about, whether it would be
> necessary, to have what right value? All that the code in this line/function
> does is checking whether the given tab, as uniquely identified by its id, has
> already been added to the TabMap object. Also TabMap._map doesn't have a
method
> "bind". It's not a function. It's just a plain object, which is used as
mapping.

Sorry, I actually meant this:

|this.had = this.has.bind(this);|

Why aren't you binding this to had, as you do in delete? Not that I think it'd
be necessary, I just don't understand why you do it for most methods that use
member variables/functions.

[Sebastian Noack, 2013-11-10 14:40:07]

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js
File chrome/background.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:265: return tab._id in this._map;
On 2013/11/10 13:00:42, Felix H. Dahlke wrote:
> Sorry, I actually meant this:
> 
> |this.had = this.has.bind(this);|
> 
> Why aren't you binding this to had, as you do in delete? Not that I think it'd
> be necessary, I just don't understand why you do it for most methods that use
> member variables/functions.

I actually only do that when it is absolutely required. And I had to bind
delete() in the constructor, in order to add and remove it to/from the
Tab.onRemoved event. If I would bind delete() inline instead, I could not remove
it again, because of fn.bind(this) != fn.bind(this).

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
File safari/background.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:408: if (this._listeners[i](message.url, message.type, tab,
0, -1) === false)
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> As above, I'd prefer a logical operator to nested ifs here.

Think of it as a more readable alternative way of AND'ing conditions, that won't
fit all on one line.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
File safari/content.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
safari/content.js:70: if (obj != null)
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> As before, why no logical operators instead of nested ifs?

Think of it as a more readable alternative way of AND'ing conditions, that won't
fit all on one line.

[Felix Dahlke, 2013-11-10 14:48:32]

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js
File chrome/background.js (right):

http://codereview.adblockplus.org/16067002/diff/430001/chrome/background.js#n...
chrome/background.js:265: return tab._id in this._map;
On 2013/11/10 14:40:08, sebastian wrote:
> On 2013/11/10 13:00:42, Felix H. Dahlke wrote:
> > Sorry, I actually meant this:
> > 
> > |this.had = this.has.bind(this);|
> > 
> > Why aren't you binding this to had, as you do in delete? Not that I think
it'd
> > be necessary, I just don't understand why you do it for most methods that
use
> > member variables/functions.
> 
> I actually only do that when it is absolutely required. And I had to bind
> delete() in the constructor, in order to add and remove it to/from the
> Tab.onRemoved event. If I would bind delete() inline instead, I could not
remove
> it again, because of fn.bind(this) != fn.bind(this).

Alright, got it :)

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
File safari/background.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:408: if (this._listeners[i](message.url, message.type, tab,
0, -1) === false)
On 2013/11/10 14:40:09, sebastian wrote:
> On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> > As above, I'd prefer a logical operator to nested ifs here.
> 
> Think of it as a more readable alternative way of AND'ing conditions, that
won't
> fit all on one line.

I think it's pretty confusing, why not just wrap the line after the logical
operator like we always do?

if ((!regex || regex.test(message)) &&
    this._listeners[i](message.url, message.type, tab, 0, -1) === false)

But since we appear to disagree here, let's hear what Wladimir says.

[Felix Dahlke, 2013-11-12 09:50:23]

Mostly done, some comments haven't been addressed.

@Wladimir, please have a look at this and give your opinion:
http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
File safari/background.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:135: TabMap.prototype["delete"] = function(tab) {
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> Opening brace should go on its own line.

This hasn't been addressed.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
File safari/common.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:71: return function(event) {
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> Opening brace should go on its own line.

This hasn't been addressed.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:89: listener(event.message, sender, function(message) {
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> Opening brace should go on its own line.

This hasn't been addressed.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/commo...
safari/common.js:105: if (responseCallback) {
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> Opening brace should go on its own line.

This hasn't been addressed.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
File safari/content.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
safari/content.js:146: this.send({
On 2013/11/10 01:07:00, Felix H. Dahlke wrote:
> Opening brace should go on its own line.

This hasn't been addressed.

http://codereview.adblockplus.org/16067002/diff/5653164804014080/safari/commo...
File safari/common.js (right):

http://codereview.adblockplus.org/16067002/diff/5653164804014080/safari/commo...
safari/common.js:175: // candidate, dont't try to load it again
Still a typo: s/dont't/don't/

[Wladimir Palant, 2013-11-12 10:37:01]

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
File safari/background.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/backg...
safari/background.js:408: if (this._listeners[i](message.url, message.type, tab,
0, -1) === false)
I agree with Felix. Please use && and insert a line break. I generally put
brackets about the if body in such cases, even for one-lines, otherwise the
indentation might be confusing.

In case you intentionally didn't indent the second if block - please don't do
that.

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
File safari/content.js (right):

http://codereview.adblockplus.org/16067002/diff/5685265389584384/safari/conte...
safari/content.js:70: if (obj != null)
As before, this is really bad style. Please don't do that.

[Wladimir Palant, 2013-11-12 15:19:52]

This is far from being a complete review. For the future: it really helps if
unrelated changes are kept out of the patchset, particularly for large reviews
like this one.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (left):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:422: */
Why remove that comment? I know that we don't use JSDoc comments consistently in
the adblockpluschrome repository but we should.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:117: return;
Won't this make the icon get stuck in its previous state that likely doesn't
apply to the current tab contents?

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:120: if ("safari" in window)
Why resort to browser sniffing? You should use require("info") here and wherever
else this approach is used, that's what we have it for.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:131: iconFilename += "-whitelisted";
The logic here is a lot more opaque than it was originally. How about you keep
the original logic for the Chrome branch:

  iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";

And the Safari branch can be simply:

  iconFilename = "icons/abp-16-whitelisted.png"

The comment here belongs into the Safari branch and should explain why we always
use the same icon there.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:136: tab.pageAction.setTitle("Adblock Plus");
Ouch, this should be localized - i18n.getMessage("name")

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:378: ext.windows.getLastFocused(function(win) {
Nit: that bracket should be on the next line.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:428: ext.windows.getLastFocused(function(win)
Why restrict it to the current browser window? We should find it in background
windows as well and bring those up - that's what the original Chrome code did.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:434: for (var i = 0; i < tabs.length; i++)
Nit: please always put brackets around multi-line blocks even if syntactically
they aren't required, that makes it much easier to read the code.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:443: win.openTab(optionsUrl, callback && function(tab) {
Nit: this bracket should be on the next line.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:501: if (!isFrameWhitelisted(sender.tab, frameId, "ELEMHIDE")) {
As with other occasions, please use && here. In general, please always indent if
block body and wrap multi-line bodies in brackets.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:502: var enabled = true;
You are redeclaring the variable here.

Side-note: fortunately, our Chrome content scripts no longer care about the
"enabled" field of the response - it is being set wrong, the ELEMHIDE check only
applies to selectors. As it is now, we can rename the request into
"get-selectors".

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:571: openOptions(function(tab) { tab.sendMessage(msg); });
Please don't compress this into one line - use the same style as the original
code.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:585: for (var i = 0; i < windows.length; i++)
Nit: Please add brackets around the multi-line block body here.

[Sebastian Noack, 2013-11-12 16:56:14]

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (left):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:422: */
On 2013/11/12 15:19:52, Wladimir Palant wrote:
> Why remove that comment? I know that we don't use JSDoc comments consistently
in
> the adblockpluschrome repository but we should.

See http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode421

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:117: return;
On 2013/11/12 15:19:52, Wladimir Palant wrote:
> Won't this make the icon get stuck in its previous state that likely doesn't
> apply to the current tab contents?

We can't control the pageAction on pages that aren't using either http or https
scheme. Chrome will hide/remove the icon automatically in that case. Otherwise
this would already be a bug in the existing code.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:120: if ("safari" in window)
On 2013/11/12 15:19:52, Wladimir Palant wrote:
> Why resort to browser sniffing? You should use require("info") here and
wherever
> else this approach is used, that's what we have it for.

In this case I agree. But when checking for the availability of window.safari in
order to call a safari.* API, I think checking for presence of the safari API is
more accurate.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:136: tab.pageAction.setTitle("Adblock Plus");
On 2013/11/12 15:19:52, Wladimir Palant wrote:
> Ouch, this should be localized - i18n.getMessage("name")

Yes, it should. However translation round already started and there is no string
"Adblock Plus" in messages.json, yet. Also note that it also wasn't translated
before.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:428: ext.windows.getLastFocused(function(win)
On 2013/11/12 15:19:52, Wladimir Palant wrote:
> Why restrict it to the current browser window? We should find it in background
> windows as well and bring those up - that's what the original Chrome code did.

You are right about that the behavior changed here. If you want I can change it
back, but I think the new behavior has a better user experience. Note that you
can't bring up background windows. You can only switch the active tab in the
window. So if the options page is already loaded in a tab in a not visible
window, and you attempt to open the options page, it seems that nothing happens.

[Wladimir Palant, 2013-11-13 07:16:27]

http://codereview.adblockplus.org/16067002/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode421
background.js:421: function openOptions(callback)
On 2013/10/25 09:55:03, sebastian wrote:
> The old docs are deprecated now (the callback is called with the tab, not its
> window object, now), and I felt that they are unneeded, since the code below
is
> pretty simple and easy to understand. Should I add docs that describe the new
> behavior of openOptions()?

I would actually prefer keeping and updating the documentation. That's a JSDoc
comment and I think that we'll eventually generate the documentation for the
code here (like we do for the core code) - it really isn't about whether the
code is understandable but rather about the API we are defining here.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:117: return;
On 2013/11/12 16:56:15, sebastian wrote:
> We can't control the pageAction on pages that aren't using either http or
https
> scheme. Chrome will hide/remove the icon automatically in that case. Otherwise
> this would already be a bug in the existing code.

This isn't really true - we can show our icon on pages that use neither http nor
https. However, it seems that Chrome will always hide our icon on a new page
unless we enable it explicitly so this approach will work.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:120: if ("safari" in window)
On 2013/11/12 16:56:15, sebastian wrote:
> In this case I agree. But when checking for the availability of window.safari
in
> order to call a safari.* API, I think checking for presence of the safari API
is
> more accurate.

I disagree. Please use require("info") for any browser-specific branching. While
we aren't expecting Opera to implement any Safari-specific features any time
soon, finding browser-specific code will be very hard if a different branching
approach is used every time.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:136: tab.pageAction.setTitle("Adblock Plus");
On 2013/11/12 16:56:15, sebastian wrote:
> Yes, it should. However translation round already started and there is no
string
> "Adblock Plus" in messages.json, yet.

There is - "name", imported from Firefox locales.

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:428: ext.windows.getLastFocused(function(win)
On 2013/11/12 16:56:15, sebastian wrote:
> You are right about that the behavior changed here. If you want I can change
it
> back, but I think the new behavior has a better user experience. Note that you
> can't bring up background windows. You can only switch the active tab in the
> window. So if the options page is already loaded in a tab in a not visible
> window, and you attempt to open the options page, it seems that nothing
happens.

No, the user experience is worse - users might unintentionally open multiple
options pages, in different browser windows. Of course the window has to be
activated when we do this - that's what Chrome is doing when opening options and
we were mimicking that functionality. In Safari you have
SafariBrowserWindow.activate() which seems to do the right thing.

[Sebastian Noack, 2013-11-13 09:33:12]

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:428: ext.windows.getLastFocused(function(win)
On 2013/11/13 07:16:27, Wladimir Palant wrote:
> No, the user experience is worse
I disagree. When you click on the options link in the bubble of a given window,
you probably want to see the options in the window, where you clicked on the
options link. You don't want it to bring the window, where you opened for
example porn sites in and put into background for a reason ;), into foreground
again.

> Of course the window has to be activated when we do this - that's
> what Chrome is doing when opening options and we were mimicking
> that functionality.
Interesting, I have experienced the opposite. When I click on "Options" in the
generic context menu of the icon, and there is already a tab in the same window
with the options in it, it will activate that tab. Otherwise it opens a new tab,
even if the options are already opened in a tab of an other window. But I
completely agree that we should mimic that behavior here.

[Felix Dahlke, 2013-11-13 12:00:16]

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:428: ext.windows.getLastFocused(function(win)
On 2013/11/13 09:33:12, sebastian wrote:
> On 2013/11/13 07:16:27, Wladimir Palant wrote:
> > No, the user experience is worse
> I disagree. When you click on the options link in the bubble of a given
window,
> you probably want to see the options in the window, where you clicked on the
> options link. You don't want it to bring the window, where you opened for
> example porn sites in and put into background for a reason ;), into foreground
> again.

I'm with Wladimir here, what the browser does is what the users will expect us
to do. Chrome opens the HTML-based settings in a new tab, Firefox and Safari
open them in a new native dialog, but that's really not a big difference from
the user perspective. The important thing is that they don't mess with the page
they are currently on.

> > Of course the window has to be activated when we do this - that's
> > what Chrome is doing when opening options and we were mimicking
> > that functionality.
> Interesting, I have experienced the opposite. When I click on "Options" in the
> generic context menu of the icon, and there is already a tab in the same
window
> with the options in it, it will activate that tab. Otherwise it opens a new
tab,
> even if the options are already opened in a tab of an other window. But I
> completely agree that we should mimic that behavior here.

Yup, that's how it works, and IMO it makes more sense.

[Sebastian Noack, 2013-11-13 12:26:34]

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
File background.js (right):

http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
background.js:428: ext.windows.getLastFocused(function(win)
On 2013/11/13 12:00:16, Felix H. Dahlke wrote:
> I'm with Wladimir here, what the browser does is what the users will expect us
to do.
What Wladimir suggested is not what the browser does.
But the new code mimics what chrome does, when it shows the options.

> Chrome opens the HTML-based settings in a new tab, Firefox and Safari open
them in a new native dialog
That's not true. We use the HTML options page for Safari, too.

> Yup, that's how it works, and IMO it makes more sense.
So you agree with me?

[Felix Dahlke, 2013-11-13 12:31:46]

On 2013/11/13 12:26:34, sebastian wrote:
> http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.js
> File background.js (right):
> 
>
http://codereview.adblockplus.org/16067002/diff/6228067651420160/background.j...
> background.js:428: ext.windows.getLastFocused(function(win)
> On 2013/11/13 12:00:16, Felix H. Dahlke wrote:
> > I'm with Wladimir here, what the browser does is what the users will expect
us
> to do.
> What Wladimir suggested is not what the browser does.
> But the new code mimics what chrome does, when it shows the options.
> 
> > Chrome opens the HTML-based settings in a new tab, Firefox and Safari open
> them in a new native dialog
> That's not true. We use the HTML options page for Safari, too.

I was talking about how the browsers do it. As a browser extension, we should
stick to how the browser is doing things unless we have a good reason not to.

> > Yup, that's how it works, and IMO it makes more sense.
> So you agree with me?

Yes, on this one: I think we should do it just like Chrome.

[Felix Dahlke, 2013-11-15 08:54:06]

LGTM from my side, let's merge this.

But let's keep this review open, Wladimir will still want to do a full review
later.