Extension updates not being accepted by Mozilla

[adam-p]

Version 2.1.0 was submitted to Mozilla (aka addons.mozilla.org, aka AMO) May 31, 2012. It was preliminarily reviewed some time after that and became available for direct download. Because the review was only "preliminary" and not "full", the extension can't be searched for (they need a direct link) and users receive a dire warning when they try to install it.

With the following releases I tried to get a full review done. The review queue takes forever (~1 month) to get through, and if you submit a new version you get put back to the end of the queue (I found out the hard way).

I'll keep this issue updated with info about efforts to get Markdown Here fully reviewed and accepted by AMO.

Note that this issue supersedes issue #20: "Firefox extension not available anymore".

[adam-p]

Version 2.2.0 was submitted to AMO on June 7, 2012. On June 27 it was reviewed and rejected. I received this email:

---

Your add-on, Markdown Here 2.2.0, has been reviewed by an editor and did not meet the criteria for being hosted in our gallery.

Reviewer:
[Reviewer Name]

Comments:
Your version was rejected because of the following problems:

1. We can't match your included version of highlight.js to the checksum of any known version. Please include only releases of libraries obtained from official sources in their original, unmodified files. Please note that third party CDNs are not considered official sources for this purpose.

Please do not store your CSS in static JavaScript strings. It should be stored in external CSS files and, if not loaded via the href attribute of link tags (via a resource package or contentaccessible chrome package) or the stylesheet service, read in via an XHR when needed.

Please fix them and submit again. Thank you.

This version of your add-on has been disabled. You may re-request review by addressing the editor's comments and uploading a new version. To learn more about the review process, please visit https://addons.mozilla.org/developers/docs/policies/reviews#selection

If you have any questions or comments on this review, please reply to this email or join #amo-editors on irc.mozilla.org

Mozilla Add-ons
https://addons.mozilla.org

[adam-p]

The following day -- June 28 -- I replied with this email:

---

Thanks for the review. I some concerns/comments/rebuttals, though...

highlight.js modifications

1. We can't match your included version of highlight.js to the checksum of any known version. Please include only releases of libraries obtained from official sources in their original, unmodified files. Please note that third party CDNs are not considered official sources for this purpose.

I have modified the highlight.js source. All of my modifications are marked in the source code with /* adam-p: made such-and-such change */. You can see the alteration comments on lines 33, 36, and 39. You can also see the highlight.js author and I discussing the changes in the highlight.js Google Group here.

I'll detail the changes:

• 33: Added EXPORTED_SYMBOLS so that I could use it as a Firefox extension module via Components.utils.import. In our discussion, the highlight.js guy says the change "could also be supported by the build system", which would be great.
• 36: Added a fake navigator object. highlight.js does some IE checking that was breaking when run in an extension (I don't remember if the problem was under Firefox/Thunderbird or Chrome, but I had to fix it regardless). After seeing my change, the highlight.js author made changes to remove the dependency on navigator -- you can see the commit here.
• 39: (That line is just a comment for a larger change...) I concatenated the main highlight.js file and all of its language-specific .js files. Why didn't I use a minified combo package built by the highlight.js download page? Rationale:
  • I didn't want to use minified code. I want people to be able to read and review the code (!!). I also want to read the code and debug issues if they arise (and they did arise, hence the other modifications).
  • I didn't want to have 40 different import calls for all of the different language sub-files. (Plus, I'm not sure that would even work -- the sub-files depend on the main file, and some of them depend on some of the other files (the XML one). I'm not sure what the namespace looks like for files being imported.) And I'd have to modify each of those 40 files with the EXPORTED_SYMBOLS change.

I don't mind using a minified combo file if there's a checksum for users to compare against, but I don't see anything like that on the highlight.js download page. (You can build your package dynamically there, so I'm not even sure how that would work -- especially for old versions.) I guess I could tell users which package I built and have them build it, extract it, and checksum their own build. Of course, the minified code would still need to have the EXPORTED_SYMBOLS and navigator changes.

Given that I can't use any currently released highlight.js code without modification, what are my options for clearing this issue?

