1280777 - put window.oninstall behind a pref

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Marcos Caceres [:marcosc]]

Attachment: Put oninstall behind a pref.

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.

Comment 2

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace734c07384

Comment 3

[Marcos Caceres [:marcosc]]

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

Comment 4

[Boris Zbarsky [: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.

Comment 5

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a366a44f8133

Comment 6

[Boris Zbarsky [:bzbarsky]]

Should browser_fire_install_event.js use pushPrefEnv too?

Comment 7

[Marcos Caceres [:marcosc]]

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)?

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Marcos Caceres [:marcosc]]

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

Comment 10

[Marcos Caceres [:marcosc]]

Attachment: Use pushPrefEnv in browser test

r bkelly, as bz is not accepting review requests right now.

Comment 11

[Marcos Caceres [:marcosc]]

Cancelled Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a366a44f8133

Comment 12

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40923cb77fe7

Comment 13

[Marcos Caceres [:marcosc]]

Argh, just realized the browser test is using "contentWindowAsCPOW". Let me kill that.

Comment 14

[Marcos Caceres [:marcosc]]

Attachment: Slaughtered a poor CPOW with a custom event

Comment 15

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e892d278d74

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

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]]});

Comment 17

[Marcos Caceres [:marcosc]]

> 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 18

[Marcos Caceres [:marcosc]]

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

Comment 19

[Marcos Caceres [:marcosc]]

Attachment: Add baku as reviewer.

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.

Comment 20

[Marcos Caceres [:marcosc]]

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

Comment 21

[Marcos Caceres [:marcosc]]

Attachment: Use partial interface instead of mixin

Comment 22

[Marcos Caceres [:marcosc]]

Test pass ok ... some kind of miracle :)

Comment 23

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43420cfb5fb7

Comment 24

[Andrea Marchesini [:baku]]

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

Comment 25

[Marcos Caceres [:marcosc]]

> 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);

Comment 26

[Marcos Caceres [:marcosc]]

Attachment: Addressed baku's feedback

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1154e7efc7f7
put window.oninstall behind a pref. r=bkelly r=baku

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1154e7efc7f7

Comment 29

[Florian Scholz (Open Web Docs)]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/Window/oninstall
https://developer.mozilla.org/en-US/docs/Web/Events/install