Closed Bug 1093033 Opened 7 years ago Closed 7 years ago

CustomElements in imports should be upgraded too

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → gkrizsanits
Blocks: 1081107
Attached patch CustomElements for imports. v1 (obsolete) — Splinter Review
Attachment #8515931 - Flags: review?(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
Attachment #8515931 - Flags: review?(wchen) → review-
(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.
Attached patch CustomElements for imports. v2 (obsolete) — Splinter Review
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?
Attachment #8515931 - Attachment is obsolete: true
Attachment #8516631 - Flags: review?(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;
}
Attachment #8516631 - Flags: review?(wchen) → review-
(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.
Attachment #8516631 - Attachment is obsolete: true
Attachment #8517421 - Flags: review?(wchen)
Attachment #8517421 - Flags: review?(wchen) → review+
https://hg.mozilla.org/mozilla-central/rev/e2a1f8575950
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.