Closed Bug 214013 Opened 17 years ago Closed 16 years ago

[FIX]nsAttributeContent is all broken and should go

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Oh, and we can remove HasAttributeContent() and its callers from the frame
manager too.
Attached file testcase
Attached patch Proof-of-concept (obsolete) — Splinter Review
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....
> 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.
Ah, excellent.  I should look into hooking into that.
*** Bug 221463 has been marked as a duplicate of this bug. ***
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.
Attachment #128611 - Attachment is obsolete: true
Comment on attachment 138788 [details] [diff] [review]
Patch that doesn't leave listeners about

David, jst, would you review?
Attachment #138788 - Flags: superreview?(jst)
Attachment #138788 - Flags: review?(dbaron)
Priority: -- → P2
Summary: nsAttributeContent is all broken and should go → [FIX]nsAttributeContent is all broken and should go
Target Milestone: --- → mozilla1.7alpha
Note to self.  Once this lands, a couple of places in the frame constructor can
be consolidated.  See bug 15608 comment 50 
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?
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
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.)
> 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.  ;)
Hmm... I could do the hashtable like thing, I guess... jst, any preference?
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...
Attachment #138788 - Flags: review?(dbaron) → review-
Attachment #138788 - Flags: superreview?(jst)
Attached patch Patch updated to comments (obsolete) — Splinter Review
Attachment #138788 - Attachment is obsolete: true
Attachment #139644 - Flags: superreview?(jst)
Attachment #139644 - Flags: review?(dbaron)
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.
Attachment #139644 - Flags: review?(dbaron) → review+
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
Attachment #139644 - Flags: superreview?(jst) → superreview+
swap requires a pointer of the same type, so you'd need to introduce an extra
variable to do that.
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.