I do not understand why this feature has landed unprefixed. Is it stable enough? AFAIUI, there isn't a real specification for that. If that's the case, we should clearly keep it prefixed to prevent any breakage with non-backward compatible changes and to make clearer that the feature is still under development.

I think it’s best to do anything *except* prefix it. If it’s not ready for use, let’s disable it on the release channel. If it is ready enough to be shipped on the release channel, let’s ship unprefixed and let the future to be constrained by the usage that accumulates (i.e. refrain from making changes that break Web content relying on the shipped API). Shipping it with a funny name won’t make Web content break less if we change/remove it later. See http://hsivonen.iki.fi/vendor-prefixes/

resolve -> WONTFIX? sicking, feedback?

(In reply to Andrea Marchesini (:baku) from comment #2) > resolve -> WONTFIX? I do not think it is as simple. Prefixing in DOM and in CSS is quite different: prefixing in DOM can be used to do feature check and could make sense IMO. I'm not fully supporting that solution over other solutions, but I do not think it is as bad as the CSS situation. I think we should have a DOM/WebAPI policy regarding prefixing. Right now, it's up to each individual to decide. Also, I do not think ArchiveReader is a mature API. It has been designed and implemented quite quickly. There is no specification, no other implementation and the comments in the whatwg list about the API were not numerous. I'm afraid that pushing this API as-is would be some kind of forcing. I do not think it is badly designed or implement, far from that, just that I'm not sure it is correct to design, implement and ship an API without a minimum of consensus.

Andrea, we had a discussion with Jonas last week and we both believe that we should not ship this unprefixed for the moment. Could you write a patch to prefix this API?

mozArchiveReader ?

Yeah, we should rename this to MozArchiveReader.

Please do not repeat the mistake of indexedDB when writing the document (see bug 770844). We should never write an example such as: var ArchiveReader = window.ArchiveReader || window.MozArchiveReader;

(In reply to Mounir Lamouri (:mounir) from comment #4) > Andrea, we had a discussion with Jonas last week and we both believe that we > should not ship this unprefixed for the moment. Could you write a patch to > prefix this API? Can you please reconsider? Prefixing will just invite pain like http://lists.w3.org/Archives/Public/www-style/2012Nov/0043.html (most likely through the mechanism shown in comment 8).

Can we ship this preffed off instead of prefixed?

Comment on attachment 678564 [details] [diff] [review] patch I think we should hide this behind a pref and make the pref disabled by default. Ideally, I think we should try to make new features hidden behind a pref and have a way at compile time to know if we are compiling for Nightly, Aurora, Beta or Release and have a default value depending on this. For example, we clearly want this feature in Nightly and Aurora channels but probably not yet in Beta and Release. Maybe some features should be in Beta but not Release. Anyway, that discussion should probably happen in dev-platform so better to just pref'd off the ArchiveReader for the moment.

Note that we should probably track this for Firefox 17 and Firefox 18 because this feature isn't mature yet and during the discussion at TPAC, it didn't get much traction. Webkit/Apple wasn't certain we were solving the right use cases with the right tools with this.

Tracking - can someone prepare the patch to pref this off in 17? We'll want that tested on trunk before uplifting to our final beta.

Sure. So I'll go for a pref approach. Patch soon.

Andrea, given that there is a very short time frame for Beta, could you simply attach here a back out patch for Beta? Backing out should be less risky than adding a pref. We should definitely have a pref for the other releases.

forgot a test.

https://hg.mozilla.org/try/rev/52dff514410b Green on try.

Comment on attachment 679458 [details] [diff] [review] patch Review of attachment 679458 [details] [diff] [review]: ----------------------------------------------------------------- r- because of the tests. ::: dom/file/ArchiveReader.cpp @@ +50,5 @@ > { > NS_ENSURE_TRUE(aArgc == 1 || aArgc == 2, NS_ERROR_INVALID_ARG); > > + if (!PrefEnabled()) { > + return NS_ERROR_DOM_SECURITY_ERR; We should not be there if the pref is not enabled so I think NS_ERROR_UNEXPECTED would be better. ::: dom/file/ArchiveReader.h @@ +48,5 @@ > nsresult GetInputStream(nsIInputStream** aInputStream); > nsresult GetSize(uint64_t* aSize); > > +public: // Pref Enabled > + static bool PrefEnabled(); nit: the comment isn't useful and I'm not sure why you have a |public:| statement for each methods here. This is odd and not part of our coding style. ::: dom/file/test/test_archivereader.html @@ +57,5 @@ > handleFinished = 0; > function markTestDone() { > ++handleFinished; > if (isFinished()) { > + SpecialPowers.setBoolPref("dom.archivereader.enabled", archiveReaderEnabled); You should be calling finishTest() here (from helpers.js). And I think it might be a good idea to handle the pref there. @@ +65,5 @@ > function isFinished() { > return handleFinished == 6; > } > > + archiveReaderEnabled = false; var archiveReaderEnabled = false; Not putting |var| adds the variable to the global scope (window). It's a very bad habit :) @@ +70,4 @@ > function testSteps() > { > + archiveReaderEnabled = SpecialPowers.getBoolPref("dom.archivereader.enabled"); > + SpecialPowers.setBoolPref("dom.archivereader.enabled", true); This is the generator... so basically, if it is called twice, |archiveReaderEnabled| value will be |true| even if the initial value is actually |false|. Put that outside of this method. Ideally, you can add this to dom/file/tests/helpers.js, in runTest(). ::: dom/file/test/test_archivereader_nonUnicode.html @@ +80,4 @@ > function testSteps() > { > + archiveReaderEnabled = SpecialPowers.getBoolPref("dom.archivereader.enabled"); > + SpecialPowers.setBoolPref("dom.archivereader.enabled", true); See previous comments. ::: dom/file/test/test_archivereader_zip_in_zip.html @@ +39,4 @@ > function testSteps() > { > + archiveReaderEnabled = SpecialPowers.getBoolPref("dom.archivereader.enabled"); > + SpecialPowers.setBoolPref("dom.archivereader.enabled", true); See previous comments. ::: dom/indexedDB/test/test_blob_archive.html @@ +13,5 @@ > > function testSteps() > { > + let archiveReaderEnabled = SpecialPowers.getBoolPref("dom.archivereader.enabled"); > + SpecialPowers.setBoolPref("dom.archivereader.enabled", true); See previous comments.

(In reply to Mounir Lamouri (:mounir) from comment #11) > I think we should hide this behind a pref and make the pref disabled by > default. Thank you.

Comment on attachment 679682 [details] [diff] [review] patch 2 Review of attachment 679682 [details] [diff] [review]: ----------------------------------------------------------------- r=me Thank you for this patch :) I will land this.

http://hg.mozilla.org/integration/mozilla-inbound/rev/77378c09bbb5 BTW, Andrea, please, make sure to not attach patches with wrong commit messages. I pushed your patch once assuming it was fine and then realized the commit message was (again) a commit message from another patch. Also, please ask for mozilla-aurora and mozilla-beta approval as soon as the patch lands in m-c.

[Approval Request Comment] Bug caused by (feature/regressing bug #): ArchiveReader API is not standard/stable enough User impact if declined: none Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): none

dev-doc-needed is useful so doc writers know that this feature has been disabled in Firefox 17 and Firefox 18 (and trunk). Also, pointing how to enable the feature for developers in the doc might be interesting. Hmmm, maybe you removed the keyword because you wrote the doc actually?

(In reply to Mounir Lamouri (:mounir) from comment #25) > dev-doc-needed is useful so doc writers know that this feature has been > disabled in Firefox 17 and Firefox 18 (and trunk). Also, pointing how to > enable the feature for developers in the doc might be interesting. Fair enough. I removed because I thought the keyword would not be required anymore no prefix was added. > Hmmm, maybe you removed the keyword because you wrote the doc actually? No. If I wrote the document, I would have changed the keyword to dev-doc-complete.

https://hg.mozilla.org/releases/mozilla-aurora/rev/83b3764cce45 https://hg.mozilla.org/releases/mozilla-beta/rev/593cf8318f49