214013 - [FIX]nsAttributeContent is all broken and should go

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

As the coming testcase shows, it doesn't correctly handle dynamic changes
anyway, since HasAttributeDependentStyle() returns false.  So my proposal is:

1)  Use a regular text node for attr() generated content
2)  Handle dynamic updates via mutation listeners on the DOM node that call
    SetText() on the anonymous text node.
3)  cvs remove nsAttributeContent

This should also take us one step closer to being able to select anonymous
content, since nsAttributeContent does not QI to nsIDOMNode and the selection
code needs that.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and we can remove HasAttributeContent() and its callers from the frame
manager too.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proof-of-concept

This is not quite ready to go.	In particular, this does not remove the event
listener when the anonymous content is torn down.

What bothers me is that as far as I can tell, the anonymous content never
really gets its document set to null, so that it won't necessarily know when it
should remove the event listener...

Perhaps we actually need to keep track of the anonymous nodes belonging under a
given content object somewhere and make sure we properly remove them from the
document when the frame is torn down....

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Perhaps we actually need to keep track of the anonymous nodes belonging under a
> given content object somewhere and make sure we properly remove them from the
> document when the frame is torn down....

The pres shell (?) already does that for anonymous content created by
nsIAnonymousContentCreator.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, excellent.  I should look into hooking into that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 221463 has been marked as a duplicate of this bug. ***

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The pres shell's recording and teardown of anonymous content is rather broken,
though. It gets confused when anonymous content from more than one source is
attached to the same real content. See my comments in
nsCSSFrameConstructor::CreateAnonymousFrames.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that doesn't leave listeners about

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138788 [details] [diff] [review]
Patch that doesn't leave listeners about

David, jst, would you review?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note to self.  Once this lands, a couple of places in the frame constructor can
be consolidated.  See bug 15608 comment 50 

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 138788 [details] [diff] [review]
Patch that doesn't leave listeners about


Using mutation events seems a little much considering how much has to be done
for mutation events.  (It seems like it would be nice to register document
observers for specific content nodes.)	Then again, this is used rarely enough
that it's probably OK.

>+nsAttributeTextNode::nsAttrChangeListener::HandleEvent(nsIDOMEvent* aEvent)

Mutation events bubble, so you need to check that the event's target is the
node you care about.

>+  nsAutoString localName;
>+  relatedNode->GetLocalName(localName);
>+  nsCOMPtr<nsIAtom> nameAtom(do_GetAtom(localName));

Perhaps it's better to use nsIAttribute and nsINodeInfo?  (nsIAttribute could
be deCOMtaminated...).

>+  if (nameAtom == mAttrName) {
>+    // Namespace time
>+    nsCOMPtr<nsINameSpaceManager> nsManager;
>+    NS_GetNameSpaceManager(getter_AddRefs(nsManager));
>+    NS_ENSURE_TRUE(nsManager, NS_ERROR_UNEXPECTED);
>+  
>+    nsAutoString namespaceURI;
>+    PRInt32 nameSpaceID;
>+    relatedNode->GetNamespaceURI(namespaceURI);
>+    nsManager->GetNameSpaceID(namespaceURI, &nameSpaceID);

Likewise, maybe the node info would be better?

Comment 12

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

Not sure if it matters here, but currently xul-attributes doesn't implement
nsIAttribute (should be fixed in a not too distant future though). Also see bug
230840

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It might be better to just maintain a hashtable (hashed by content, attribute
name (string or atom?) and namespace) in something that implements
nsIDocumentObserver and register/unregister.  (But remember that the value in
the table has to be a list.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Then again, this is used rarely enough that it's probably OK.

That was my thinking.... document observers are even worse in some ways (you get
notified of _all_ attribute changes).

Will address the other comments; good catch on nsIAttribute.  sicking, I'm not
worried about dynamic changes to attr() content in generated content in XUL,
really...  especially not if you'll fix it once you finish your rewrite.  ;)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm... I could do the hashtable like thing, I guess... jst, any preference?

Comment 16

[Johnny Stenback (:jst)]

Not really. While I don't by any means love our DOM mutation event code, it
seems like any other alternative will be even more code than what's needed here
for the mutation listener. So I don't know what I'd prefer. Ideally we'd have a
better change notification scheme (on which our DOM mutation event system could
be build too, possibly), but we don't have that at this point...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 139644 [details] [diff] [review]
Patch updated to comments

I'm not crazy about implementations of virtual functions being inline, but
r=dbaron.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 139644 [details] [diff] [review]
Patch updated to comments

+  nsCOMPtr<nsINodeInfo> attrNodeInfo;
+  attr->GetNodeInfo(getter_AddRefs(attrNodeInfo));
+  NS_ASSERTION(attrNodeInfo, "nsIAttribute must have nodeinfo");
+
+  if (attrNodeInfo->Equals(mAttrName, mNameSpaceID)) {

That won't compile any more, nsIAttribute was deCOMtaminated, now you can just
do:

+  if (attr->NodeInfo()->Equals(mAttrName, mNameSpaceID)) {

- In NS_NewAttributeContent():

+  return CallQueryInterface(textNode.get(), aResult);

How about something like:

+  textNode.swap(*aResult);
+
+  return NS_OK;

in stead?

sr=jst

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

swap requires a pointer of the same type, so you'd need to introduce an extra
variable to do that.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that's merged and with comments fixed

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in to trunk.  Wins somewhere around 1k of mZ (the vtable for the
textnode thing is _huge_).

This also seems to coincide with a Tp rise on btek, but I don't see why it
would.... and the other tboxes are showing nothing.