Implemented ext.contextMenus for Safari

Implemented ext.contextMenus for Safari and changed current implementation in Chrome to make it compatible with Safari.

This review also includes a change that made "shouldShowBlockElementMenu" and "hidePlaceholders" use Prefs to allow listening to changes made to it.

[Thomas Greiner, 2014-01-07 12:13:11]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
This was necessary because Safari is otherwise showing both scrollbars.

[Sebastian Noack, 2014-01-07 14:13:13]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
On 2014/01/07 12:13:11, Thomas Greiner wrote:
> This was necessary because Safari is otherwise showing both scrollbars.

This is a hack. The problem here is that the code sending "clickhide-init", is
executed on DOMContentLoaded, and therefore before the resources are loaded. And
when the resources (like the ABP logo) are loaded the height will change. So the
proper solution would be to use the "load" event instead.

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:597: document.addEventListener("contextmenu",
function(event)
This should go in safari/ext/content.js

[Thomas Greiner, 2014-01-07 15:01:39]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
On 2014/01/07 14:13:14, Sebastian Noack wrote:
> On 2014/01/07 12:13:11, Thomas Greiner wrote:
> > This was necessary because Safari is otherwise showing both scrollbars.
> 
> This is a hack. The problem here is that the code sending "clickhide-init", is
> executed on DOMContentLoaded, and therefore before the resources are loaded.
And
> when the resources (like the ABP logo) are loaded the height will change. So
the
> proper solution would be to use the "load" event instead.

Sounds like you addressed that already?! In that case I could just remove this
change from this review.

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:597: document.addEventListener("contextmenu",
function(event)
On 2014/01/07 14:13:14, Sebastian Noack wrote:
> This should go in safari/ext/content.js

Done.

[Sebastian Noack, 2014-01-07 15:42:27]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
On 2014/01/07 15:01:39, Thomas Greiner wrote:
> On 2014/01/07 14:13:14, Sebastian Noack wrote:
> > On 2014/01/07 12:13:11, Thomas Greiner wrote:
> > > This was necessary because Safari is otherwise showing both scrollbars.
> > 
> > This is a hack. The problem here is that the code sending "clickhide-init",
is
> > executed on DOMContentLoaded, and therefore before the resources are loaded.
> And
> > when the resources (like the ABP logo) are loaded the height will change. So
> the
> > proper solution would be to use the "load" event instead.
> 
> Sounds like you addressed that already?! In that case I could just remove this
> change from this review.

No I didn't. I just figured out what the problem is while reviewing your
changes. You have to replace |$(init);| with |window.addEventListener("load",
init);| in popup.js. Also the +5 hack above that already has been there before,
can be removed, then.

[Thomas Greiner, 2014-01-07 16:15:51]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
On 2014/01/07 15:42:28, Sebastian Noack wrote:
> On 2014/01/07 15:01:39, Thomas Greiner wrote:
> > On 2014/01/07 14:13:14, Sebastian Noack wrote:
> > > On 2014/01/07 12:13:11, Thomas Greiner wrote:
> > > > This was necessary because Safari is otherwise showing both scrollbars.
> > > 
> > > This is a hack. The problem here is that the code sending
"clickhide-init",
> is
> > > executed on DOMContentLoaded, and therefore before the resources are
loaded.
> > And
> > > when the resources (like the ABP logo) are loaded the height will change.
So
> > the
> > > proper solution would be to use the "load" event instead.
> > 
> > Sounds like you addressed that already?! In that case I could just remove
this
> > change from this review.
> 
> No I didn't. I just figured out what the problem is while reviewing your
> changes. You have to replace |$(init);| with |window.addEventListener("load",
> init);| in popup.js. Also the +5 hack above that already has been there
before,
> can be removed, then.

I see your point but block.html is included as an iFrame and iFrames don't adapt
their size dynamically. We need to explicitly set its width and height.

[Sebastian Noack, 2014-01-07 16:28:34]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
On 2014/01/07 16:15:51, Thomas Greiner wrote:
> On 2014/01/07 15:42:28, Sebastian Noack wrote:
> > On 2014/01/07 15:01:39, Thomas Greiner wrote:
> > > On 2014/01/07 14:13:14, Sebastian Noack wrote:
> > > > On 2014/01/07 12:13:11, Thomas Greiner wrote:
> > > > > This was necessary because Safari is otherwise showing both
scrollbars.
> > > > 
> > > > This is a hack. The problem here is that the code sending
> "clickhide-init",
> > is
> > > > executed on DOMContentLoaded, and therefore before the resources are
> loaded.
> > > And
> > > > when the resources (like the ABP logo) are loaded the height will
change.
> So
> > > the
> > > > proper solution would be to use the "load" event instead.
> > > 
> > > Sounds like you addressed that already?! In that case I could just remove
> this
> > > change from this review.
> > 
> > No I didn't. I just figured out what the problem is while reviewing your
> > changes. You have to replace |$(init);| with
|window.addEventListener("load",
> > init);| in popup.js. Also the +5 hack above that already has been there
> before,
> > can be removed, then.
> 
> I see your point but block.html is included as an iFrame and iFrames don't
adapt
> their size dynamically. We need to explicitly set its width and height.

Yes, that is true. And that is why we send a message with the content dimensions
from block.js to include.postload.js where it adjusts the size of the frame.

I just say that you have to get the dimensions of the frame's content during the
"load" event instead of the  DOMContentLoaded event. And that then there is no
need to add any offset when setting the dimensions of the frame.

[Thomas Greiner, 2014-01-07 17:39:42]

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
Sorry, I got a bit confused when you were talking about popup.js there.

Done.

[Sebastian Noack, 2014-01-07 17:42:20]

LGTM

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5629499534213120/incl...
include.postload.js:566: clickHideFiltersDialog.style.height = (msg.height + 15)
+ "px";
On 2014/01/07 17:39:42, Thomas Greiner wrote:
> Sorry, I got a bit confused when you were talking about popup.js there.
> 
> Done.

Sorry, I meant block.js.

[Wladimir Palant, 2014-01-17 15:36:55]

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
File chrome/background.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
chrome/background.js:373: var contextMenu = [];
Nit: Would contextMenuItems be a better name?

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
chrome/background.js:405: hideMenu: function()
Nit: These methods should be called showMenuItems/hideMenuItems to indicate that
they affect all menu items.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
chrome/background.js:411: })();
The API here is somewhat unexpected - addMenuItem and removeMenuItems have no
immediate effect. I would expect an internal flag variable isContextMenuHidden -
false by default, set in showMenu and hideMenu, same as for Safari. If
isContextMenuHidden is true then addMenuItem calls showMenu and removeMenuItems
calls hideMenu automatically. Also, showMenu can check that flag and return
early if shown is already true - no point wasting effort here. Note that this is
already how it works in Safari.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
File safari/background.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:532: id: "block-element",
So all menu items have the same ID?

