Issue 2816 - Partial fix for EHH button in inspector tool, preview functionality still broken

Issue 2816 - Partial fix for EHH button in inspector tool, preview functionality still broken

[Thomas Greiner, 2015-07-28 13:52:19]

https://codereview.adblockplus.org/29322778/diff/29322779/chrome/content/comp...
File chrome/content/composer.js (right):

https://codereview.adblockplus.org/29322778/diff/29322779/chrome/content/comp...
chrome/content/composer.js:27: function findElement(node, property)
Detail: Note that if you'd make this function iterative instead of recursive you
could get rid of the unnecessary "node" parameter.

Just a personal preference, though, so feel free to keep it like that.

https://codereview.adblockplus.org/29322778/diff/29322779/chrome/content/comp...
chrome/content/composer.js:48: let prevSibling = findElement(node,
"prevSibling");
This string should say "previousSibling" from what I see.

https://codereview.adblockplus.org/29322778/diff/29322779/lib/inspectorObserv...
File lib/inspectorObserver.js (right):

https://codereview.adblockplus.org/29322778/diff/29322779/lib/inspectorObserv...
lib/inspectorObserver.js:32: let stringBundle =
Services.strings.createBundle("chrome://elemhidehelper/locale/global.properties?"
+ Math.random());
Here you're using "Services" without importing it first so why are you importing
it below?

https://codereview.adblockplus.org/29322778/diff/29322779/lib/inspectorObserv...
lib/inspectorObserver.js:56: let domain = null;
This variable should be named "host" from looking at the rest of the code.

https://codereview.adblockplus.org/29322778/diff/29322779/lib/inspectorObserv...
lib/inspectorObserver.js:67:
panelWindow.openDialog("chrome://elemhidehelper/content/composer.xul", "_blank",
Detail: Please break these lines accordingly to fit in the 80 characters limit.

[Wladimir Palant, 2015-07-29 12:04:49]

As you noticed correctly, the previous approach didn't quite work, in addition
to relying too much on unstable Inspector APIs. I reverted it and tried a
cleaner approach. I didn't find any way to leave developer tools APIs so now we
register our own actor that will produce the data for any given node.

The actor is installed via a frame script loading a JS module - process scripts
would be much less messy but they require Firefox 38 and for some reason my
process script was only being installed in the chrome process, not in the
content process.

There is also still quite a bit of code duplication - node data is being
generated in the composer as well. This will have to be changed so that the
second code path generates the data in the content process as well.

[Wladimir Palant, 2015-07-29 12:28:08]

The two code paths have been unified now and the data is always collected in the
content process. Note that the old NodeData constructor and the new getNodeData
function aren't really identical, sorry about that - in addition to returning
the data instead of storing it in properties, I've also corrected outdated
syntax and updated the code to use nextElementSibling and such.

The one outstanding issue is the preview functionality now, the composer needs
to communicate with the frame script in order to apply the stylesheet to the
correct document.

[Wladimir Palant, 2015-07-29 12:43:04]

This is polished enough now as far as I am concerned. Note that compatibility
info only had to be upped to Firefox 24 I think. However, I went with Firefox 29
to have it match the compatibility info of Adblock Plus.

[Thomas Greiner, 2015-07-29 15:02:53]

LGTM