EEH inspector integration fix

This issue is related to current ticket:
https://issues.adblockplus.org/ticket/317

[saroyanm, 2014-04-14 15:38:01]

Wladimir please have a look when you will have time.
Thanks in advance.

http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/...
File lib/inspectorObserver.js (right):

http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/...
lib/inspectorObserver.js:53: button.setAttribute("class",
"devtools-toolbarbutton");
Wladimir I have doubt regarding the ABP button look with this class in FF
nightly inpector, 
but seams like it's a style that inspector uses for buttons. Not sure if we are
okey with it.

[Wladimir Palant, 2014-04-14 21:40:39]

http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/...
File lib/inspectorObserver.js (right):

http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/...
lib/inspectorObserver.js:53: button.setAttribute("class",
"devtools-toolbarbutton");
On 2014/04/14 15:38:01, saroyanm wrote:
> Not sure if we are okey with it.

No, we are not.

This is only an issue with the light DevTools theme, it inverts the icons (see
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/...).
Messing up extension icons isn't the intention here, we need to overwrite the
filter.

You should move the styles for this icon into a new
chrome/skin/devToolsOverlay.css file. There you can also add a filter:none as a
style for #inspector-abp-elemhide-toolbarbutton>image element. You can then
inject that style file here similarly to how main.js does it with overlay.css.

[saroyanm, 2014-04-15 10:21:49]

Updated,
hope this will work.

http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/...
File lib/inspectorObserver.js (right):

http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/...
lib/inspectorObserver.js:53: button.setAttribute("class",
"devtools-toolbarbutton");
On 2014/04/14 21:40:40, Wladimir Palant wrote:
> On 2014/04/14 15:38:01, saroyanm wrote:
> > Not sure if we are okey with it.
> 
> No, we are not.
> 
> This is only an issue with the light DevTools theme, it inverts the icons (see
>
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/...).
> Messing up extension icons isn't the intention here, we need to overwrite the
> filter.
> 
> You should move the styles for this icon into a new
> chrome/skin/devToolsOverlay.css file. There you can also add a filter:none as
a
> style for #inspector-abp-elemhide-toolbarbutton>image element. You can then
> inject that style file here similarly to how main.js does it with overlay.css.

Thanks for notes,
Done.

[Wladimir Palant, 2014-04-15 13:38:19]

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chro...
File chrome/skin/devToolsOverlay.css (right):

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chro...
chrome/skin/devToolsOverlay.css:9: #inspector-abp-elemhide-toolbutton>image
Nit: please add a space before and after > here (I wrote it down without spaces
in the review comment for readability, not because that's our usual style).

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chro...
chrome/skin/devToolsOverlay.css:12: }
We should also define list-style-image and other styles for
#inspector-abp-elemhide-toolbarbutton here - if we have a proper stylesheet we
shouldn't use inline styles.

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/lib/...
File lib/inspectorObserver.js (right):

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/lib/...
lib/inspectorObserver.js:49: button.setAttribute("id",
"inspector-abp-elemhide-toolbutton");
Nit: I'm not sure why we use that weird id here, I think it was originally
generated by the DevTools API. Maybe change that into
ehh-inspector-toolbarbutton while you are at it?

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/lib/...
lib/inspectorObserver.js:63: let style =
panelWindow.document.createProcessingInstruction("xml-stylesheet",
'class="elemhidehelper-node"
href="chrome://elemhidehelper/skin/devToolsOverlay.css" type="text/css"');
Nit: class attribute is unnecessary if we aren't going to remove that
stylesheet.

[saroyanm, 2014-04-15 13:54:42]

Thanks for notes Wladimi, 
Please look at updated patch.

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chro...
File chrome/skin/devToolsOverlay.css (right):

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chro...
chrome/skin/devToolsOverlay.css:9: #inspector-abp-elemhide-toolbutton>image
On 2014/04/15 13:38:19, Wladimir Palant wrote:
> Nit: please add a space before and after > here (I wrote it down without
spaces
> in the review comment for readability, not because that's our usual style).

Done.

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chro...
chrome/skin/devToolsOverlay.css:12: }
On 2014/04/15 13:38:19, Wladimir Palant wrote:
> We should also define list-style-image and other styles for
> #inspector-abp-elemhide-toolbarbutton here - if we have a proper stylesheet we
> shouldn't use inline styles.

Good point!

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/lib/...
File lib/inspectorObserver.js (right):

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/lib/...
lib/inspectorObserver.js:49: button.setAttribute("id",
"inspector-abp-elemhide-toolbutton");
On 2014/04/15 13:38:19, Wladimir Palant wrote:
> Nit: I'm not sure why we use that weird id here, I think it was originally
> generated by the DevTools API. Maybe change that into
> ehh-inspector-toolbarbutton while you are at it?

Done.

http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/lib/...
lib/inspectorObserver.js:63: let style =
panelWindow.document.createProcessingInstruction("xml-stylesheet",
'class="elemhidehelper-node"
href="chrome://elemhidehelper/skin/devToolsOverlay.css" type="text/css"');
On 2014/04/15 13:38:19, Wladimir Palant wrote:
> Nit: class attribute is unnecessary if we aren't going to remove that
> stylesheet.

Done.

[Wladimir Palant, 2014-04-15 14:03:35]

LGTM with the remaining nit fixed.

http://codereview.adblockplus.org/5765003219042304/diff/5194384987389952/chro...
File chrome/skin/devToolsOverlay.css (right):

http://codereview.adblockplus.org/5765003219042304/diff/5194384987389952/chro...
chrome/skin/devToolsOverlay.css:13: -moz-image-region: rect(0px, 16px, 16px,
0px);
We should not go down into the anonymous elements unless necessary -
list-style-image and -moz-image-region should still be applied to
#ehh-inspector-toolbarbutton itself. filter property is the only exception,
there we cannot do it better.