1093033 - CustomElements in imports should be upgraded too

Status: RESOLVED FIXED

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

Description

[Gabor Krizsanits (INACTIVE)]

The order is undefined for now, and that problem will be taken care of once the spec is fixed (see bug 1081107). But for now we can just do the trivial approach which will do the right thing in simple cases: just register the custom elements in the masters registry for upgrade candidates at the time when they are being parsed.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: CustomElements for imports. v1

Comment 2

[William Chen [:wchen]]

Comment on attachment 8515931 [details] [diff] [review]
CustomElements for imports. v1

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

We should probably use nsDocument::UseRegistryFromDocument when we create the import document and set it to use registry from the main document.

http://w3c.github.io/webcomponents/spec/custom/#creating-and-passing-registries

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to William Chen [:wchen] from comment #2)
> 
> We should probably use nsDocument::UseRegistryFromDocument when we create
> the import document and set it to use registry from the main document.
> 
> http://w3c.github.io/webcomponents/spec/custom/#creating-and-passing-
> registries

Yeah, you're probably right... I've been struggling with this "use" term. Share would be a better word then I think. I was not sure to what extend should it use it, like, only for registering upgrade candidates like I did, or for import.registerElement too...

Anyway, after reading it again, I believe sharing is the right way to do it indeed.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Attachment: CustomElements for imports. v2

I ended up calling it in ResetToURI, since we don't set scriptglobal for imports, and SetMasterDocument is called too early (before ResetToURI). What do you think?

Comment 5

[William Chen [:wchen]]

Comment on attachment 8516631 [details] [diff] [review]
CustomElements for imports. v2

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

::: dom/base/nsDocument.cpp
@@ +2305,5 @@
>    mInUnlinkOrDeletion = oldVal;
>  
>    mRegistry = nullptr;
>  
> +  if (mMasterDocument) {

It looks like the problem was that you were calling UseRegistryFromDocument when you created the import document, then it loaded and called ResetToURI and cleared the registry.

I think we should still call UseRegistryFromDocument somewhere near where you call SetMasterDocument on the import document, then we should change to code here to something like:

if (!mMasterDocument) {
  // "When creating an import, use the registry of the master document."
  // Note: at this point the mMasterDocument is already set for imports
  // (and only for imports)
  mRegistry = nullptr;
}

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: CustomElements for imports. v3

(In reply to William Chen [:wchen] from comment #5)
> It looks like the problem was that you were calling UseRegistryFromDocument
> when you created the import document, then it loaded and called ResetToURI
> and cleared the registry.

Exactly.

> 
> I think we should still call UseRegistryFromDocument somewhere near where
> you call SetMasterDocument on the import document, then we should change to
> code here to something like:

Sure, we can do that too. Probably makes more sense.

Comment 7

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee93c58a23ce

Comment 8

[Gabor Krizsanits (INACTIVE)]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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