Closed Bug 1331322 Opened 3 years ago Closed 3 years ago

Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(11 files, 1 obsolete file)

6.86 KB, patch
Details | Diff | Splinter Review
3.63 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.98 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
39.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.02 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
38.02 KB, patch
bholley
: review+
heycam
: superreview+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
bholley
: review+
Details
Splitting this out of bug 1330843.
Summary: Encode all pseudo-implementing native anonymous content with the pseudo type at creation time → Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time
Assignee: nobody → bobbyholley
This is the base functionality split out of bug 1330843. I still need to
implement the new inheritance strategy described in bug 1331047 comment 11.
Flags: needinfo?(bobbyholley)
Depends on: 1334247
Depends on: 1334358
Duplicate of this bug: 1331045
Note to self: I was concerned that this would regress the issue described in bug 939248, but it doesn't seem to.
Flags: needinfo?(bobbyholley)
This is only called during URI resolution, and the proptable bit should make
this relatively fast.
Attachment #8831588 - Flags: review?(bzbarsky)
This fits a bit better with the other stuff, and allows us to add our new NAC
bit with the other NAC related bits, which also happens to be a field that
Servo already has easy access to.
Attachment #8831589 - Flags: review?(bzbarsky)
Comment on attachment 8831588 [details] [diff] [review]
Part 1 - Stop using a node bit for HasExplicitBaseURI. v1

So did it turn out that the dir attribute flags were not amenable to having one of them removed?

    // Set if node has a dir attribute with a valid value (ltr, rtl, or auto)
    NodeHasValidDirAttribute,
    // Set if node has a dir attribute with a fixed value (ltr or rtl, NOT auto)
    NodeHasFixedDir,
    // Set if the node has dir=auto.
    NodeHasDirAuto,

I guess, looking at it just now, one problem is <bdi>, which has NodeHasDirAuto when its dir attribute is not set?

But still, looking at nsGenericHTMLElement::AfterSetAttr, there are only four possible states corresponding to these three flags:

  NodeHasValidDirAttribute && NodeHasDirAuto (dir="auto")
  NodeHasValidDirAttribute && NodeHasFixedDir (dir="rtl" or dir="ltr")
  NodeHasDirAuto (no dir attr, <bdi> element)
  no flags (no dir attr, other elements)

so we should really be able to encode this into two bits, right?  Might still be worth doing, even if we nix the base URI thing.

>+    auto maybeURI = static_cast<nsIURI*>(GetProperty(nsGkAtoms::baseURIProperty, &status));

So there are two problems here:

1)  You need to check HasProperties().  No, GetProperty does not normally do that, afaict.
2)  Why do you care about the status?  The docs for GetProperty are clear that null is returned if not present, so you can just do:

  return static_cast<nsIURI*>(GetProperty(nsGkAtoms::baseURIProperty));

like we used to.  r=me with at least those two problems fixed, but please do at least file a followup on the dir thing....
Attachment #8831588 - Flags: review?(bzbarsky) → review+
> I guess, looking at it just now, one problem is <bdi>

I just checked, and the SetHasDirAuto() bit in nsHTMLInputElement doesn't seem like it changes anything about the setup either.

So it seems to me like we can map the existing 3 bits to two bits named "NodeHasValidDirAttribute" and "NodeHasDirAuto" like so:

  NodeHasValidDirAttribute() => NodeHasValidDirAttribute()
  NodeHasFixedDir() => NodeHasValidDirAttribute() && !NodeHasDirAuto()
  NodaHasDirAuto() => NodeHasDirAuto()
Comment on attachment 8831589 [details] [diff] [review]
Part 2 - Move MAY_HAVE_CLASS to mBoolFlags. v1

> nsIContent::DoGetClasses() const

Can you file a followup to move GetClasses/DoGetClasses to Element?  It should be pretty straightforward based on some quick searching; feel free to assign it to me.

It's too bad that moving this flag to element-specific flags helps nothing, because elements are our most flag-constrained nodes anyway....

r=me
Attachment #8831589 - Flags: review?(bzbarsky) → review+
Comment on attachment 8831590 [details] [diff] [review]
Part 3 - Add a flag to indicate that a node is native anonymous content. v1

