asciidoctor / asciidoctor-firefox-addon

:wolf: An add-on for Mozilla Firefox that converts AsciiDoc files to HTML directly in the browser using Asciidoctor.js.
https://addons.mozilla.org/en-US/firefox/addon/asciidoctorjs-live-preview/
MIT License
32 stars 9 forks source link

Code review #19

Closed mojavelinux closed 10 years ago

mojavelinux commented 10 years ago
mojavelinux commented 10 years ago

I absolutely could not figure out how to use nsParserUtils from the content script. I guess this just isn't possible. The benefit would be that we could get a document fragment back when sanitizing instead of a string. I suppose it's plenty fast to do it the way we are doing, and technically more correct.

I noticed, however, that the Markdown Viewer is able to use the nsParserUtils because they don't use contentScript. (See https://github.com/Thiht/markdown-viewer/blob/master/chrome/content/markdown-viewer.js) I'm not sure if that's better. Is it more correct to use contentScript for an extension?

ggrossetie commented 10 years ago

Thanks that's way better now!

I absolutely could not figure out how to use nsParserUtils from the content script. I guess this just isn't possible.

Same here.

The benefit would be that we could get a document fragment back when sanitizing instead of a string.

Yes but I asked Kris Maglione (the reviewer) and he's OK with the sanitize method.

I noticed, however, that the Markdown Viewer is able to use the nsParserUtils because they don't use contentScript. (See https://github.com/Thiht/markdown-viewer/blob/master/chrome/content/markdown-viewer.js) I'm not sure if that's better. Is it more correct to use contentScript for an extension?

I don't know, I used the same "architecture" as the Chrome extension with a "background" script and a "content script". I didn't know that we can retrieve the document content from the main script (i.e. have access to the DOM of the page).