Closed Bug 1280777 Opened 4 years ago Closed 4 years ago

put window.oninstall behind a pref

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: marcosc)

References

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active)

Attachments

(1 file, 5 obsolete files)

Bug 1265279 landed support for windows.oninstall, but we don't have product integration for the feature yet.  This means the install event will never fire in practice.

We should put this event handler behind a pref for now so we people can feature detect if its really there.  Once we land product support for installing manifests we can flip the pref on and eventually remove the pref.
Attached patch Put oninstall behind a pref. (obsolete) — Splinter Review
Ben, I slapped this together quickly. Let's see what the try-server says about it, but any initial feedback would be appreciated. Can then ask Boris or someone who knows a bit more about putting things "behind a pref" to review.
Attachment #8763490 - Flags: feedback?(bkelly)
Summary: put windows.oninstall behind a pref → put window.oninstall behind a pref
(Boris, bit of context: trying to put 'window.oninstall' behind a pref but not sure what the right way of doing that is... should I maybe just make it only be exposed in Nightly instead of trying to enable it with a pref? or can it actually be dynamically enabled with a pref?)

Ok, yeah... so, that didn't quite work... I tried also:

SpecialPowers.setBoolPref("dom.manifest.oninstall", true);
SpecialPowers.pushPrefEnv({"set": [['dom.manifest.oninstall', true]]}, doTest);

Maybe one can't enable/disable WebIDL attributes on the fly using prefs?
Flags: needinfo?(bzbarsky)
WebIDL conditional annotations for an interface are evaluate once per global: when the interface prototype object is created.  The right way to write tests that toggle such prefs is to set the pref (via pushPrefEnv, please, not setBoolPref!) then once the pref is set create a new global to test in: either window.open or creating an iframe.
Flags: needinfo?(bzbarsky)
Should browser_fire_install_event.js use pushPrefEnv too?
Sorry, I thought `pushPrefEnv` was only on the SpecialPowers interface on the content side? 

If it's exposed in browser_ tests, I certainly can use pushPrefEnv... but is it necessary given the the pref is set before the window is constructed (and hence I should be able to set the preference sync there)?
Flags: needinfo?(bzbarsky)
> Sorry, I thought `pushPrefEnv` was only on the SpecialPowers interface on the content side? 

http://searchfox.org/mozilla-central/search?q=pushPrefEnv&case=false&regexp=false&path=browser_*.js%24 suggests not.

> but is it necessary given the the pref is set before the window is constructed
> (and hence I should be able to set the preference sync there)?

Yes, but what _unsets_ it so you don't pollute the environment for later tests?  pushPrefEnv handles this for you.
Flags: needinfo?(bzbarsky)
> Yes, but what _unsets_ it so you don't pollute the environment for later tests?  pushPrefEnv handles this for you.

Ok, cool. Will switch over.
Attached patch Use pushPrefEnv in browser test (obsolete) — Splinter Review
r bkelly, as bz is not accepting review requests right now.
Attachment #8763490 - Attachment is obsolete: true
Attachment #8763490 - Flags: feedback?(bkelly)
Attachment #8763765 - Flags: review?(bkelly)
Argh, just realized the browser test is using "contentWindowAsCPOW". Let me kill that.
Attachment #8763765 - Attachment is obsolete: true
Attachment #8763765 - Flags: review?(bkelly)
Attachment #8763775 - Flags: review?(bkelly)
Whiteboard: btpp-active
Comment on attachment 8763775 [details] [diff] [review]
Slaughtered a poor CPOW with a custom event

Review of attachment 8763775 [details] [diff] [review]:
-----------------------------------------------------------------

I did not really dig into the tests, but overall looks reasonable.  You will need an official DOM peer to sign off on the webidl changes.  Thanks.

::: dom/manifest/test/test_window_oninstall_event.html
@@ +30,5 @@
> +    ]);
> +    const ops = {
> +      "set": [...prefs.entries()],
> +    };
> +    return SpecialPowers.pushPrefEnv(ops);

This seems like a very verbose way of doing this.  Why not just:

return SpecialPowers.pushPrefEnv({'set' : [['dom.manifest.oninstall', true]]});
Attachment #8763775 - Flags: review?(bkelly) → review+
> return SpecialPowers.pushPrefEnv({'set' : [['dom.manifest.oninstall', true]]});

We might end up adding/enabling more prefs, so I'd rather just keep with the more verbose way.
Comment on attachment 8763775 [details] [diff] [review]
Slaughtered a poor CPOW with a custom event

Baku, could you kindly review the IDL change (just puts the `oninstall` attribute behind a pref).
Attachment #8763775 - Flags: review+ → review?(amarchesini)
Attached patch Add baku as reviewer. (obsolete) — Splinter Review
Baku, if it's all ok, can you please label as "checkin needed". Given our TZ difference, it's likely I'll be sleep when you review this.
Attachment #8763775 - Attachment is obsolete: true
Attachment #8763775 - Flags: review?(amarchesini)
Attachment #8765691 - Flags: review?(amarchesini)
In the spec, we just decided to change from using a mixin style WebIDL to a `partial interface`. The change should have no effect on how the api is exposed in JS... (famous last words; compiling, testing now).
Attachment #8765691 - Attachment is obsolete: true
Attachment #8765691 - Flags: review?(amarchesini)
Attachment #8765739 - Flags: review?(amarchesini)
Test pass ok ... some kind of miracle :)
Comment on attachment 8765739 [details] [diff] [review]
Use partial interface instead of mixin

Review of attachment 8765739 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/manifest/test/browser_fire_install_event.js
@@ +14,5 @@
> +  ]);
> +  const ops = {
> +    "set": [...prefs.entries()],
> +  };
> +  return SpecialPowers.pushPrefEnv(ops);

return SpecialPowers.pushPrefEnv({'set': [['dom.manifest.oninstall', true]]});

I like your syntax, but everywhere we use this more compact version.

::: dom/manifest/test/test_window_oninstall_event.html
@@ +30,5 @@
> +    ]);
> +    const ops = {
> +      "set": [...prefs.entries()],
> +    };
> +    return SpecialPowers.pushPrefEnv(ops);

here too
Attachment #8765739 - Flags: review?(amarchesini) → review+
> return SpecialPowers.pushPrefEnv({'set': [['dom.manifest.oninstall', true]]});

code beautifier cleans that up tho, so went with:

    const ops = {
      "set": [
        ["dom.manifest.oninstall", true],
      ],
    };
    return SpecialPowers.pushPrefEnv(ops);
Attachment #8765739 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1154e7efc7f7
put window.oninstall behind a pref. r=bkelly r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1154e7efc7f7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1285560
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.