59 bytes, text/x-review-board-request
Based on a bunch of testing I just did to help a colleague, we have come to the conclusion that the name one uses when creating an experiment (i.e., the prefix of the id in the <em:id> entry in install.rdf) cannot be the same as the namespace as defined in the schema.json and api.js files. When they are both the same, and one loads the experiment add-on, and then subsequently an extension that requests permission to use the experiment, the following error appears in the browser console: Error opening input stream (invalid filename?): resource://extension-newtabcontent-api/api.js Extension.jsm:1178 If one changes either the name or the namespace then the problem goes away. This is contrary to the documentation both at  and , the latter of which specifically states: "The first part of the id (before the @) must match the namespace used in api.js and schema.json." So it seems like this is likely a regression that was introduced at some point. A further note is that the boilerplate example include the permission string in the schema.json file, but Kris says that is unnecessary, and if that is the case it should be removed from the boilerplate example.  https://webextensions-experiments.readthedocs.io/en/latest/how.html  https://github.com/web-ext-experiments/boilerplate-experiment/blob/master/install.rdf
On further testing I don't think that the issue is that the experiment name cannot be the same as the namespace. When the namespace is lowercase only (e.g. newtabcontent) and the name is the same then everything works correctly. It's only if the name and namespace have capital letters (e.g. newTabContent) that the error occurs. I've noticed that if the name of the experiment and the namespace are different but the name is not wholly lowercase then the same issue occurs (the namespace can be capitalised regardless). Hence my guess would be that input stream error arises when the experiment name is not wholly lowercase, and the cause is that the path that is being opened is transformed to lowercase (or some similar error with path capitalisation).
I think the problem is that when we register resources for an experiment here: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/mozapps/extensions/internal/APIExtensionBootstrap.js#26 we use the experiment name as provided, but it should be normalized to lower case first: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/netwerk/protocol/res/nsISubstitutingProtocolHandler.idl#23-24 A fix should be as simple as adding `.toLowerCase()` to the definition of `resource`. Bob, do you want to take a shot at it?
Summary: The experiment name cannot be the same as the namespace when creating an experiment → Experiment names must be lower case
Sure, unless Adam wants to give it a go, as he did most of the follow-up. Do you want to take this bug, Adam?
Sure I'll take it, thanks
Comment on attachment 8890055 [details] Bug 1382819 - Allow non-lowercase names for API extension experiments https://reviewboard.mozilla.org/r/161114/#review166858 This looks good based on what Andrew said in the bug. Thanks for doing this, Adam. I assume you tested this locally to verify it fixed the issue? We should really have an automated test for this too, before landing, but I'm not sure how much test coverage we already have for experiments. If you want to see if you can find the existing tests and find a good place to test this that would be great. If you'd like some help locating the tests let me know and I'll see if I can find them.
Attachment #8890055 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8890055 [details] Bug 1382819 - Allow non-lowercase names for API extension experiments https://reviewboard.mozilla.org/r/161114/#review166858 Yeah it works locally to fix it. I'll have a look and see what I can find for the tests.
Comment on attachment 8890055 [details] Bug 1382819 - Allow non-lowercase names for API extension experiments https://reviewboard.mozilla.org/r/161114/#review170300 This looks great, thanks for finding and fixing this, Adam. The only comment I have is to add a bit more information to the commit message, but this gets my r+. Please request an final review by a peer (aswan is probably the best choice for this) so this can land. ::: commit-message-4b32e:2 (Diff revision 3) > +Bug 1382819 - Allow non-lowercase names for API extension experiments > + Could you please add a comment to the commit message to explain why you updated the test in the manner you did?
Comment on attachment 8890055 [details] Bug 1382819 - Allow non-lowercase names for API extension experiments https://reviewboard.mozilla.org/r/161114/#review170464 I think you changed a lot more than necessary (just the id needs have some upper case, not the namespace). If you want to revise it, you could just capitalize the existing name. But its not necessary, its also fine if you want to land this as is.
Attachment #8890055 - Flags: review?(aswan) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/535faaf4f99d Allow non-lowercase names for API extension experiments r=aswan,bsilverberg
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.