ArchiveReader should live behind a pref

RESOLVED FIXED in Firefox 17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mounir, Assigned: baku)

Tracking

({dev-doc-needed})

Trunk
mozilla19
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Blocks: 772434
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?
(Reporter)

Comment 3

5 years ago
(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.
(Reporter)

Comment 4

5 years ago
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?
Assignee: nobody → amarchesini
mozArchiveReader ?
Yeah, we should rename this to MozArchiveReader.
Created attachment 678564 [details] [diff] [review]
patch
Attachment #678564 - Flags: review?(mounir)
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;
Keywords: dev-doc-needed
(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?
(Reporter)

Comment 11

5 years ago
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.
Attachment #678564 - Flags: review?(mounir) → review-
(Reporter)

Comment 12

5 years ago
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-firefox17: --- → ?
tracking-firefox18: --- → ?
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.
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Sure. So I'll go for a pref approach. Patch soon.
(Reporter)

Comment 15

5 years ago
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.
Created attachment 679324 [details] [diff] [review]
patch pref
Attachment #678564 - Attachment is obsolete: true
Attachment #679324 - Flags: review?(mounir)
Created attachment 679335 [details] [diff] [review]
patch pref

forgot a test.
Attachment #679324 - Attachment is obsolete: true
Attachment #679324 - Flags: review?(mounir)
Attachment #679335 - Flags: review?(mounir)
Created attachment 679458 [details] [diff] [review]
patch

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

Green on try.
Attachment #679335 - Attachment is obsolete: true
Attachment #679335 - Flags: review?(mounir)
Attachment #679458 - Flags: review?(mounir)
(Reporter)

Updated

5 years ago
Summary: ArchiveReader should be prefixed → ArchiveReader should live behind a pref
(Reporter)

Comment 19

5 years ago
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.
Attachment #679458 - Flags: review?(mounir) → review-
(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.
Created attachment 679682 [details] [diff] [review]
patch 2
Attachment #679458 - Attachment is obsolete: true
Attachment #679682 - Flags: review?(mounir)
(Reporter)

Comment 22

5 years ago
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.
Attachment #679682 - Flags: review?(mounir) → review+
(Reporter)

Comment 23

5 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla19
Created attachment 679693 [details] [diff] [review]
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
Attachment #679682 - Attachment is obsolete: true
Attachment #679693 - Flags: review+
Attachment #679693 - Flags: approval-mozilla-beta?
Attachment #679693 - Flags: approval-mozilla-aurora?
Keywords: dev-doc-needed
(Reporter)

Comment 25

5 years ago
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?
Keywords: dev-doc-needed
(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/mozilla-central/rev/77378c09bbb5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #679693 - Flags: approval-mozilla-beta?
Attachment #679693 - Flags: approval-mozilla-beta+
Attachment #679693 - Flags: approval-mozilla-aurora?
Attachment #679693 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/83b3764cce45
https://hg.mozilla.org/releases/mozilla-beta/rev/593cf8318f49
status-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox19: --- → fixed
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.