put window.oninstall behind a pref

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: bkelly, Assigned: marcosc)

Tracking

({dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8763490 [details] [diff] [review]
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.
Attachment #8763490 - Flags: feedback?(bkelly)
(Assignee)

Updated

a year ago
Summary: put windows.oninstall behind a pref → put window.oninstall behind a pref
(Assignee)

Comment 2

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace734c07384
(Assignee)

Comment 3

a year ago
(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)
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a366a44f8133
Should browser_fire_install_event.js use pushPrefEnv too?
(Assignee)

Comment 7

a year ago
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)
(Assignee)

Comment 9

a year ago
> 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.
(Assignee)

Comment 10

a year ago
Created attachment 8763765 [details] [diff] [review]
Use pushPrefEnv in browser test

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

Comment 11

a year ago
Cancelled Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a366a44f8133
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40923cb77fe7
(Assignee)

Comment 13

a year ago
Argh, just realized the browser test is using "contentWindowAsCPOW". Let me kill that.
(Assignee)

Comment 14

a year ago
Created attachment 8763775 [details] [diff] [review]
Slaughtered a poor CPOW with a custom event
Attachment #8763765 - Attachment is obsolete: true
Attachment #8763765 - Flags: review?(bkelly)
Attachment #8763775 - Flags: review?(bkelly)
(Assignee)

Comment 15

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e892d278d74
Whiteboard: btpp-active
(Reporter)

Comment 16

a year ago
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+
(Assignee)

Comment 17

a year ago
> 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.
(Assignee)

Comment 18

a year ago
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)
(Assignee)

Comment 19

a year ago
Created attachment 8765691 [details] [diff] [review]
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.
Attachment #8763775 - Attachment is obsolete: true
Attachment #8763775 - Flags: review?(amarchesini)
Attachment #8765691 - Flags: review?(amarchesini)
(Assignee)

Comment 20

a year ago
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).
(Assignee)

Comment 21

a year ago
Created attachment 8765739 [details] [diff] [review]
Use partial interface instead of mixin
Attachment #8765691 - Attachment is obsolete: true
Attachment #8765691 - Flags: review?(amarchesini)
Attachment #8765739 - Flags: review?(amarchesini)
(Assignee)

Comment 22

a year ago
Test pass ok ... some kind of miracle :)
(Assignee)

Comment 23

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43420cfb5fb7
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+
(Assignee)

Comment 25

a year ago
> 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);
(Assignee)

Comment 26

a year ago
Created attachment 8766615 [details] [diff] [review]
Addressed baku's feedback
Attachment #8765739 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 27

a year ago
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

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1154e7efc7f7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

a year ago
Depends on: 1285560
Keywords: dev-doc-needed
Updated:
https://developer.mozilla.org/en-US/docs/Web/API/Window/oninstall
https://developer.mozilla.org/en-US/docs/Web/Events/install
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.