Closed Bug 887538 Opened 11 years ago Closed 10 years ago

Implement web components shadow element.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: wchen, Assigned: wchen)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 2 obsolete files)

Attached patch Implement HTMLShadowElement (obsolete) — Splinter Review
Attachment #798090 - Flags: review?(mrbkap)
Comment on attachment 798090 [details] [diff] [review]
Implement HTMLShadowElement

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

I have a bunch of questions and would like to see a lot more comments explaining what's going on. It'd be especially nice if some of the new comments referred to specific parts of the spec.

::: content/base/src/ChildIterator.cpp
@@ +101,4 @@
>      mChild = mChild->GetNextSibling();
>    }
>  
> +  if (ShadowRoot::IsShadowInsertionPoint(mChild)) {

Comment what's going on here.

::: content/base/src/ChildIterator.h
@@ +66,4 @@
>    // we continue iterating at mChild's next sibling.
>    nsIContent* mDefaultChild;
>  
> +  nsAutoPtr<ExplicitChildIterator> mShadowIterator;

Please add a comment explaining what this is.

::: content/base/src/ShadowRoot.cpp
@@ +293,4 @@
>    HTMLContentElement* insertionPoint = nullptr;
>    for (uint32_t i = 0; i < mInsertionPoints.Length(); i++) {
>      if (mInsertionPoints[i]->Match(aContent)) {
> +      if (mInsertionPoints[i]->MatchedNodes().Contains(aContent)) {

Out of curiosity, why is this in this patch?

@@ +340,5 @@
>        matchedNodes.AppendElement(aContent);
>      }
>  
> +    // Handle the case where insertion point is distributed to
> +    // a <shadow> element.

For all of these cases, can you explain what the consequence is? Here, it isn't clear why the insertion point being distributed to a shadow element means that we need to call MatchSingleNode one the younger shadow's shadow element, for example.

@@ +414,4 @@
>  {
>    // Create node pool.
>    nsTArray<nsIContent*> nodePool;
> +  // Make sure there is a pool host, an older shadow may not have

Uber-nit: blank line before the comment.

@@ +567,4 @@
>   */
> +bool
> +ShadowRoot::IsPooledNode(nsIContent* aContent, nsIContent* aContainer,
> +                         nsIContent* aHost) {

Nit: { on its own line.

@@ +609,5 @@
>  {
> +  if (mInsertionPointChanged) {
> +    MatchAllNodes();
> +    mInsertionPointChanged = false;
> +  }

Why wouldn't we return after calling MatchAllNodes?

::: content/base/src/ShadowRoot.h
@@ +59,5 @@
> +
> +  /**
> +   * Change the node that is parent of the nodes which will end up in
> +   * the pool of the pool population algorithm. This is distinct from
> +   * the DocumentFragment host that is used to prevent cycles in the DOM.

Prevent cycles in the DOM?

@@ +136,5 @@
>    nsRefPtr<ShadowRootStyleSheetList> mStyleSheetList;
> +  HTMLShadowElement* mShadowElement;
> +  nsRefPtr<ShadowRoot> mOlderShadow;
> +  nsRefPtr<ShadowRoot> mYoungerShadow;
> +  bool mInsertionPointChanged;

Comments describing these members would be nice.

::: content/html/content/src/HTMLShadowElement.cpp
@@ +130,5 @@
> +void
> +HTMLShadowElement::UnbindFromTree(bool aDeep, bool aNullParent)
> +{
> +  if (mIsInsertionPoint) {
> +    ShadowRoot* rootShadow = GetRootShadow();

Where's the definition of GetRootShadow?

@@ +138,5 @@
> +      rootShadow->ShadowDescendants().RemoveElement(this);
> +      rootShadow->SetShadowElement(nullptr);
> +
> +      nsTArray<HTMLShadowElement*>& shadowDescendants = rootShadow->ShadowDescendants();
> +      if (shadowDescendants.Length() > 0) {

Please add comments describing more clearly what's happening here. It's not immediately obvious.

@@ +154,5 @@
> +
> +void
> +HTMLShadowElement::MatchSingleNode(nsIContent* aContent)
> +{
> +  ShadowRoot* parentShadowRoot = GetParent()->GetShadowRoot();

Need more comments describing what's happening here and why. Why are we getting the parent's shadow root? What does it mean if it's null?

::: content/html/content/src/HTMLShadowElement.h
@@ +61,5 @@
> +  ShadowRoot* GetOlderShadowRoot() { return mProjectedShadow; }
> +
> +protected:
> +  virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +  nsRefPtr<ShadowRoot> mProjectedShadow;

I would like to see more comments describing the terminology here. What is a "projected" shadow? The term doesn't appear in the spec.

::: parser/htmlparser/src/nsElementTable.cpp
@@ +420,5 @@
>      /*parent,leaf*/ kFormControl, false
>    },
>    {
> +    /*tag*/         eHTMLTag_shadow,
> +    /*parent,leaf*/ kNone, false

Nit: the parent should probably be kFlowEntity.
Attachment #798090 - Flags: review?(mrbkap)
Attached patch Implement HTMLShadowElement (obsolete) — Splinter Review
(In reply to Blake Kaplan (:mrbkap) from comment #3)
> Comment on attachment 798090 [details] [diff] [review]
> Implement HTMLShadowElement
> 
> Review of attachment 798090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/ShadowRoot.cpp
> @@ +293,4 @@
> >    HTMLContentElement* insertionPoint = nullptr;
> >    for (uint32_t i = 0; i < mInsertionPoints.Length(); i++) {
> >      if (mInsertionPoints[i]->Match(aContent)) {
> > +      if (mInsertionPoints[i]->MatchedNodes().Contains(aContent)) {
> 
> Out of curiosity, why is this in this patch?
> 
It occurred to me while working on this patch that I should check to see if the node has already been distributed, it could just as easily go in an earlier patch.
> 
> @@ +609,5 @@
> >  {
> > +  if (mInsertionPointChanged) {
> > +    MatchAllNodes();
> > +    mInsertionPointChanged = false;
> > +  }
> 
> Why wouldn't we return after calling MatchAllNodes?
Good catch, it should return right after.
> 
> @@ +61,5 @@
> > +  ShadowRoot* GetOlderShadowRoot() { return mProjectedShadow; }
> > +
> > +protected:
> > +  virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> > +  nsRefPtr<ShadowRoot> mProjectedShadow;
> 
> I would like to see more comments describing the terminology here. What is a
> "projected" shadow? The term doesn't appear in the spec.
Hmmm, the term "projected" and "reprojected" used to be in the spec. It's too bad that it was removed because I thought it was a good way to describing the concept of nodes being rendered in place of insertion points.
> 
I've added a lot more comments so hopefully it answers the questions you had in your other comments.
Attachment #798090 - Attachment is obsolete: true
Attachment #820835 - Flags: review?(mrbkap)
Comment on attachment 820835 [details] [diff] [review]
Implement HTMLShadowElement

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

Closer, still some stuff to fix though.

::: content/base/src/ChildIterator.cpp
@@ +105,5 @@
>    }
>  
> +  // If the current child being iterated is a shadow insertion point then
> +  // the iterator needs to go into the projected ShadowRoot.
> +  if (ShadowRoot::IsShadowInsertionPoint(mChild)) {

I think this block needs to be combined with the while loop under it. Otherwise, if the first <shadow> child of a shadow DOM is immediately after a non-matched no-default-content <content> element we'll return the <shadow> instead of its projection. Also, add a test for this!

@@ +227,5 @@
>    }
>  
> +  // If the current child being iterated is a shadow insertion point then
> +  // the iterator needs to go into the projected ShadowRoot.
> +  if (ShadowRoot::IsShadowInsertionPoint(mChild)) {

Ditto.

::: content/base/src/ShadowRoot.cpp
@@ +270,5 @@
> +  mYoungerShadow = aYoungerShadow;
> +  mYoungerShadow->mOlderShadow = this;
> +
> +  nsIContent* newPoolHost = mYoungerShadow->GetShadowElement();
> +  ChangePoolHost(newPoolHost);

Nit: no need for the single-use variable.

@@ +331,5 @@
> +    // that is projected into the younger ShadowRoot's shadow insertion point.
> +    // The node distributed into the insertion point must be reprojected
> +    // to the shadow insertion point.
> +    if (insertionPoint->GetParent() == this) {
> +      if (mYoungerShadow && mYoungerShadow->GetShadowElement()) {

Nit (here and below): combine the nested if statements.

@@ +410,5 @@
>  void
>  ShadowRoot::DistributeAllNodes()
>  {
>    // Create node pool.
>    nsTArray<nsIContent*> nodePool;

Uber-nit: blank line before the comment.

::: content/base/src/ShadowRoot.h
@@ +110,4 @@
>    }
>  
> +  nsIContent* GetPoolHost() { return mPoolHost; }
> +  nsTArray<HTMLShadowElement*>& ShadowDescendants() { return mShadowDescendants; }

Nit: be consistent as to whether these one liners go on one line or three.

@@ +130,5 @@
>      GetElementsByClassName(const nsAString& aClasses);
>    void GetInnerHTML(nsAString& aInnerHTML);
>    void SetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError);
>  protected:
> +  nsCOMPtr<nsIContent> mPoolHost;

Add a comment referring to the comment above SetPoolHost.

::: content/html/content/src/HTMLShadowElement.cpp
@@ +35,5 @@
> +                                                nsGenericHTMLElement)
> +  if (tmp->mProjectedShadow) {
> +    tmp->mProjectedShadow->RemoveMutationObserver(tmp);
> +  }
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mProjectedShadow)

I'd just set tmp->mProjectedShadow = nullptr; in the if statement.

@@ +83,5 @@
> +
> +  return false;
> +}
> +
> +class TreeOrderComparator {

Can you combine this with the version of this exact comparator found in HTMLPropertiesCollection.cpp in nsNodeUtils?

@@ +117,5 @@
> +      // Only the first <shadow> (in tree order) of a ShadowRoot can be an insertion point.
> +      return NS_OK;
> +    }
> +
> +    if (IsInFallbackContent(this)) {

Need code before here to unset the current shadow root, even if this isn't a valid shadow root (e.g. if it's in fallback content).

I wonder if we should file a spec bug on this? It's kind of odd that it wouldn't be the first *valid* <shadow> element.

@@ +137,5 @@
> +    ShadowRoot* rootShadow = GetRootShadow();
> +    // Make sure that rootShadow exists, it may have been nulled
> +    // during unlinking in which case the ShadowRoot is going away.
> +    if (rootShadow) {
> +      rootShadow->ShadowDescendants().RemoveElement(this);

I guess this doesn't need to be RemoveElementSorted because this element must be first element in the array?

@@ +141,5 @@
> +      rootShadow->ShadowDescendants().RemoveElement(this);
> +      rootShadow->SetShadowElement(nullptr);
> +
> +      // Find the next shadow insertion point.
> +      nsTArray<HTMLShadowElement*>& shadowDescendants = rootShadow->ShadowDescendants();

You can pull this above the RemoveElement(this); call.

@@ +143,5 @@
> +
> +      // Find the next shadow insertion point.
> +      nsTArray<HTMLShadowElement*>& shadowDescendants = rootShadow->ShadowDescendants();
> +      if (shadowDescendants.Length() > 0) {
> +        if (!IsInFallbackContent(shadowDescendants[0])) {

Nit: combine nested ifs.
Attachment #820835 - Flags: review?(mrbkap)
Attached patch v1 diff v2Splinter Review
(In reply to Blake Kaplan (:mrbkap) from comment #5)
> Comment on attachment 820835 [details] [diff] [review]
> Implement HTMLShadowElement
> 
> Review of attachment 820835 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +117,5 @@
> > +      // Only the first <shadow> (in tree order) of a ShadowRoot can be an insertion point.
> > +      return NS_OK;
> > +    }
> > +
> > +    if (IsInFallbackContent(this)) {
> 
> Need code before here to unset the current shadow root, even if this isn't a
> valid shadow root (e.g. if it's in fallback content).
> 
> I wonder if we should file a spec bug on this? It's kind of odd that it
> wouldn't be the first *valid* <shadow> element.
> 
Yeah, it does seem strange. I'll file a spec bug as followup.
> @@ +137,5 @@
> > +    ShadowRoot* rootShadow = GetRootShadow();
> > +    // Make sure that rootShadow exists, it may have been nulled
> > +    // during unlinking in which case the ShadowRoot is going away.
> > +    if (rootShadow) {
> > +      rootShadow->ShadowDescendants().RemoveElement(this);
> 
> I guess this doesn't need to be RemoveElementSorted because this element
> must be first element in the array?
Yes, and generally this array should be tiny. If it's more than one element in size, then the author is doing something wrong because there shouldn't be multiple shadow element.

Thanks for the thourough review Blake.

All comments addressed.
Attachment #820835 - Attachment is obsolete: true
Attachment #8339658 - Flags: review?(mrbkap)
Comment on attachment 8339658 [details] [diff] [review]
Implement HTMLShadowElement v2

This looks great!
Attachment #8339658 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/f592ebe28538
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: