Last Comment Bug 795930 - ArchiveReader should live behind a pref
: ArchiveReader should live behind a pref
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Andrea Marchesini (:baku)
:
Mentors:
Depends on:
Blocks: 772434
  Show dependency treegraph
 
Reported: 2012-10-01 09:00 PDT by Mounir Lamouri (:mounir)
Modified: 2013-04-04 13:53 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
fixed


Attachments
patch (34.00 KB, patch)
2012-11-05 17:27 PST, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
patch pref (9.43 KB, patch)
2012-11-07 12:44 PST, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch pref (10.14 KB, patch)
2012-11-07 12:58 PST, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch (10.48 KB, patch)
2012-11-07 17:13 PST, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
patch 2 (10.26 KB, patch)
2012-11-08 07:40 PST, Andrea Marchesini (:baku)
mounir: review+
Details | Diff | Review
patch (10.24 KB, patch)
2012-11-08 08:18 PST, Andrea Marchesini (:baku)
amarchesini: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2012-10-01 09:00:37 PDT
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) 2012-10-02 07:32:02 PDT
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) 2012-10-09 08:19:18 PDT
resolve -> WONTFIX?
sicking, feedback?
Comment 3 Mounir Lamouri (:mounir) 2012-10-09 08:30:54 PDT
(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) 2012-11-05 10:22:16 PST
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) 2012-11-05 10:30:31 PST
mozArchiveReader ?
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-11-05 14:42:09 PST
Yeah, we should rename this to MozArchiveReader.
Comment 7 Andrea Marchesini (:baku) 2012-11-05 17:27:53 PST
Created attachment 678564 [details] [diff] [review]
patch
Comment 8 Masatoshi Kimura [:emk] 2012-11-05 18:46:05 PST
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) 2012-11-05 23:14:02 PST
(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 [:bz] (Out June 25-July 6) 2012-11-06 08:25:01 PST
Can we ship this preffed off instead of prefixed?
Comment 11 Mounir Lamouri (:mounir) 2012-11-07 09:07:14 PST
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) 2012-11-07 09:10:13 PST
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 2012-11-07 11:58:28 PST
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) 2012-11-07 12:03:07 PST
Sure. So I'll go for a pref approach. Patch soon.
Comment 15 Mounir Lamouri (:mounir) 2012-11-07 12:23:34 PST
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) 2012-11-07 12:44:24 PST
Created attachment 679324 [details] [diff] [review]
patch pref
Comment 17 Andrea Marchesini (:baku) 2012-11-07 12:58:23 PST
Created attachment 679335 [details] [diff] [review]
patch pref

forgot a test.
Comment 18 Andrea Marchesini (:baku) 2012-11-07 17:13:40 PST
Created attachment 679458 [details] [diff] [review]
patch

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

Green on try.
Comment 19 Mounir Lamouri (:mounir) 2012-11-08 04:05:36 PST
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) 2012-11-08 05:45:05 PST
(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) 2012-11-08 07:40:47 PST
Created attachment 679682 [details] [diff] [review]
patch 2
Comment 22 Mounir Lamouri (:mounir) 2012-11-08 07:53:08 PST
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) 2012-11-08 08:01:52 PST
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) 2012-11-08 08:18:04 PST
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
Comment 25 Mounir Lamouri (:mounir) 2012-11-08 12:36:28 PST
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] 2012-11-08 12:48:22 PST
(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] 2012-11-08 17:02:11 PST
https://hg.mozilla.org/mozilla-central/rev/77378c09bbb5

Note You need to log in before you can comment on or make changes to this bug.