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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Splitting this out of bug 1330843.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Allow tagging of pseudo-implementing native anonymous content with the pseudo type at creation time. WIP

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.

Comment 3

[Bobby Holley (:bholley)]

Note to self: I was concerned that this would regress the issue described in bug 939248, but it doesn't seem to.

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a7e028943e93dfe446b492d53eb3f4e8f4dd9c1

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d196e60166a7faa73c6a2d785a4f2a3e531b7a54

This is green.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop using a node bit for HasExplicitBaseURI. v1

This is only called during URI resolution, and the proptable bit should make
this relatively fast.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move MAY_HAVE_CLASS to mBoolFlags. v1

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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a flag to indicate that a node is native anonymous content. v1

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Boris Zbarsky [: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....

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Bobby Holley (:bholley)]

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

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a flag to indicate that a node is native anonymous content. v2

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[Bobby Holley (:bholley)]

Attachment: 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. r=bz

Flagging heycam to take a quick look at the RestyleManager changes per bz's
request.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[Bobby Holley (:bholley)]

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 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Cameron McCormack (:heycam)]

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

Comment 26

[Bobby Holley (:bholley)]

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 27

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2d7ce6a11d36e45ba3d0ecb2214085ba8cfa5ea

Comment 28

[Bobby Holley (:bholley)]

Attachment: Bug 1331322 - Stop using a node bit for HasExplicitBaseURI.

This is only called during URI resolution, and the proptable bit should make
this relatively fast.

Review commit: https://reviewboard.mozilla.org/r/112884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112884/

Comment 29

[Bobby Holley (:bholley)]

Attachment: Bug 1331322 - Move MAY_HAVE_CLASS to mBoolFlags.

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.

Review commit: https://reviewboard.mozilla.org/r/112886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112886/

Comment 30

[Bobby Holley (:bholley)]

Attachment: Bug 1331322 - Add a flag to indicate that a node is native anonymous content.

Review commit: https://reviewboard.mozilla.org/r/112888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112888/

Comment 31

[Bobby Holley (:bholley)]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/112890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112890/

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8837888 [details]
Bug 1331322 - Stop using a node bit for HasExplicitBaseURI.

https://reviewboard.mozilla.org/r/112884/#review114374

Comment 33

[Bobby Holley (:bholley)]

Comment on attachment 8837889 [details]
Bug 1331322 - Move MAY_HAVE_CLASS to mBoolFlags.

https://reviewboard.mozilla.org/r/112886/#review114376

Comment 34

[Bobby Holley (:bholley)]

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

Comment 35

[Bobby Holley (:bholley)]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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 37

[Bobby Holley (: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.

Comment 38

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/8c8b54b13be7ec12cb8e104b772162a80b524497 for a pair of Android reftest failures (opt and debug both), https://treeherder.mozilla.org/logviewer.html#?job_id=77788008&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=77788052&repo=autoland

Comment 39

[Bobby Holley (:bholley)]

Attachment: Bug 1331322 - Tweak some fuzzy annotations.

Review commit: https://reviewboard.mozilla.org/r/113132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113132/

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 8838140 [details]
Bug 1331322 - Tweak some fuzzy annotations.

https://reviewboard.mozilla.org/r/113132/#review114592

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e4af18d8ed6
https://hg.mozilla.org/mozilla-central/rev/25419350a537
https://hg.mozilla.org/mozilla-central/rev/dd44bee444c5
https://hg.mozilla.org/mozilla-central/rev/4c84e0a380ec
https://hg.mozilla.org/mozilla-central/rev/08cec1c29418