795930 - ArchiveReader should live behind a pref

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

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.

Comment 1

[Henri Sivonen (:hsivonen)]

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/

Comment 2

[Andrea Marchesini [:baku]]

resolve -> WONTFIX?
sicking, feedback?

Comment 3

[Mounir Lamouri (:mounir)]

(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.

Comment 4

[Mounir Lamouri (:mounir)]

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?

Comment 5

[Andrea Marchesini [:baku]]

mozArchiveReader ?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, we should rename this to MozArchiveReader.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 8

[Masatoshi Kimura [:emk]]

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;

Comment 9

[Henri Sivonen (:hsivonen)]

(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).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Can we ship this preffed off instead of prefixed?

Comment 11

[Mounir Lamouri (:mounir)]

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.

Comment 12

[Mounir Lamouri (:mounir)]

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.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 14

[Andrea Marchesini [:baku]]

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

Comment 15

[Mounir Lamouri (:mounir)]

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.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch pref

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch pref

forgot a test.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch

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

Green on try.

Comment 19

[Mounir Lamouri (:mounir)]

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.

Comment 20

[Henri Sivonen (:hsivonen)]

(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 21

[Andrea Marchesini [:baku]]

Attachment: patch 2

Comment 22

[Mounir Lamouri (:mounir)]

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.

Comment 23

[Mounir Lamouri (:mounir)]

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.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch

[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

Comment 25

[Mounir Lamouri (:mounir)]

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?

Comment 26

[Masatoshi Kimura [:emk]]

(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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77378c09bbb5

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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