>+   * Returns true of |this| is native anonymous (i.e. created by

s/of/if/

I guess the reason this is a flag, not a bool bit (i.e. the reason for the MAY_HAVE_CLASS change), is so you can do the bitwise or in SetIsNativeAnonymousRoot?

> ConnectAnonymousTreeDescendants(nsIContent* aParent,

As the comments say, this function is a convenience.  Anonymous content creators do not have to actually create a tree of nsIAnonymousContentCreator::ContentInfo.  They can just hook things up themselves.  And some do just that.

What I think you should do is that inside ConnectAnonymousTreeDescendants, before you connect anything, you check whether aParent->IsNativeAnonymous() and if so walk over the subtree rooted at aParent and mark them all NODE_IS_NATIVE_ANONYMOUS.  That will handle whatever descendants aParent already has hooked up.

As for the ones that are getting newly added...  This kinda depends on the semantics of the flag.  Should <br> nodes created by the editor in the <div> inside a <textarea> have this flag?  That is, do we want to set the flag on newly-appended kids in ConnectAnonymousTreeDescendants, or in FragmentOrElement::InsertChildAt or so?  I think it would be least-suprising if we set it in InsertChildAt.  But it would mean that any non-anonymous child of native anon content is marked native anon.  Which I think is reasonable...

r=me with the above nits.
Attachment #8831590 - Flags: review?(bzbarsky) → review+
Comment on attachment 8831591 [details] [diff] [review]
Part 4 - Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time, and eliminate explicit style contexts in nsIAnonymousContentCreator::ContentInfo. v1

>+  CSSPseudoElementType GetPseudoElementType() const {

Again, you need to check for the "has properties" bit, right?

>+    if (rv == NS_PROPTABLE_PROP_NOT_THERE) {

It's too bad NotPseudo != 0...

>+  void SetPseudoElementType(CSSPseudoElementType aPseudo) {

Can you please static_assert(sizeof(CSSPseudoElementType) <= sizeof(uintptr_t)) or so?

>+++ b/layout/base/RestyleManager.cpp

I _think_ this part makes sense, but maybe have heycam give this a once-over?  This restyling code has changed a _lot_ since I last really kept up with it.

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+    // If we don't have an explicit style context, that means we need the

What explicit style context?  They got ripped out, no?

I'm really not sure what this comment is saying; please fix it to make sense.

>+++ b/layout/style/nsComputedDOMStyle.cpp

Why is the change to this file needed?

r=me with that explained and the other bits above fixed.
Attachment #8831591 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #10)
> Comment on attachment 8831588 [details] [diff] [review]
> Part 1 - Stop using a node bit for HasExplicitBaseURI. v1
> 
> So did it turn out that the dir attribute flags were not amenable to having
> one of them removed?
> 
>     // Set if node has a dir attribute with a valid value (ltr, rtl, or auto)
>     NodeHasValidDirAttribute,
>     // Set if node has a dir attribute with a fixed value (ltr or rtl, NOT
> auto)
>     NodeHasFixedDir,
>     // Set if the node has dir=auto.
>     NodeHasDirAuto,
> 
> I guess, looking at it just now, one problem is <bdi>, which has
> NodeHasDirAuto when its dir attribute is not set?

Yes.

> 
> But still, looking at nsGenericHTMLElement::AfterSetAttr, there are only
> four possible states corresponding to these three flags:
> 
>   NodeHasValidDirAttribute && NodeHasDirAuto (dir="auto")
>   NodeHasValidDirAttribute && NodeHasFixedDir (dir="rtl" or dir="ltr")
>   NodeHasDirAuto (no dir attr, <bdi> element)
>   no flags (no dir attr, other elements)
> 
> so we should really be able to encode this into two bits, right?  Might
> still be worth doing, even if we nix the base URI thing.

I filed bug 1338723, for the next person who needs a node bit.

> 
> >+    auto maybeURI = static_cast<nsIURI*>(GetProperty(nsGkAtoms::baseURIProperty, &status));
> 
> So there are two problems here:
> 
> 1)  You need to check HasProperties().  No, GetProperty does not normally do
> that, afaict.
> 2)  Why do you care about the status?  The docs for GetProperty are clear
> that null is returned if not present, so you can just do:
> 
>   return static_cast<nsIURI*>(GetProperty(nsGkAtoms::baseURIProperty));
> 
> like we used to.

Fixed.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12)
> Comment on attachment 8831589 [details] [diff] [review]
> Part 2 - Move MAY_HAVE_CLASS to mBoolFlags. v1
> 
> > nsIContent::DoGetClasses() const
> 
> Can you file a followup to move GetClasses/DoGetClasses to Element?  It
> should be pretty straightforward based on some quick searching; feel free to
> assign it to me.

Filed bug 1338725.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> Comment on attachment 8831590 [details] [diff] [review]
> Part 3 - Add a flag to indicate that a node is native anonymous content. v1
> 
> >+   * Returns true of |this| is native anonymous (i.e. created by
> 
> s/of/if/

Fixed.

> 
> I guess the reason this is a flag, not a bool bit (i.e. the reason for the
> MAY_HAVE_CLASS change), is so you can do the bitwise or in
> SetIsNativeAnonymousRoot?

Also that it makes things more convenient from the servo side, where we generally don't deal with bool flags as much. In general, it seemed like this flag belongs with the other related flags on mFlags.

> 
> > ConnectAnonymousTreeDescendants(nsIContent* aParent,
> 
> As the comments say, this function is a convenience.  Anonymous content
> creators do not have to actually create a tree of
> nsIAnonymousContentCreator::ContentInfo.  They can just hook things up
> themselves.  And some do just that.

Ah good point.

> What I think you should do is that inside ConnectAnonymousTreeDescendants,
> before you connect anything, you check whether aParent->IsNativeAnonymous()
> and if so walk over the subtree rooted at aParent and mark them all
> NODE_IS_NATIVE_ANONYMOUS.  That will handle whatever descendants aParent
> already has hooked up.

Well, but only one level deep right? It seems like we basically want to do this as a separate traversal. I'll attach another patch.

> As for the ones that are getting newly added...  This kinda depends on the
> semantics of the flag.  Should <br> nodes created by the editor in the <div>
> inside a <textarea> have this flag?  That is, do we want to set the flag on
> newly-appended kids in ConnectAnonymousTreeDescendants, or in
> FragmentOrElement::InsertChildAt or so?  I think it would be least-suprising
> if we set it in InsertChildAt.  But it would mean that any non-anonymous
> child of native anon content is marked native anon.  Which I think is
> reasonable...

The whole point of this flag is that it's not set on non-native-anonymous children, which is what differentiates it from IS_IN_NATIVE_ANONYMOUS_SUBTREE. So I'm not sure I totally understand the suggestion here.
Attachment #8831590 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> Comment on attachment 8831591 [details] [diff] [review]
> Part 4 - Allow tagging of pseudo-implementing native anonymous content with
> the pseudo type at creation time, and eliminate explicit style contexts in
> nsIAnonymousContentCreator::ContentInfo. v1
> 
> >+  CSSPseudoElementType GetPseudoElementType() const {
> 
> Again, you need to check for the "has properties" bit, right?

Fixed.

> 
> >+    if (rv == NS_PROPTABLE_PROP_NOT_THERE) {
> 
> It's too bad NotPseudo != 0...
> 
> >+  void SetPseudoElementType(CSSPseudoElementType aPseudo) {
> 
> Can you please static_assert(sizeof(CSSPseudoElementType) <=
> sizeof(uintptr_t)) or so?

Fixed.

> 
> >+++ b/layout/base/RestyleManager.cpp
> 
> I _think_ this part makes sense, but maybe have heycam give this a
> once-over?  This restyling code has changed a _lot_ since I last really kept
> up with it.

Will do.

> 
> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> >+    // If we don't have an explicit style context, that means we need the
> 
> What explicit style context?  They got ripped out, no?
> 
> I'm really not sure what this comment is saying; please fix it to make sense.

Fixed.

> 
> >+++ b/layout/style/nsComputedDOMStyle.cpp
> 
> Why is the change to this file needed?

I'm not totally sure. It was part of heycam's original patch, and it seemed like a sensible optimization, except that it's in debug code so I don't know why he added it. I'll remove it and see if try is still happy.
the pseudo type at creation time, and eliminate explicit style contexts in
nsIAnonymousContentCreator::ContentInfo. r=bz

Flagging heycam to take a quick look at the RestyleManager changes per bz's
request.
Attachment #8836281 - Flags: superreview?(cam)
Attachment #8836281 - Flags: review+
> Well, but only one level deep right?

Hmm.  It _really_ depends on the semantics we want here, no?  Why do we do it multiple levels deep if we're hooking up the elements ourselves but only one level deep if the frame already hooked them up?

> which is what differentiates it from IS_IN_NATIVE_ANONYMOUS_SUBTREE.

So I thought the main reason we needed to differentiate was that we had the XBL-bound anon content in <video> and we did NOT want that to have the bit because it broke things.

So in my head this bit was basically set on nodes that are "native anonymous from the point of view of their GetBindingParent".

But it sounds like that's not the mental model you had?  I'd really like to get this part squared away and clearly documented!
Comment on attachment 8836280 [details] [diff] [review]
Part 3 - Add a flag to indicate that a node is native anonymous content. v2

>+  AllChildrenIterator it(aRoot, nsIContent::eAllChildren);

Do we really need this?  Why can't we just do a basic GetNextNode(aRoot) loop?  All the things we want to mark should be actual DOM descendants of aRoot, no?
Summarizing IRC discussion, we will leave part 3 v2 as-is except:
(1) We will document the philosophy behind IS_NATIVE_ANONYMOUS and be explicit that it doesn't necessarily extend into children, including ones created by editor.
(2) We will use GetNextNode instead of the heavyweight AllChildrenIterator.

I'll get back to this next week.
Comment on attachment 8836280 [details] [diff] [review]
Part 3 - Add a flag to indicate that a node is native anonymous content. v2

r=me with the changes we discussed.
Attachment #8836280 - Flags: review?(bzbarsky) → review+
Comment on attachment 8836281 [details] [diff] [review]
Part 4 - Allow tagging of pseudo-implementing native anonymous content with

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

::: layout/base/RestyleManager.cpp
@@ +2886,5 @@
> +      // Each NAC element inherits from the first non-NAC ancestor, so child
> +      // NAC may inherit from our parent instead of us. That means we can't
> +      // cull traversal if our style context didn't change.
> +      LOG_RESTYLE_CONTINUE("native anonymous content");
> +      result = RestyleResult::eContinue;

This looks right, but this is conceptually similar to the check we do for the old style context having a pseudo, so let's move this to the same point (inside ElementRestyler::ComputeRestyleResultFromFrame).
Attachment #8836281 - Flags: superreview?(cam) → superreview+
Blocks: 1331047
I discovered that this is landable by disabling just one servo crashtest. I think that's worth doing, so I'm going to get this in the tree and deal with bug 1331047 separately.
Comment on attachment 8837888 [details]
Bug 1331322 - Stop using a node bit for HasExplicitBaseURI.

https://reviewboard.mozilla.org/r/112884/#review114374
Attachment #8837888 - Flags: review+
Comment on attachment 8837889 [details]
Bug 1331322 - Move MAY_HAVE_CLASS to mBoolFlags.

https://reviewboard.mozilla.org/r/112886/#review114376
Attachment #8837889 - Flags: review+
Comment on attachment 8837890 [details]
Bug 1331322 - Add a flag to indicate that a node is native anonymous content.

https://reviewboard.mozilla.org/r/112888/#review114380
Attachment #8837890 - Flags: review+
Comment on attachment 8837891 [details]
Bug 1331322 - Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time, and eliminate explicit style contexts in nsIAnonymousContentCreator::ContentInfo.

https://reviewboard.mozilla.org/r/112890/#review114382
Attachment #8837891 - Flags: review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02096c5eb029
Stop using a node bit for HasExplicitBaseURI. r=bholley
https://hg.mozilla.org/integration/autoland/rev/841d608704d7
Move MAY_HAVE_CLASS to mBoolFlags. r=bholley
https://hg.mozilla.org/integration/autoland/rev/15877d32de7d
Add a flag to indicate that a node is native anonymous content. r=bholley
https://hg.mozilla.org/integration/autoland/rev/96c6b5a11284
Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time, and eliminate explicit style contexts in nsIAnonymousContentCreator::ContentInfo. r=bholley
Comment on attachment 8837888 [details]
Bug 1331322 - Stop using a node bit for HasExplicitBaseURI.

MozReview is dumb and I had to self-review these to push to autoland. Sorry for the noise.
Attachment #8837888 - Flags: review?(bzbarsky)
Attachment #8837889 - Flags: review?(bzbarsky)
Attachment #8837890 - Flags: review?(bzbarsky)
Attachment #8837891 - Flags: review?(bzbarsky)
Comment on attachment 8838140 [details]
Bug 1331322 - Tweak some fuzzy annotations.

https://reviewboard.mozilla.org/r/113132/#review114592
Attachment #8838140 - Flags: review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e4af18d8ed6
Stop using a node bit for HasExplicitBaseURI. r=bholley
https://hg.mozilla.org/integration/autoland/rev/25419350a537
Move MAY_HAVE_CLASS to mBoolFlags. r=bholley
https://hg.mozilla.org/integration/autoland/rev/dd44bee444c5
Add a flag to indicate that a node is native anonymous content. r=bholley
https://hg.mozilla.org/integration/autoland/rev/4c84e0a380ec
Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time, and eliminate explicit style contexts in nsIAnonymousContentCreator::ContentInfo. r=bholley
https://hg.mozilla.org/integration/autoland/rev/08cec1c29418
Tweak some fuzzy annotations. r=bholley
Attachment #8838140 - Flags: review?(cam) → review+
Depends on: 1343879
You need to log in before you can comment on or make changes to this bug.