(I'll ask the highlight.js guy in that Google Groups thread about when he might fix/release this stuff.)

CSS-in-JS-string

Please do not store your CSS in static JavaScript strings. It should be stored in external CSS files and, if not loaded via the href attribute of link tags (via a resource package or contentaccessible chrome package) or the stylesheet service, read in via an XHR when needed.

I agree that the CSS JavaScript string is hideously ugly. (Although I'm not sure why it would be a blocking issue.) Rationale for doing it that way:

• I'm not using the CSS in a typical stylesheet manner. I create a style element only temporarily, use it to parse the CSS-JS-string, then pull out the stylesheet object, and then immediately remove the style element. And then I explicitly apply the styles to the elements in the email (this is how it has to be done for email). Again: No long-lived stylesheet object is created with the CSS.
  • Would it fix this issue if I converted the CSS into a JS object? It would suit my purposes just as well to have a selector-to-rules mapping object. I left it CSS-ish mostly so that people would be more comfortable editing their own. Also because of the next item...
• One of the next things I'm going to implement is preferences, where the user can fully edit their own styles. So, again, the CSS will likely end up being a string pulled from prefs. I'd prefer if users modify something that looks like CSS, rather than JSON.
• I know this isn't Mozilla's problem, but the extension is fo Firefox/Thunderbird and Chrome, so browser-specific APIs are something I've avoided.

Regarding your suggestions: I'll look into them. No, seriously. The CSS-in-JS-string thing is ugly as sin, so it'd be great if I can use straight CSS. Surely the browser-specific code can be limited and contained.

Next steps?

I can probably rectify the CSS-in-JS-string issue to your satisfaction, but I probably can't rectify the highlight.js issue. Or, at least, I don't see how. Can I either be excused from it -- at least until the highlight.js author incorporates my changes -- or can you suggest how I rectify it?

Thank you.

[adam-p]

Because one of the AMO reviewer's complaints was that I was using a modified version of Highlight.js, on June 28th I contacted the author of that project to ask him if he would be making the changes I needed so that I could use his code without modification. He indicated that he wouldn't be making the changes any time soon, but that the problem didn't make sense to him. He also suggested that I fork Highlight.js so that my version becomes canonical (for me).

The exchange can be seen here:
https://groups.google.com/forum/?fromgroups#!topic/highlightjs/IK-uPMIK8DQ

[adam-p]

On June 26th or 27th the Firefox/Thunderbird extension became completely unavailable in addons.mozilla.org. Approximately 1 week later (but certainly by July 10) the extension reappeared on AMO -- but still version 2.1.0, not 2.2.0.

(Issue #20 resulted from this removal.)

[adam-p]

After getting no response to my previous appeal/email, I sent another to AMO on July 13, 2012.

---

It has been two weeks and I've had no response to my appeal.

I have contacted the Highlight.js author and he seems to think that I shouldn't have to match a checksum of his code and/or that there isn't one to match. He also suggests that I fork his code so that I can claim it as canonical (for Markdown Here). See the bottom of this thread: https://groups.google.com/forum/?fromgroups#!topic/highlightjs/IK-uPMIK8DQ

(And surely having CSS in a string won't block an approval.)

Can I get references to the rules that I'm violating? Or something?

I would like this review to proceed so that I can fully support Firefox and Thunderbird.

Thank you.

Adam Pritchard

[adam-p]

I received a reply from the Mozilla reviewer on July 17, 2012.

The reviewer's mention of "subscript loader" led me (not coincidentally) mozIJSSubScriptLoader, which is an alternative to Components.utils.import. After reading about it (and a bit of testing) I realized that I will be able to use Highlight.js unmodified. Phew!

---

I'm sorry. I distinctly recall replying to this over a week ago.
I'm not sure what happened.

On Thu, Jun 28, 2012 at 01:21:46PM -0400, Adam Pritchard wrote:

An addendum to my original message:

Regarding the modifications to Highlight.js: If I fork the Highlight.js
repo, does it then become my project and I can modify it as I see fit?

This isn't a matter of semantic games. We disallow trivial
modifications to libraries because the burden of reviewing those
changes is more than we can reasonably handle. Your changes to
highlight.js can by and large be avoided by a) not amalgamating
the source libraries, b) using the subscript loader rather than
altering the source to load id as a JavaScript module, and c)
monkey patching as necessary. Where the above are not reasonably
achievable, we can allow you to make necessary source
modifications, but points a and b still apply.

33: Added EXPORTED_SYMBOLS so that I could use it as a Firefox
extension module via Components.utils.import. In our discussion, the
highlight.js guy says the change "could also be supported by the build
system", which would be great.

Using the subscript loader obviates the need for this change.

36: Added a fake navigator object. highlight.js does some IE
checking that was breaking when run in an extension (I don't remember if
the problem was under Firefox/Thunderbird or Chrome, but I had to fix it
regardless). After seeing my change, the highlight.js author made changes
to remove the dependency on navigator -- you can see the commit herehttps://github.com/isagalaev/highlight.js/commit/9c492dfc43f67dd4aeb92a1cf4041b1e378c5587

You can do this without any source modifications. Simply create
a navigator object in the scope into which you load the library.

39: (That line is just a comment for a larger change...) I
concatenated the main highlight.js file and all of its language-specific
.js files. Why didn't I use a minified combo package built by the highlight.js
download page http://softwaremaniacs.org/soft/highlight/en/download/?
Rationale:
- I didn't want to use minified code. I want people to be able to
read and review the code (!!). I also want to read the code and debug
issues if they arise (and they did arise, hence the other modifications).
- I didn't want to have 40 different import calls for all of the
different language sub-files. (Plus, I'm not sure that would even work --
the sub-files depend on the main file, and some of them depend on some of
the other files (the XML one). I'm not sure what the namespace looks like
for files being imported.) And I'd have to modify each of those 40 files
with the EXPORTED_SYMBOLS change.

You don't. Just use the subscript loader to load them all into
the same scope. You can either list all of the necessary files
in an array and iterate over them or use the nsIZipReader to
iterate over the contents of a folder in your XPI.

Please do not store your CSS in static JavaScript strings. It should be
stored in external CSS files and, if not loaded via the href attribute of
link tags (via a resource package or contentaccessible chrome package) or
the stylesheet service, read in via an XHR when needed.

I agree that the CSS JavaScript string is hideously ugly. (Although I'm
not sure why it would be a blocking issue.)

It wouldn't be on its own, but they are incredibly difficult to
review in their current form, and since you need to provide an
update anyway, this change should be included. In any case, this
is not a requirement.

I'm not using the CSS in a typical stylesheet manner. I create a styleelementhttps://github.com/adam-p/markdown-here/blob/master/src/common/markdown-here.js#L146only temporarily, use it to parse
the CSS-JS-stringhttps://github.com/adam-p/markdown-here/blob/master/src/common/markdown-here.js#L150,
then pull out the stylesheet objecthttps://github.com/adam-p/markdown-here/blob/master/src/common/markdown-here.js#L158,
and then immediately remove the style elementhttps://github.com/adam-p/markdown-here/blob/master/src/common/markdown-here.js#L168.
And then I explicitly apply the styles to the elements in the email (this
is how it has to be done for email). Again: No long-lived stylesheet object
is created with the CSS.
- Would it fix this issue if I converted the CSS into a JS object? It
would suit my purposes just as well to have a selector-to-rules mapping
object. I left it CSS-ish mostly so that people would be more comfortable
editing their own. Also because of the next item...

I'm not particularly concerned how you do it, so long as I can
make sense of it.

[adam-p]

I replied to the Mozilla reviewer on July 22, 2012.

---

Thanks for the reply.

I'm pretty sure now that, thanks to your suggestion, I can use mozIJSSubScriptLoader to load Highlight.js without modifying it. So that's great.

But another question: What versions of Highlight.js are okay (and from where)? Can I pull a copy from the Highight.js download page? That seems like the best place, but it dynamically creates the package depending on what language checkboxes are selected (I want all of them), and in your original review you mentioned "checksum of any known version", so... it's hard to imagine you have checksums of all combinations. Plus the new version of Highlight.js is quite recent.

So where should I pull Highlight.js code from that works best for you? A particular source tag (and then use then unpacked)? Download page and tell you what checkboxes I picked so that you can get the same package and verify? The CDN version mentioned on the download page (which has core languages, and then each extra language is separate)? Something else?

Please let me know, so I can make sure the next release is satisfactory.

Thank you.

Adam Pritchard

[adam-p]

The Mozilla reviewer replied to me on July 23, 2012.

---

Thanks for the reply.

I'm pretty sure now that, thanks to your suggestion, I can use
mozIJSSubScriptLoader to load Highlight.js without modifying it. So that's
great.

But another question: What versions of Highlight.js are okay (and from
where)? Can I pull a copy from the Highight.js download
pagehttp://softwaremaniacs.org/soft/highlight/en/download/?
That seems like the best place, but it dynamically creates the package
depending on what language checkboxes are selected (I want all of them),
and in your original review you mentioned "checksum of any known version",
so... it's hard to imagine you have checksums of all combinations. Plus the
new version of Highlight.js is quite recent.

Any release version should be fine, but you should let me know
which one it is. Development versions are OK if you have a
particular reason that you can't use a release, or the project
doesn't offer releases.

So where should I pull Highlight.js code from that works best for you? A
particular source tag (and then use then unpacked)? Download page and tell
you what checkboxes I picked so that you can get the same package and
verify? The CDN version mentioned on the download page (which has core
languages, and then each extra language is separate)? Something else?

It would be best to use the source from a release tag in the git
repo. Auto-amalgamated libraries are difficult to verify, and I
only have special code to handle inordinately popular libraries
that are almost always amalgamated, like jQuery UI.

[adam-p]

I replied to the Mozilla reviewer on July 23, 2012.

I also committed changes that I hope will allow Mozilla to approve Markdown Here. See: d0edc3f. Note that I used the build step mentioned in the below email.

---

I don't think that it will be possible to just pull a tag and use the raw
code without building. If you look at the main source file --
https://github.com/isagalaev/highlight.js/blob/master/src/highlight.js --
you can see that it's just a nameless function. If I try to load it as a
script, I (unsurprisingly) get an error: "function statement requires a
name".

(I suppose I could XHR the file into a string, prepend "var hljs = new ",
and then eval() it, but... that seems super dirty.)

Highlight.js includes a Python build script that creates files suitable for
use in the browser or Node.js. It also has the option to not minify.
Would that be acceptable for me to use? You could reproduce my build (from
the latest tag) with python tools/build.py -n.

Thanks.

Adam Pritchard

[adam-p]

The Mozilla reviewer replied on July 24, 2012.

His response means that the currently committed code should be okay. It was built from the Marked.js 7.0.1 tag using Marked's build system (python tools/build.py -n). I'll be submitting a new release in the near future.

---

Ok. Just let me know what version you build from and what build flags you use.

[adam-p]

I submitted a new version -- 2.5.0 -- on July 27, 2012. I also email the below to the Mozilla reviewer.

Fingers crossed! (But don't hold your breath -- it's at the back of the queue. It might take another month.)

---

I submitted a new version a few minutes ago. I also noted this in the reviewer notes field, but it uses an unmodified Highlight.js built from tag 7.0.1 by running python tools/build.py -n.

I also moved the CSS that was hacked into JS strings into separate CSS files that are loaded via XHR.

So, hopefully I've addressed the issues and haven't introduced any new ones.

I look forward to your review, and thanks for all your help.

[adam-p]

Mozilla sent me the results of the review of version 2.5.0 on August 7, 2012. And... failed again.

The first problem (the "loose" variable) is quite simple and I've committed a fix for it (c6eafd7).

I'm very concerned about the second problem -- that the tester couldn't make the extension work anywhere. That suggests that either the extension is horribly broken, or it's not clear that how/where the extension should be used.

I don't think it's horribly broken -- I've tested it and use it in Firefox. So when I submit a new release tomorrow I'll make sure I explain what it does and where.

---

Your add-on, Markdown Here 2.5.0, has been reviewed by an editor and did not meet the criteria for full review. However, your add-on has been granted preliminary review and is now available for download in our gallery at https://addons.mozilla.org/addon/markdown-here/

Reviewer:
[Reviewer Name]

Comments:
This version didn't pass full review because of the following issues:

1. In order to prevent conflicts with other add-ons that may be installed by users, you need to wrap your "loose" variables and functions within a JavaScript object. You can see examples on how to do this at https://developer.mozilla.org/en/XUL_School/JavaScript_Object_Management

   Problem globals include:

    scriptLoader

2. Please provide us with detailed information on how to test your add-on. I can't find any site where this seems to work. This information can be added to the Approval Notes of the version you are submitting (and future versions preferably), and in a reply to this review email.

You need to correct them to get full approval. Thanks.

Your add-on will now appear in search results and categories with some limitations. You may re-request full review by addressing the editor's comments and uploading a new version. To learn more about the review process, please visit https://addons.mozilla.org/developers/docs/policies/reviews#selection

If you have any questions or comments on this review, please reply to this email or join #amo-editors on irc.mozilla.org

[adam-p]

I submitted v2.5.2 on August 8, 2012. The loose variable is fixed, and I included a fairly detailed description of how to test. That description is included below.

I also noticed that v2.5.0 has been marked as "preliminarily reviewed", so that's good. That will be the version that's available on AMO now.

---

    This extension is used to manipulate the content of email before sending. It can be used in Gmail, Hotmail, or Yahoo webmail interfaces, or in Thunderbird. Make sure you use the rich-edit interface for the email client. 

    You write email in Markdown (if you're not familiar with Markdown, I made a little reference -- https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet), then right-click, click the "Markdown Toggle" menu item, and the content of the email will be replaced with a rendered-HTML version of the Markdown you had written.

    If you select a subset of your email and then "Markdown Toggle", only that selection will be rendered/converted.

    The Markdown is "Github-flavored". Syntax highlighting is supported. So, for example, you can use fenced code blocks with the language name specified, like so:

    ```javascript
    var i = 0;
    ```

    For complete info, see the project page: https://github.com/adam-p/markdown-here

    Please don't hesitate to contact me with questions.

    ---

    Edited to add:
    Maybe I should include a somewhat-more-formal test methodology.

    1. Go into your webmail of choice, or Thunderbird. Make sure rich editing is enabled. 

    2. Write a bit of Markdown. (I'll include some at the bottom of this that you can paste, if you want. Make sure to paste as plaintext.)

    3. Right-click and choose "Markdown Toggle". 

    Test #1: The entire email should be converted to HTML, in accordance with Markdown syntax. The sample MD should have H1, H3, UL, and PRE/CODE elements, with some bold and code inline.

    4. Right-click again and choose "Markdown Toggle".

    Test #2: The email email should be reverted back the original raw Markdown.

    5. Select a few lines (but don't split the code block). Then right-click and choose "Markdown Toggle".

    Test #3: Only the selected lines should be rendered to HTML.

    6. Right-click again and choose "Markdown Toggle".

    Test #4: The email should be reverted back to original Markdown.

    7. Render the email again (either entirely or partially) and send it (to yourself or someone else).

    Test #5: The recipient of the email should see it pretty much as it was before you sent it. 

    Test done.

    Caveat: Hotmail is a little flaky. Gmail and Thunderbird are the only first-class clients. Details: https://github.com/adam-p/markdown-here#compatibility 

    A few secondary features that can be tested if desired:

    - Signatures should be excluded from the conversion. Sigs must be preceded by the standard-ish '-- ' (note the trailing space).
    - Images embedded in the email before conversion (Gmail and Thunderbird) should be retained by the conversion.
    - Quoted reply text should be excluded by the conversion.

    ---

    # Markdown Sample (H1)

    ### Smaller header (H3)

    * Bullet **with bold**
    * Bullet with `inline code`

    ```javascript
    var s = 'code block';
    console.log(s);
    ```

[adam-p]

Version 2.5.2 passed review on August 27, 2012. PASSED!.

There's a caveat, though: The reviewer points out some some variables leaking into the global scope, "which should be addressed before your next update". These variables belong to Highlight.js (integer_re, float_re). Except... I'm not allowed to modify Highlight.js (see above). I think I see what the problem is, so I'll submit a pull request to @isagalaev and hope for the best.

I'm closing this issue, since it's resolved. But I'll keep it updated if I encounter resistance in the future.

---

Your add-on, Markdown Here 2.5.2, has been fully reviewed by an editor and is now available for download in our gallery at https://addons.mozilla.org/addon/markdown-here/

Reviewer:
[Reviewer Name]

Comments:
This version has been approved for the public. Due to caching and mirroring on our site, it can take a few hours before this change is visible, so please be patient.

However, I have the following issues which should be addressed before your next update:

1. You assign to the undeclared variables float_re, integer_re, and probably others, thus leaking them to the global scope which could cause, among other issues, compatibility problems with other add-ons. You can help prevent these types of problems in the future by adding "use strict"; (with quotes) to the top of your JavaScript files.

Keep up the good work!

Your add-on will no longer display warnings and can make full use of features such as contributions and beta channels. To learn more about the review process, please visit https://addons.mozilla.org/developers/docs/policies/reviews#selection

If you have any questions or comments on this review, please reply to this email or join #amo-editors on irc.mozilla.org

[isagalaev]

Hey! If you do send a pull request, please be so kind as to write a summary what is it all about. I don't think I'll have courage to read this whole thread :-)

[adam-p]

@isagalaev: No need. It looks like the problem was fixed 4 days ago in this rev. Any idea when you're going to put out a release with this change?

[isagalaev]

I believe I can do a point release even today or in a few days at most.

[adam-p]

Awesome. I'll keep an eye on your tags. I have a release ready to go.

[isagalaev]

Actually, I went ahead and just made the 7.2 release. There was nothing holding it anyway. Enjoy!

It won't be available on the CDN for some time though but I guess you don't care about it.