How about using "context-menu-item-" + contextMenu.length as identifier? I'm
assuming here that menu item IDs have to be unique across all extensions,
otherwise String(contextMenu.length) will be sufficient - and you will be able
to look up the right menu item in the array without looping.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:558: context = "image";
Seeing that srcUrl can be null, it probably makes sense to add one more check
here:

  if (!event.userInfo.srcUrl)
    context = null;

This will make sure that the context menu will only be shows if context="all" is
requested.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:563: return;
Why is this check inside the loop? This belongs to the beginning of the
function.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:568: return;
This should be continue, not return - there can be multiple menu items.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:570: menuItem.item =
event.contextMenu.appendContextMenuItem(menuItem.id, menuItem.title);
I don't think that menuItem.item is being used, no point setting it.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/content.js:316: tagName: element.tagName
Please use localName here.

[Thomas Greiner, 2014-01-18 10:32:05]

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
File chrome/background.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
chrome/background.js:373: var contextMenu = [];
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> Nit: Would contextMenuItems be a better name?

Done.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
chrome/background.js:405: hideMenu: function()
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> Nit: These methods should be called showMenuItems/hideMenuItems to indicate
that
> they affect all menu items.

Done.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/chro...
chrome/background.js:411: })();
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> The API here is somewhat unexpected - addMenuItem and removeMenuItems have no
> immediate effect. I would expect an internal flag variable isContextMenuHidden
-
> false by default, set in showMenu and hideMenu, same as for Safari. If
> isContextMenuHidden is true then addMenuItem calls showMenu and
removeMenuItems
> calls hideMenu automatically. Also, showMenu can check that flag and return
> early if shown is already true - no point wasting effort here. Note that this
is
> already how it works in Safari.

Done. However, I decided to go with true as the default value for
isContextMenuHidden because it makes more sense to have it hidden by default and
only show it when something has been added to it.
I also changed this in the Safari implementation to make it consistent.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
File safari/background.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:532: id: "block-element",
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> So all menu items have the same ID?
> 
> How about using "context-menu-item-" + contextMenu.length as identifier? I'm
> assuming here that menu item IDs have to be unique across all extensions,
> otherwise String(contextMenu.length) will be sufficient - and you will be able
> to look up the right menu item in the array without looping.

Done. According to Apple "The identifier must be unique within your extension."
https://developer.apple.com/library/safari/documentation/Tools/Conceptual/Saf...

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:558: context = "image";
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> Seeing that srcUrl can be null, it probably makes sense to add one more check
> here:
> 
>   if (!event.userInfo.srcUrl)
>     context = null;
> 
> This will make sure that the context menu will only be shows if context="all"
is
> requested.

Done.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:563: return;
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> Why is this check inside the loop? This belongs to the beginning of the
> function.

Done.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:568: return;
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> This should be continue, not return - there can be multiple menu items.

Done.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/background.js:570: menuItem.item =
event.contextMenu.appendContextMenuItem(menuItem.id, menuItem.title);
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> I don't think that menuItem.item is being used, no point setting it.

Done.

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/5676830073815040/safa...
safari/content.js:316: tagName: element.tagName
On 2014/01/17 15:36:55, Wladimir Palant wrote:
> Please use localName here.

Done.

[Wladimir Palant, 2014-01-18 11:13:38]

LGTM (one nit remaining)

http://codereview.adblockplus.org/5589897452716032/diff/6252742238535680/safa...
File safari/background.js (right):

http://codereview.adblockplus.org/5589897452716032/diff/6252742238535680/safa...
safari/background.js:560: var context = event.userInfo.tagName.toLowerCase();
Nit: toLowerCase() is no longer necessary now that you are using localName.