Boris Zbarsky [:bzbarsky] Assignee
	Description • 21 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 21 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 21 years ago

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

	David Baron :dbaron:
	Comment 4 • 21 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 21 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 21 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 21 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 21 years ago

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

David, jst, would you review?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 21 years ago

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

	David Baron :dbaron:
	Comment 11 • 21 years ago

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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 12 • 21 years ago

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

	David Baron :dbaron:
	Comment 13 • 21 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 21 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 21 years ago

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

	Johnny Stenback (:jst)
	Comment 16 • 21 years ago

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

	David Baron :dbaron:
	Comment 18 • 21 years ago

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.

	Johnny Stenback (:jst)
	Comment 19 • 21 years ago

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

	David Baron :dbaron:
	Comment 20 • 21 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 21 years ago

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.