Enable Web Components by default for certified apps

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: overholt, Assigned: wchen)

Tracking

(Blocks 1 bug, {dev-doc-needed})

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
This will let some gaia developers and others more easily give feedback.  William is hoping to do this before the end of June 2014.
I'd like to see this also on the Aurora/Nightly channels for desktop, rather than using a polyfill.
Reporter

Comment 2

5 years ago
(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.
Assignee

Comment 3

5 years ago
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.
Attachment #8448889 - Flags: review?(gkrizsanits)
Attachment #8448889 - Flags: review?(bugs)
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
Attachment #8448889 - Flags: review?(bugs) → review-
Assignee

Comment 5

5 years ago
(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.
Attachment #8449057 - Flags: review?(gkrizsanits)
Attachment #8449057 - Flags: review?(bugs)
Attachment #8449057 - Flags: review?(bugs) → review+
Attachment #8448889 - Attachment is obsolete: true
Attachment #8448889 - Flags: review?(gkrizsanits)
Attachment #8449057 - Flags: review?(gkrizsanits) → review+
https://hg.mozilla.org/mozilla-central/rev/b6c82c9d6b8c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee

Comment 8

5 years ago
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
Attachment #8449057 - Flags: approval-mozilla-aurora?
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?"
Attachment #8449057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.