1000199 - Enable Web Components by default for certified apps

Status: RESOLVED FIXED

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

Description

[Andrew Overholt [:overholt]]

This will let some gaia developers and others more easily give feedback.  William is hoping to do this before the end of June 2014.

Comment 1

[Kohei Yoshino]

I'd like to see this also on the Aurora/Nightly channels for desktop, rather than using a polyfill.

Comment 2

[Andrew Overholt [:overholt]]

(In reply to Kohei Yoshino [:kohei] from comment #1)
> I'd like to see this also on the Aurora/Nightly channels for desktop, rather
> than using a polyfill.

Last I heard, there were still pending spec changes so we shouldn't put it on pre-release (or any) channels without a pref until that is sorted out.

Comment 3

[William Chen [:wchen]]

Attachment: Enable web components for certified apps.

Gaia is using web components by turning on the flag in B2G, but this enables web components globally and essentially exposes it to the web on that platform. We need to change all the web components API checks to also check for certified apps, remove the flag on B2G and make sure these changes gets uplifted.

Comment 4

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

Comment on attachment 8448889 [details] [diff] [review]
Enable web components for certified apps.

> static uint32_t ToLinkMask(const nsAString& aLink)
> { 
>   if (aLink.EqualsLiteral("prefetch"))
>     return nsStyleLinkElement::ePREFETCH;
>   else if (aLink.EqualsLiteral("dns-prefetch"))
>     return nsStyleLinkElement::eDNS_PREFETCH;
>   else if (aLink.EqualsLiteral("stylesheet"))
>     return nsStyleLinkElement::eSTYLESHEET;
>   else if (aLink.EqualsLiteral("next"))
>     return nsStyleLinkElement::eNEXT;
>   else if (aLink.EqualsLiteral("alternate"))
>     return nsStyleLinkElement::eALTERNATE;
>-  else if (aLink.EqualsLiteral("import") && nsStyleLinkElement::IsImportEnabled())
>+  else if (aLink.EqualsLiteral("import"))
This feels a bit error prone. Couldn't you keep IsImportEnabled and require one to pass nsIPrincipal.
and then
else if (aLink.EqualsLiteral("import") && aPrincipal && nsStyleLinkElement::IsImportEnabled(aPrincipal))


>+      } else if (linkTypes & eHTMLIMPORT &&
>+                 nsStyleLinkElement::IsImportEnabled(NodePrincipal())) {
Then you wouldn't need this

Comment 5

[William Chen [:wchen]]

Attachment: Enable web components for certified apps. v2

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8448889 [details] [diff] [review]
> Enable web components for certified apps.
> 
> This feels a bit error prone. Couldn't you keep IsImportEnabled and require
> one to pass nsIPrincipal.
> and then
> else if (aLink.EqualsLiteral("import") && aPrincipal &&
> nsStyleLinkElement::IsImportEnabled(aPrincipal))
ToLinkMask, and its only consumer (ParseLinkTypes) are pretty simple static functions that map strings to enums and it doesn't seem worth it to require that the callers provide a principal when there are only two callers that care about an eHTMLIMPORT return value (both of which already end up checking IsImportEnabled()). It seems to me that the IsImportEnabled() check in ToLinkMask wasn't necessary in the first place. I understand your concern about this being a little error prone, but I think that any behaviour that needs to be gated by IsImportEnabled() is better done with an explicit check rather than checking eHTMLIMPORT on the link mask.

I've created a new patch with your review comments addressed. I've left the old patch up in case you've changed your mind.

Comment 6

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c82c9d6b8c

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6c82c9d6b8c

Comment 8

[William Chen [:wchen]]

Comment on attachment 8449057 [details] [diff] [review]
Enable web components for certified apps. v2

Gaia 2.0 currently has web component features turned on via a preference, this has a consequence of enabling the APIs for all of the web, but these APIs are experimental and intended only for certified apps right now. This patch enable web components by default for certified apps and will let us remove the preference from Gaia.

Approval Request Comment
[Feature/regressing bug #]: Bug 1006118
[User impact if declined]: Users will be exposed to DOM API that is not ready to be exposed to the general web.
[Describe test coverage new/current, TBPL]: There are many web component tests running on TBPL.
[Risks and why]: Low risk, this patch enables web component features for certified apps that are already being used in Gaia.
[String/UUID change made/needed]: None

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8449057 [details] [diff] [review]
Enable web components for certified apps. v2

Aurora+ 

:overholt - Glad to see we're asking "Is this good for the WEB?"

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/936ff3b9a5ce