Last Comment Bug 1280777 - put window.oninstall behind a pref
: put window.oninstall behind a pref
Status: RESOLVED FIXED
btpp-active
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla50
Assigned To: Marcos Caceres [:marcosc]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 1285560
Blocks: 1265279
  Show dependency treegraph
 
Reported: 2016-06-18 14:43 PDT by Ben Kelly [not reviewing due to deadline][:bkelly]
Modified: 2016-10-17 06:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Put oninstall behind a pref. (3.11 KB, patch)
2016-06-20 02:20 PDT, Marcos Caceres [:marcosc]
no flags Details | Diff | Splinter Review
Use pushPrefEnv in browser test (7.74 KB, patch)
2016-06-20 21:56 PDT, Marcos Caceres [:marcosc]
no flags Details | Diff | Splinter Review
Slaughtered a poor CPOW with a custom event (9.42 KB, patch)
2016-06-20 23:34 PDT, Marcos Caceres [:marcosc]
no flags Details | Diff | Splinter Review
Add baku as reviewer. (9.42 KB, patch)
2016-06-27 18:31 PDT, Marcos Caceres [:marcosc]
no flags Details | Diff | Splinter Review
Use partial interface instead of mixin (9.67 KB, patch)
2016-06-27 21:41 PDT, Marcos Caceres [:marcosc]
amarchesini: review+
Details | Diff | Splinter Review
Addressed baku's feedback (9.59 KB, patch)
2016-06-29 19:04 PDT, Marcos Caceres [:marcosc]
no flags Details | Diff | Splinter Review

Description User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-06-18 14:43:32 PDT
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 User image Marcos Caceres [:marcosc] 2016-06-20 02:20:15 PDT
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.
Comment 2 User image Marcos Caceres [:marcosc] 2016-06-20 02:23:49 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace734c07384
Comment 3 User image Marcos Caceres [:marcosc] 2016-06-20 02:43:47 PDT
(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 User image Boris Zbarsky [:bz] (still a bit busy) 2016-06-20 07:34:25 PDT
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 User image Marcos Caceres [:marcosc] 2016-06-20 20:54:41 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a366a44f8133
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2016-06-20 21:00:42 PDT
Should browser_fire_install_event.js use pushPrefEnv too?
Comment 7 User image Marcos Caceres [:marcosc] 2016-06-20 21:43:25 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2016-06-20 21:47:25 PDT
> 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 User image Marcos Caceres [:marcosc] 2016-06-20 21:50:06 PDT
> 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 User image Marcos Caceres [:marcosc] 2016-06-20 21:56:09 PDT
Created attachment 8763765 [details] [diff] [review]
Use pushPrefEnv in browser test

r bkelly, as bz is not accepting review requests right now.
Comment 11 User image Marcos Caceres [:marcosc] 2016-06-20 21:57:40 PDT
Cancelled Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a366a44f8133
Comment 13 User image Marcos Caceres [:marcosc] 2016-06-20 22:10:18 PDT
Argh, just realized the browser test is using "contentWindowAsCPOW". Let me kill that.
Comment 14 User image Marcos Caceres [:marcosc] 2016-06-20 23:34:45 PDT
Created attachment 8763775 [details] [diff] [review]
Slaughtered a poor CPOW with a custom event
Comment 16 User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-06-27 09:02:03 PDT
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 User image Marcos Caceres [:marcosc] 2016-06-27 18:22:51 PDT
> 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 User image Marcos Caceres [:marcosc] 2016-06-27 18:26:30 PDT
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 User image Marcos Caceres [:marcosc] 2016-06-27 18:31:03 PDT
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.
Comment 20 User image Marcos Caceres [:marcosc] 2016-06-27 21:37:09 PDT
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 User image Marcos Caceres [:marcosc] 2016-06-27 21:41:15 PDT
Created attachment 8765739 [details] [diff] [review]
Use partial interface instead of mixin
Comment 22 User image Marcos Caceres [:marcosc] 2016-06-27 21:42:07 PDT
Test pass ok ... some kind of miracle :)
Comment 24 User image Andrea Marchesini [:baku] 2016-06-29 00:17:23 PDT
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 User image Marcos Caceres [:marcosc] 2016-06-29 18:09:04 PDT
> 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 User image Marcos Caceres [:marcosc] 2016-06-29 19:04:55 PDT
Created attachment 8766615 [details] [diff] [review]
Addressed baku's feedback
Comment 27 User image Pulsebot 2016-06-29 21:34:15 PDT
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1154e7efc7f7
put window.oninstall behind a pref. r=bkelly r=baku
Comment 28 User image Carsten Book [:Tomcat] 2016-06-30 03:46:53 PDT
https://hg.mozilla.org/mozilla-central/rev/1154e7efc7f7

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