Closed Bug 1075050 Opened 10 years ago Closed 10 years ago

GetCurrentDoc fixes in content/xul

Categories

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

34 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Comment on attachment 8497669 [details] [diff] [review]
v1

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

r+ with the comments addressed.

::: content/xul/content/src/nsXULElement.cpp
@@ +1414,5 @@
>  already_AddRefed<nsIXULTemplateBuilder>
>  nsXULElement::GetBuilder()
>  {
>      // XXX sXBL/XBL2 issue! Owner or current document?
> +    nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(GetComposedDoc());

How about we use the uncomposed document here and not support the template builder with shadow DOM.

@@ +1919,5 @@
>  
>  nsIWidget*
>  nsXULElement::GetWindowWidget()
>  {
> +    nsIDocument* doc = GetComposedDoc();

I think this should be using the uncomposed doc because the consumers are doing things like setting titlebar properties and I'm not sure this is something we want to support in the shadow DOM given that we don't even allow <title> in the shadow tree to set the title. Also, the special XUL elements that use this probably won't ever end up in a shadow DOM anyways.

::: content/xul/document/src/XULDocument.cpp
@@ +1706,5 @@
>  
>  NS_IMETHODIMP
>  XULDocument::AddSubtreeToDocument(nsIContent* aContent)
>  {
> +    NS_ASSERTION(aContent->GetComposedDoc() == this, "Element not in doc!");

I think we should use the uncomposed doc here because we aren't even using the flattened tree in the code below and I'm not sure we want to support this with the composed tree.

::: content/xul/templates/src/nsXULContentBuilder.cpp
@@ +1056,5 @@
>                                             &container, &newIndexInContainer);
>      }
>  
>      if (aNotifyAtEnd && container) {
> +        MOZ_AUTO_DOC_UPDATE(container->GetComposedDoc(), UPDATE_CONTENT_MODEL,

I don't think we should be trying to support shadow DOM here.

::: content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp
@@ +153,5 @@
>      nsCOMPtr<nsIContent> root = do_QueryInterface(aRootNode);
>      if (!root)
>          return NS_ERROR_UNEXPECTED;
>  
> +    nsCOMPtr<nsIDocument> doc = root->GetComposedDoc();

I feel that this should be using the uncomposed document. If somehow we are provided a datasource that exists in a shadow tree, we should just treat it like as if they provided a node from some random document fragment and let it error. I don't think there will be any good trying to make this template query processor work with shadow DOM especially since this kind of interaction isn't specified at all and probably won't be.
Attachment #8497669 - Flags: review?(wchen) → review+
> @@ +1919,5 @@
> >  
> >  nsIWidget*
> >  nsXULElement::GetWindowWidget()
> >  {
> > +    nsIDocument* doc = GetComposedDoc();
> 
> I think this should be using the uncomposed doc because the consumers are
> doing things like setting titlebar properties and I'm not sure this is
> something we want to support in the shadow DOM given that we don't even
> allow <title> in the shadow tree to set the title. Also, the special XUL
> elements that use this probably won't ever end up in a shadow DOM anyways.
Well, I think a method like this should just work, and it is up to the caller to check if they are in
shadow dom. (and they do so currently)
Attached patch v2Splinter Review
Assignee: nobody → bugs
https://hg.mozilla.org/mozilla-central/rev/b35c63fe50da
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.