1181555 - Allow usage of web components in privileged apps when they opt-in with a permission

Status: RESOLVED FIXED

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

Description

[Chris Lord [:cwiiis]]

The web components/shadow DOM spec is certainly in heavy flux, but this is a core technology that we've been using in Gaia, and writing maintainable, large web applications is very hard without some kind of components system (and I'd posit that having to use pre-processors/build systems to provide it is also raising the barrier by an uncomfortable amount).

I think it's bad that we've relied on this technology heavily in Gaia, but not made it available to app developers to use in apps distributed on the marketplace.

I suggest that we add an 'experimental-webcomponents' permission that privileged apps can use, in the interim between now and the spec being stabilised and implemented (which we can't put a date on).

Having 'experimental' in the name is an admission that you as a developer know this is not a stable feature and may disappear/change in future versions, and this will inform your code (and you can inform your users) as such.

On a self-centered note, I need this if the new homescreen prototype is to remain both a privileged app and with a reasonably clean code-base that requires no pre-processing.

A follow-up would be to use this permission in certified apps as well. Currently, certified apps have no option but to get our implementation of web components, which is missing several core features from other popular implementations and shims.

The ideal situation would be the spec stabilises and we implement it in time for FirefoxOS 2.5, and we can just remove this permission. I dare say that this won't happen in time, however, and I think this is a reasonable compromise.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

Adds a permission 'experimental-webcomponents' that privileged apps can use to opt-in to our web components implementation.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8631022 [details] [diff] [review]
0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

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

::: dom/base/nsDocument.cpp
@@ +5859,5 @@
> +    return true;
> +  }
> +
> +  // Privileged apps are allowed if they have the experimental-webcomponents
> +  // permission.

The permission level is controlled by the permission table, so you should not do any app status check here anymore. Just check for the permission on the document's principal.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

Don't check twice for privileged app status.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I wish the name of the permission hinted that the API is something which will be mostly replaced with something a bit different - so the name should hint that if you use the permission, expect your stuff to be broken later.

Comment 5

[Chris Lord [:cwiiis]]

(In reply to Olli Pettay [:smaug] from comment #4)
> I wish the name of the permission hinted that the API is something which
> will be mostly replaced with something a bit different - so the name should
> hint that if you use the permission, expect your stuff to be broken later.

I thought 'experimental' was enough for that, but open to suggestions?

Comment 6

[Boris Zbarsky [:bzbarsky]]

"totally-unstable-and-will-change-webcomponents"?

More importantly, what sort of leverage do we have in terms of getting the privileged apps you describe updated when (not if) we break them?  With gaia, we just control the code so can fix it, which is pretty important in terms of allowing web component usage there.

On a technical note, you should be using "obj", not "aObject" if you want to pass the rooting static analysis.

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> "totally-unstable-and-will-change-webcomponents"?

This seems a bit wordy to me :) I assume that we'll document this permission (I don't think you can stumble onto these?), and in the documentation we can be as wordy as we like about how much of a bad idea it may or may not be to use this feature.

> More importantly, what sort of leverage do we have in terms of getting the
> privileged apps you describe updated when (not if) we break them?  With
> gaia, we just control the code so can fix it, which is pretty important in
> terms of allowing web component usage there.

Privileged apps are reviewed and hosted on the marketplace. When we change our implementation, we can hide any apps that have this permission, I would suppose (I assume we'd also inform any apps that they'd been hidden).

> On a technical note, you should be using "obj", not "aObject" if you want to
> pass the rooting static analysis.

In this line, I assume?:

JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, aObject));

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Chris Lord [:cwiiis] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #6)
> > "totally-unstable-and-will-change-webcomponents"?
> 
> This seems a bit wordy to me :) I assume that we'll document this permission
> (I don't think you can stumble onto these?), and in the documentation we can
> be as wordy as we like about how much of a bad idea it may or may not be to
> use this feature.

The danger is people cargo-culting the permission without understanding the consequences.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

"experimental" sounds at least to me something which will be probably non-experimental in the future, but in this case the API to be exposed is something which as such will die soon, and a new similar API will be added then.

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Chris Lord [:cwiiis] from comment #7)

> 
> Privileged apps are reviewed and hosted on the marketplace. When we change
> our implementation, we can hide any apps that have this permission, I would
> suppose (I assume we'd also inform any apps that they'd been hidden).

That's not so simple. Some people will have devices running with the current implementation, so for the marketplace to serve the right apps to the right people, we also need to add something to navigator.hasFeature/getFeature, and make the changes on the marketplace side. 
The other option is to have a marketplace policy that specify which apps can use the new permission. We did that before for Loop in order to get it in the marketplace. That doesn't scale well obviously.

Given comment #9 about the upcoming changes, I don't think that we should expose our current implementation to 3rd party apps yet.

Comment 11

[Chris Lord [:cwiiis]]

That's fair, let's see what happens in the next month or so and reevaluate.

I'm still convinced that we should make either an updated implementation or what we have now available in the next release.

Comment 12

[Guillaume Marty [:gmarty]]

What about prefixing the permission with a version number and bump it whenever the web components implementation changes in a non backward compatible way?
e.g. 'experimental-webcomponents-v1', then when the new spec lands, rename the permission to 'experimental-webcomponents-v2'.
To address Boris's comment on comment 6, the version number clearly means "things will change in the future".
Apps can request both permissions but will have to make sure that the code is compatible in both cases using feature detection or a polyfill.

Alternatively, the reason why we need that is to make replaceable homescreens easier to build and distribute as the new implementation is based on web components. We're already opening datastore to privileged homescreens in Bug 1181329. Maybe we could do the same here and enable web components to homescreen apps only for now?
This is of course very specific and I'd rather go for the solution described above.

As a 3rd party developer, I find it really frustrating not to be able to reuse gaia-components (https://github.com/gaia-components) on my apps. Currently it's hard to build an app that integrates nicely in the look and feel of the OS. So I hope web components will be enable soon to all types of apps.

Comment 13

[Chris Lord [:cwiiis]]

Not quite a month, but 3 weeks and as I understood it, there were going to have been meetings by this point. n?smaug to see where we are with web components now.

We'd love to be able to use web components in privileged apps in b2g 2.5. The cut-off date for 2.5 is end of October, will we have a stable implementation by then that we can ship? Or pref on with a permission?

If not, can I suggest the permission name 'fxos2.5-experimental-webcomponents', which we can enable *only* on FirefoxOS 2.5, with the necessary documentation explaining that it will go away in subsequent versions?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

anne participated the meeting, so he can comment on the stability of the web components.

As far as I know, we don't know yet what web components API will look like, not from Shadow DOM
perspective nor from Custom Elements.


fxos2.5-experimental-webcomponents sounds way to optimistic permission name to me. The name really should hint that the API you get is something that will definitely disappear (or will be changed significantly).

Comment 15

[Chris Lord [:cwiiis]]

What about 'temporary-webcomponents-fxos2.5-only'? Any suggestions?

Comment 16

[Anne (:annevk)]

I share the concerns from comment 6 (and that name suggestion from bz makes sense if we decide to do it anyway).

Shadow DOM: There is agreement on a new form. We will change our implementation and ship it. It will be incompatible with Gaia. 

Custom elements: No plans yet. What we have today for custom elements will not get standardized. 

HTML imports: Remove (they might make a return, but not in the same form).

I'm not sure what William's schedule is with regards to Shadow DOM and HTML imports, and some of it may depend on specification updates happening.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

I think it's pretty clear at this point that we aren't going to have an updated implementation of the parts that make up web components for Firefox OS 2.5.

It'd be great to get this permission in for then so that at least the homescreen can be a privileged app and we can open up opportunities for third parties to experiment.

With regards to FirefoxOS apps, we could just not accept apps with this permission on the marketplace if we think it's too dangerous, but it'd be nice to start making steps towards this.

I understand if we absolutely can't have this; if that's the case, please say.

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 8649310 [details] [diff] [review]
0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

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

The code is ok, but I really don't know if we should do that or not. Jonas?

::: dom/apps/PermissionsTable.jsm
@@ +579,4 @@
>                               privileged: DENY_ACTION,
>                               certified: ALLOW_ACTION
>                             },
> +                           "totally-unstable-and-will-change-webcomponents": {

I would rather use moz-webcomponents as the moz- prefix is the sign for the marketplace that this is a "special" permission.

::: dom/base/nsDocument.cpp
@@ +5821,4 @@
>    JS::Rooted<JSObject*> obj(aCx, aObject);
> +  if (IsInCertifiedApp(aCx, obj)) {
> +    return true;
> +  }

If we use the permission table, no need to hardcode certified apps here anymore, unless you are worried about the performance.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8649310 [details] [diff] [review]
0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

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

Yes, I'm fine with doing this.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8649310 [details] [diff] [review]
0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

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

OK, so r=me with comments addressed.

Comment 21

[Chris Lord [:cwiiis]]

Thanks for the quick attention folks!

I just want to clarify the permission name - moz-webcomponents-unstable-and-will-change?

Comment 22

[Anne (:annevk)]

If we're doing this, I'd suggest moz-extremely-unstable-webcomponents. We are planning changes in platform. If it's still unclear when 2.5 will branch, you might get bitten.

Comment 23

[Chris Lord [:cwiiis]]

ok, moz-extremely-unstable-and-will-change-webcomponents? Wordy, but I suppose there's really no mistaking what it implies...

wrt Fabrice's certified-check comment in the review, I left that in because certified apps need to get the permission even if they don't ask for it.

Comment 24

[[:fabrice] Fabrice Desré]

Well, certified apps can ask for it. Explicit is better than implicit in general.

Comment 25

[Chris Lord [:cwiiis]]

(In reply to [:fabrice] Fabrice Desré from comment #24)
> Well, certified apps can ask for it. Explicit is better than implicit in
> general.

I agree, but currently we'd have to change every app in Gaia (and I suppose inform any partners making certified apps with Gaia shared components?) - do we want to do that? And if so, how should we coordinate that?

I'm going to assume that 'moz-extremely-unstable-and-will-change-webcomponents' is ok unless anyone says otherwise.

n?me to deal with this tomorrow.

Comment 26

[[:fabrice] Fabrice Desré]

(In reply to Chris Lord [:cwiiis] from comment #25)
> (In reply to [:fabrice] Fabrice Desré from comment #24)
> > Well, certified apps can ask for it. Explicit is better than implicit in
> > general.
> 
> I agree, but currently we'd have to change every app in Gaia (and I suppose
> inform any partners making certified apps with Gaia shared components?) - do
> we want to do that? And if so, how should we coordinate that?

Write a gaia patch that adds the permission to all gaia apps that use web components, and land it before the gecko patch. r=me
Send and email to dev-gaia to warn partners.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We have dependencies between Gaia and Gecko all the time, this is really no different. Basically, whenever we check in a patch to change gecko here, we'll have to at the same time land a patch to Gaia to update to the new syntax.

3rd party developers that use this will simply be broken. They should not expect to be notified. That is why this is experimental.

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1181555-webcomponents-permission > mozilla-b2g:master

Comment 29

[Chris Lord [:cwiiis]]

Comment on attachment 8650411 [details] [review]
[gaia] Cwiiis:bug1181555-webcomponents-permission > mozilla-b2g:master

Carrying r=fabrice (see comment #26)

Comment 30

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1181555-Add-experimental-webcomponents-permissio.patch

Carrying r=fabrice, f=jonas

Comment 31

[Chris Lord [:cwiiis]]

Merged Gaia patch: https://github.com/mozilla-b2g/gaia/commit/8f0e7b9b7b3c3611010220365a412d4bcb484857

Waiting on a green try run before merging Gecko patch to inbound. Not expecting any failures, but in case there's a test that I haven't seen that's affected.

Comment 32

[Chris Lord [:cwiiis]]

Pushed revised patch to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d838084096

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Linux x64 opt and B2G Desktop Linux x64 opt Hazard analysis fail: Found 1 hazards and 41 unsafe references

Comment 34

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1bd7b7c0d13

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbffbd9aef3

Comment 36

[Chris Lord [:cwiiis]]

Sorry about that, I missed this (can you get the hazard check to run on try?)

Think I've fixed the error, pushed to inbound again: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbffbd9aef3

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bbffbd9aef3

Comment 38

[Kohei Yoshino]

Why don't you allow all apps to use Web components? I do know the spec is still unstable, but this is not a security sensitive API, and it's difficult to ask Firefox OS users to flip their pref. I believe that having a priority seat, or allowing only specific apps to use the latest Web standards, is a horrible idea especially at Mozilla.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Why don't you allow all apps to use Web components? 

Because then they will break when we change the implementation as the spec evolves.  Note that this is a "when", not "if"; we know what we have implemented right now looks nothing like the final spec will in all sorts of ways.

Comment 40

[Wilson Page [:wilsonpage]]

(In reply to Kohei Yoshino [:kohei] from comment #38)
> Why don't you allow all apps to use Web components? I do know the spec is
> still unstable, but this is not a security sensitive API, and it's difficult
> to ask Firefox OS users to flip their pref. I believe that having a priority
> seat, or allowing only specific apps to use the latest Web standards, is a
> horrible idea especially at Mozilla.

We can't allow all apps to use them because if/when APIs change, apps that have already started using them will break. Allowing internal use of APIs before opening them up to the web allows us to test features and find bugs early, so that when we switch them on they're rock solid :)

Comment 41

[Kohei Yoshino]

I'm aware of the situation. I'm caring about site compatibility stuff by running https://www.fxsitecompat.com/. But, I'm really disappointed in Mozilla for having such a priority seating policy. Why don't you just allow all apps to use the API *with warnings of potential future breakages* and ask bug reporting?

Comment 42

[Kohei Yoshino]

(In reply to Kohei Yoshino [:kohei] from comment #41)
> Why don't you just allow all apps to use the API

I meant: all apps *opted in* with this permission flag

Comment 43

[Boris Zbarsky [:bzbarsky]]

> Why don't you just allow all apps to use the API

Because when an app that we wrote breaks, we can fix it.  But when some other app breaks... what are our options, exactly?

Now maybe you're arguing that we shouldn't use the API ourselves in production code.  I'm moderately sympathetic to that argument.