Closed Bug 1026047 (shadowcurrentdoc) Opened 7 years ago Closed 7 years ago

Figure out what to do to Shadow DOM's IsInDoc/GetCurrentDoc()


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

29 Branch
Not set





(Reporter: smaug, Assigned: smaug)


(Blocks 4 open bugs)



(2 files)

Bug 992521 broke some functionality because it added new invariants
("Not-in-doc-but-has-nsIFrames" etc).

In bug 992521 only some parts of code was checked whether changes for the
IsInDoc/GetCurrentDoc/GetDocument usage are needed.

I think we should re-evaluate the IsInDoc behavior we want and then
go through _all_ the code using IsInDoc()/GetCurrentDoc()/GetDocument().
And we should also rename IsInDoc() and GetCurrentDoc(), and get rid of the ancient GetDocument().
Summary: Figure out how to handle Shadow DOM's IsInDoc/GetCurrentDoc() → Figure out what to do to Shadow DOM's IsInDoc/GetCurrentDoc()
My suggestion is to add a GetDisplayDoc() which returns the same thing as GetCrossShadowCurrentDoc().

Then change the layout code to use this function as appropriate.

And yes, I too would love to see GetDocument() go away. It has been slated for death for a decade now I think :)
We already have a concept of "display document" which means something different, fwiw.  See nsIDocument::GetDisplayDocument.
Works for me.  I suspect we'll want to replace pretty much all the GetDocument/GetCurrentDoc() callsites with that, but we'll see.
I think we need to also rename GetCurrentDoc() and IsInDoc() so that whoever uses them realizes
in case of shadow dom null/false is always returned.

We should have a name for the document which doesn't contain shadow stuff, and then for the tree which contains also shadow dom.
RenderDoc sounds ok-ish for the latter, or ComposedDoc, that would be closer to the spec terminology, and doesn't
hint anything about rendering, which shadow dom doesn't necessarily have.

BaseDoc for the first one? DOMDoc would be nice, but has different meaning in Gecko.
Or even UncomposedDoc? Looks ugly and is a bit long, but would be rather clear.

So IsInUncomposedDoc()/GetUncomposedDoc() and IsInComposedDoc()/GetComposedDoc()  and remove IsInDoc()/GetCurrentDoc()/GetDocument()/GetCrossShadowCurrentDoc() ?
So I like IsInComposedDoc()/GetComposedDoc().

IsInUncomposedDoc() is not great, though.  How about IsInDocumentTree()?  And maybe GetDocumentTreeRoot() instead of GetUncomposedDoc()?  Not totally happy with that one, unlike IsInDocumentTree().

We definitely want to remove the things you list as needing removal.
If we wanted to align with the spec terminology we would probably want:

IsInRootTree(), GetRootTreeDoc(), IsInComposedTree(), GetComposedTreeDoc()
IsInDocumentTree is ok, but GetDocumentTreeRoot()...
I think the name should indicate that the return value is a Document object.

IsInRootTree() doesn't really indicate the node needs to be in composed (or uncomposed) doc - just that it is in a tree of subtrees.
Alias: shadowcurrentdoc
> I think the name should indicate that the return value is a Document object.

GetDocumentRootOfTree?  I still don't like it.  Maybe GetRootDocument()?  That might confuse people in terms of frame/subframe relationship.

GetRootAsDocument?  GetTreeRootAsDocument?  GetTreeRootDoc?

I agree that IsInRootTree() doesn't mean what we want.  That just says we're in the light DOM; a root tree may not have a document root.  I'm not sure that IsInComposedTree() does what we want either: you can be in a composed tree that's not rooted at a document, right?

The spec term for the thing we want is "document tree" hence IsInDocumentTree()...
When not thinking shadow dom, IsInDocumentTree() sounds pretty much the same as IsInDoc(), 
and doesn't really say much whether we're talking about DOM tree, or flattened tree, IMO.
(shadom dom spec doesn't talk about flattened tree.)
Sure, 'document tree' is a spec term, but not necessarily very good one.
IsInCoreDoc() / GetCoreDoc()
IsInComposedDoc() / GetComposedDoc()
I still suspect that RenderDoc will be more intuitively understandable to more gecko hackers than ComposedDoc. And it's shorter to type.
But you may have documents without any rendering. That is why I don't like RenderDoc
But I still prefer composed/uncomposed. That forces the API user to think about shadow dom composition.
Attached patch proposal 1Splinter Review
So this add methods for the following cases (which I think cover all the interesting cases. We haven't had anything like GetRenderDoc and I don't see why we'd need such)
- is the node in a DOM tree where Document object is the root?
- if the node is in a DOM tree where Document object is the root, return the Document object.
- is the node in flattened tree where a Document object is the root?
- if the node is in a flattened tree where a Document object is the root, return the Document object.
- is the node in a shadow dom.

Use of composed/uncomposed force, IMO, the caller to think whether shadow dom should be taken care of.
If uncomposed is replaced with something else, that something should make it clear that shadow dom isn't being handled.
Attachment #8445437 - Flags: review?(bzbarsky)
Note, once we propagate is-in-doc state to shadow trees (via BindToTree, I think), we can
improve performance of these checks for shadow dom nodes a bit.
(In reply to Olli Pettay [:smaug] from comment #16)
> Note, once we propagate is-in-doc state to shadow trees (via BindToTree, I
> think),
and I'm not talking about the current IsInDoc(), but the stuff that needs to happen when
an element is added to a composed tree, which has doc as root.
Comment on attachment 8445437 [details] [diff] [review]
proposal 1

OK, let's just go with these names for now.  They're searchable enough we can do a mass rename later if we find something better.

>+  bool IsInComposedDoc() const
>+  {
>+    return IsInUncomposedDoc() || GetComposedDoc();

This will do extra work for the not-in-shadow-tree, not in doc case, no?

How about:

  return IsInUncomposedDoc() || (IsInShadowTree() &&  GetComposedDocInternal());


Attachment #8445437 - Flags: review?(bzbarsky) → review+
Sure. (I'm hoping to improve GetComposedDocInternal perf by having a flag for the state when it would
return non-null.)
Assignee: nobody → bugs
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1037610
Blocks: 1037612
Blocks: 1037643
Blocks: 1037664
Blocks: 1037668
Blocks: 1037687
Blocks: 1037709
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.