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

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

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().

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

We already have a concept of "display document" which means something different, fwiw.  See nsIDocument::GetDisplayDocument.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

GetRenderDoc()?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Works for me.  I suspect we'll want to replace pretty much all the GetDocument/GetCurrentDoc() callsites with that, but we'll see.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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() ?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[William Chen [:wchen]]

If we wanted to align with the spec terminology we would probably want:

IsInRootTree(), GetRootTreeDoc(), IsInComposedTree(), GetComposedTreeDoc()

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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()...

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

IsInCoreDoc() / GetCoreDoc()
IsInComposedDoc() / GetComposedDoc()
?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I still suspect that RenderDoc will be more intuitively understandable to more gecko hackers than ComposedDoc. And it's shorter to type.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

But you may have documents without any rendering. That is why I don't like RenderDoc

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

But I still prefer composed/uncomposed. That forces the API user to think about shadow dom composition.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposal 1

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(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 18

[Boris Zbarsky [:bzbarsky]]

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());

?

r=me

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Sure. (I'm hoping to improve GetComposedDocInternal perf by having a flag for the state when it would
return non-null.)

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: faster IsInComposedDoc

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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