Closed
Bug 1196283
Opened 9 years ago
Closed 8 years ago
Support comments in manifest.json file
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox43 affected, firefox48 verified)
VERIFIED
FIXED
mozilla48
People
(Reporter: mossop, Assigned: kmag)
References
Details
Attachments
(1 file)
Apparently chrome supports comments in the manifest.json file, something that isn't part of the JSON spec and so our JSON parser rejects the file.
Comment 1•9 years ago
|
||
I think comment support is intentional. Chrome documentation[1] contains an example manifest with comments. [1] https://developer.chrome.com/extensions/manifest
I'm going to WONTFIX this. We're not going to be bug-for-bug compatible with Chrome.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Comment 3•9 years ago
|
||
You call it bug, I call it feature.
Assignee | ||
Comment 5•8 years ago
|
||
It looks like this is going to be standardized across browsers, so we're going to have to support it.
Assignee: nobody → kmaglione+bmo
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47511/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47511/
Attachment #8742989 -
Flags: review?(aswan)
Comment 7•8 years ago
|
||
We will need amo-validator or amo-linter support for this as well.
Comment 8•8 years ago
|
||
Comment on attachment 8742989 [details] MozReview Request: Bug 1196283: [webext] Support comments in JSON files. r?aswan https://reviewboard.mozilla.org/r/47511/#review44313 ::: toolkit/components/extensions/Extension.jsm:107 (Diff revision 1) > flushJarCache, > } = ExtensionUtils; > > const LOGGER_ID_BASE = "addons.webextension."; > > +const COMMENT_REGEXP = new RegExp(String.raw` Oh the perils of doing parsing with regular expressions. I think this technique fails on: ``` { "foo": "tricky\" // ", ... } ``` That does seem awfully obscure, and living with it sound preferrable to actually writing a parser.
Attachment #8742989 -
Flags: review?(aswan)
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/47511/#review44313 > Oh the perils of doing parsing with regular expressions. I think this technique fails on: > ``` > { > "foo": "tricky\" // ", > ... > } > ``` > That does seem awfully obscure, and living with it sound preferrable to actually writing a parser. Nope, it works as expected there. I was actually considering updating the patch so that it explicitly tested for it.
Comment 10•8 years ago
|
||
(In reply to Andy McKay [:andym] from comment #7) > We will need amo-validator or amo-linter support for this as well. This is dated, I don't know if they've fixed it but Chrome didn't let lack of support in their store stop them from shipping this originally: http://stackoverflow.com/questions/14151058/manifest-file-is-invalid-when-uploading-my-chrome-extension-to-the-chrome-web
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/47511/#review44321 ::: toolkit/components/extensions/Extension.jsm:107 (Diff revision 1) > flushJarCache, > } = ExtensionUtils; > > const LOGGER_ID_BASE = "addons.webextension."; > > +const COMMENT_REGEXP = new RegExp(String.raw` Eh, I added that line to the test and it threw a SyntaxError in JSON.parse(). And this is from node but ought to be the same here: ``` > const COMMENT_REGEXP = new RegExp(String.raw` ... ^ ... ( ..... (?: ....... [^"] | ....... " (?:[^"\\] | \\.)* " ....... )*? ..... ) ... // .* ... `.replace(/\s+/g, ""), "gm"); undefined > let s = '"foo": "tricky\" // "'; undefined > s.replace(COMMENT_REGEXP, "$1") '"foo": "tricky" ' ```
Comment 12•8 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #10) > This is dated, I don't know if they've fixed it but Chrome didn't let lack > of support in their store stop them from shipping this originally: > http://stackoverflow.com/questions/14151058/manifest-file-is-invalid-when- > uploading-my-chrome-extension-to-the-chrome-web I'm not suggesting it block, but if we don't file the relevant bugs, it will never happen :)
Assignee | ||
Comment 13•8 years ago
|
||
https://reviewboard.mozilla.org/r/47511/#review44321 > Eh, I added that line to the test and it threw a SyntaxError in JSON.parse(). And this is from node but ought to be the same here: > ``` > > const COMMENT_REGEXP = new RegExp(String.raw` > ... ^ > ... ( > ..... (?: > ....... [^"] | > ....... " (?:[^"\\] | \\.)* " > ....... )*? > ..... ) > ... // .* > ... `.replace(/\s+/g, ""), "gm"); > undefined > > let s = '"foo": "tricky\" // "'; > undefined > > s.replace(COMMENT_REGEXP, "$1") > '"foo": "tricky" ' > ``` That's because the backslash in your string literal isn't escaped. const COMMENT_REGEXP = new RegExp(String.raw` ^ ( (?: [^"] | " (?:[^"\\] | \\.)* " )*? ) // .* `.replace(/\s+/g, ""), "gm"); print((String.raw`"foo \" // bar"`).replace(COMMENT_REGEXP, "$1")); Result: "foo \" // bar"
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8742989 [details] MozReview Request: Bug 1196283: [webext] Support comments in JSON files. r?aswan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47511/diff/1-2/
Attachment #8742989 -
Flags: review?(aswan)
Comment 15•8 years ago
|
||
Comment on attachment 8742989 [details] MozReview Request: Bug 1196283: [webext] Support comments in JSON files. r?aswan https://reviewboard.mozilla.org/r/47511/#review44329 I think a test that includes an escaped quote inside a string would be a decent addition but looks good. ::: toolkit/components/extensions/Extension.jsm:115 (Diff revision 1) > + (?: > + [^"] | > + " (?:[^"\\] | \\.)* " > + )*? > + ) > + // .* The space here threw me before I got to the `replace()` below. Maybe remove it for clarity?
Attachment #8742989 -
Flags: review?(aswan) → review+
Comment 16•8 years ago
|
||
From a quick&dirty test it looks like Chrome also supports /* */ comments. Looking forward to seeing what sort of regexp Kris comes up with for those :-)
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/036c763dedca215c00853146dc050a766dfd1f04 Bug 1196283: [webext] Support comments in JSON files. r=aswan
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #16) > From a quick&dirty test it looks like Chrome also supports /* */ comments. > Looking forward to seeing what sort of regexp Kris comes up with for those > :-) I could, but the consensus so far seems to be that the webextension spec will only allow for // comments.
Comment 19•8 years ago
|
||
(In reply to Andy McKay [:andym] from comment #7) > We will need amo-validator or amo-linter support for this as well. Since writing a custom parser is not a particularly good idea I wanted to share a simple esprima hack I used here: https://github.com/abarreir/crx2ff/blob/d2b882056f902d751ad05e329efda7eddcb9d268/libs/ext-converter.js#L19-L37
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/036c763dedca
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 21•8 years ago
|
||
FYI: The linter has landed support for comments, but only line comments; block-level comments are an error that will fail validation on AMO. https://github.com/mozilla/addons-linter/pull/724
Comment 22•8 years ago
|
||
I was able to reproduce this issue using the following webextension https://addons-dev.allizom.org/en-US/firefox/addon/tab-wrangler-stage1/ on Firefox 47 (20160604131506) under Windows 10 64-bit. The webextension which contains comments in manifest.json file is successfully installed on Firefox 48 beta 3 (20160623122823) using Windows 10 64-bit and Ubuntu 16.04 32-bit.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•