Closed Bug 253888 Opened 20 years ago Closed 18 years ago

Support for content node properties

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

Details

Attachments

(2 files, 2 obsolete files)

For some forms work I'm doing currently, it would be really nice to be able to
attach properties to content nodes the way you can for frames.  To do this, I
think the frame property code should be made generic, and then have documents
own a property table for content nodes the same way a frame manager owns the
frame properties now.  This would also be a good underpinning for supporting the
DOM3 userData methods (user data should live in a separate table, in my opinion,
to avoid name collisions).
Attached patch patch (obsolete) — Splinter Review
This implements a generic nsPropertyTable class and reimplements frame
properties using that.	In addition, GenericElement and XULElement now support
setting properties.  (other types of nodes currently _don't_ because we
couldn't easily identify a bit to set to indicate that properties are present
and wanted to avoid a hashtable lookup for every node destruction).
Attachment #154901 - Flags: superreview?(dbaron)
Attachment #154901 - Flags: review?(jst)
So... Don't box objects do this already, in effect?
Comment on attachment 154901 [details] [diff] [review]
patch

- In nsGenericElement::SetProperty():

+  nsIDocument *doc = GetDocument();
+  if (!doc)
+    return nsnull;

This method returns nsresult, return an error here, not nsnull.

- In content/shared/public/nsPropertyTable.h:

+ * Callback type for property destructors.  |aElement| is the element
+ * the property is being removed for, |aPropertyName| is the property

This is used for non-elements as well, like frames, and maybe non-element DOM
nodes at some point. s/element/object/g in this whole file?

- In nsPropertyTable::PropertyList::RemovePropertyFor()

    mDtorFunc(NS_CONST_CAST(void*, aElement), mName,
	      entry->value, mDtorData);

Tab used for indentation, uses spaces.

nsresult
nsXULElement::SetProperty(nsIAtom	      *aPropertyName,
			   void 	      *aValue,
			   NSPropertyDtorFunc  aDtor)
...

Fix next-line argument indentation.

...
    nsIDocument *doc = GetDocument();
    if (!doc)
	return nsnull;

This returns nsresult, return an error here, not nsnull.

r=jst
Attachment #154901 - Flags: review?(jst) → review+
Comment on attachment 154901 [details] [diff] [review]
patch

>@@ -781,12 +748,6 @@ nsFrameManager::NotifyDestroyingFrame(ns
>   // Dequeue and destroy and posted events for this frame
>   DequeuePostedEventFor(aFrame);
> 
>-  // Remove all properties associated with the frame
>-  nsCOMPtr<nsPresContext> presContext;
>-  mPresShell->GetPresContext(getter_AddRefs(presContext));
>-
>-  RemoveAllPropertiesFor(presContext, aFrame);

This could cause some significant leaks given some types of DOM animation.  Is
there something that replaces this that I'm not seeing?
Comment on attachment 154901 [details] [diff] [review]
patch

>Index: layout/html/table/src/nsTableFrame.cpp
>Index: layout/mathml/base/src/nsMathMLmtableFrame.cpp

In both these files, remove the now unused |frameManager| variables.

>Index: layout/xul/base/src/nsBox.cpp
>@@ -570,8 +570,7 @@ nsBox::SetBounds(nsBoxLayoutState& aStat
>     // it if necessary.
>     if (aRemoveOverflowArea && (frame->GetStateBits() & NS_FRAME_OUTSIDE_CHILDREN)) {
>       // remove the previously stored overflow area 
>-      frame->GetPresContext()->FrameManager()->
>-        RemoveFrameProperty(frame, nsLayoutAtoms::overflowAreaProperty);
>+      frame->RemoveProperty(nsLayoutAtoms::overflowAreaProperty);
>       frame->RemoveStateBits(NS_FRAME_OUTSIDE_CHILDREN);
>     }
> 

This and any other changes like it introduce a leak.  (I didn't notice this
distinction until most of the way through the patch.) 
nsFrameManager::RemoveFrameProperty calls the dtor, but nsFrame::RemoveProperty
doesn't -- it calls GetFrameProperty with the REMOVE_PROP flag.

The fact that you're getting it wrong suggests that this API is broken and
shouldn't be spread further without being fixed.  I'd suggest:
 1. renaming all the RemoveProperty calls that call the dtor to DeleteProperty.
 2. renaming all the RemoveProperty calls that don't call the dtor to
GetAndRemoveProperty.
but I'm open to other ideas.
Attachment #154901 - Flags: superreview?(dbaron) → superreview-
oh, and:
 3. Providing DeleteProperty on nsIFrame/nsIContent in addition to
GetAndRemoveProperty and using it to fix the leaks
(In reply to comment #5)
> The fact that you're getting it wrong suggests that this API is broken and

I should add: that you got it wrong and that I missed it a whole bunch of times
while reviewing the patch and didn't notice until the very last change in the
patch, when I was about to mark superreview+.
> shouldn't be spread further without being fixed.  I'd suggest:
>  1. renaming all the RemoveProperty calls that call the dtor to DeleteProperty.

This one sounds fine.

>  2. renaming all the RemoveProperty calls that don't call the dtor to
> GetAndRemoveProperty.

Hm, I don't think calling it GetAndRemoveProperty helps explain that the
ownership has been transferred to you any better than just RemoveProperty
(though it does imply it, since it wouldn't make sense to return a deleted
property).  Remove has an unfortunate connotation that things have been cleaned
up for you.  I'm thinking maybe PullProperty, or just UnsetProperty?
Another idea would be to have a boolean parameter to RemoveProperty() that would
say whether the property dtor should run.  If true, RemoveProperty would return
null, otherwise, it would return the property value.
Attached patch patch #2 (obsolete) — Splinter Review
Fixed the leaks by splitting RemoveProperty into DeleteProperty and
UnsetProperty.
Attachment #154901 - Attachment is obsolete: true
Attachment #155751 - Flags: review?(dbaron)
Comment on attachment 155751 [details] [diff] [review]
patch #2

Looks good.  Two comments:
 * You should probably rename RemovePropertyFor to DeletePropertyFor.  (It's on
nsPropertyTable::PropertyList.)

 * I think I prefer DeleteProperty in nsBlockFrame::ClearLineCursor, although
it really doesn't matter.

With that, sr=dbaron.
Attachment #155751 - Flags: review?(dbaron) → review+
This patch caused crashes so I backed it out.  The crash was in
nsCSSFrameConstructor::ConstructHTMLFrame, where it was setting a property on a
frame that has a null style context.  Will post a new patch after discussing
this with dbaron/bz.
The following will also unravel other crashes. The |res| (meant for HasProperty
here) is not computed.

===========
@@ -9826,12 +9817,9 @@ nsCSSFrameConstructor::ProcessRestyledFr
     if (frame) {
       nsresult res;
 
-      void* dummy =
-        frameManager->GetFrameProperty(frame,
-                                       nsLayoutAtoms::changeListProperty, 0,
-                                       &res);
+      void* dummy = frame->GetProperty(nsLayoutAtoms::changeListProperty);
 
-      if (NS_IFRAME_MGR_PROP_NOT_THERE == res)
+      if (NS_PROPTABLE_PROP_NOT_THERE == res)
         continue;
     }
Attached patch more fixesSplinter Review
- Delete all properties for the deleted frame in
PresShell::NotifyDestroyingFrame
- When there is a shorter way available to get to the property table than
nsIFrame::Get/SetProperty, use it (doing it from the frame requires 3 pointer
derefernces; if a PresContext is available there should not need to be any)

I'd already fixed the return value problem in nsCSSFrameConstructor before
checking in.
Attachment #155751 - Attachment is obsolete: true
Comment on attachment 156840 [details] [diff] [review]
more fixes

I ran this through the page loader and couldn't measure any difference from an
unmodified build.
Attachment #156840 - Flags: review?(dbaron)
Comment on attachment 156840 [details] [diff] [review]
more fixes

Other than the difference in nsBlockFrame::SetOverflowLines between this patch
and the last patch (to which I don't see a point), r=dbaron.
Attachment #156840 - Flags: review?(dbaron) → review+
@@ -4333,10 +4333,12 @@ nsBlockFrame::SetOverflowLines(nsLineLis
   NS_ASSERTION(aOverflowLines, "null lines");
   NS_ASSERTION(!aOverflowLines->empty(), "empty lines");
 
-  nsresult rv = SetProperty(nsLayoutAtoms::overflowLinesProperty,
-                            aOverflowLines, DestroyOverflowLines);
+  nsPresContext *presContext = GetPresContext();
+  nsresult rv = presContext->PropertyTable()->
+    SetProperty(this, nsLayoutAtoms::overflowLinesProperty, aOverflowLines,
+                DestroyOverflowLines, presContext);
   // Verify that we didn't overwrite an existing overflow list
-  NS_ASSERTION(rv != NS_IFRAME_MGR_PROP_OVERWRITTEN, "existing overflow list");
+  NS_ASSERTION(rv != NS_PROPTABLE_PROP_OVERWRITTEN, "existing overflow list");
   return rv;
 }
 
The previous version of this code had to get the prescontext pointer twice (once
as a parameter to SetProperty, once to get at the property table from within
SetProperty).  This change makes it so it only needs to do that once.
btek says it went up 9ms (825 -> 834) on average after this checkin:

http://axolotl.mozilla.org/graph/query.cgi?testname=pageload&tbox=btek&autoscale=1&days=7&avg=1&showpoint=2004:08:24:12:56:55,822

Acceptable to support this, I'd say.
Erh, I suck. Though it did go up, it was only by 5ms, from 822ms to 827ms. And
it looks like it might actually have been just 3 ms, with some other checkin
responsible for the other 2ms. So nevermind :-)
This is a trivial follow-on patch to implement support for properties on the
document itself.
Attachment #156996 - Flags: review?(jst)
Shouldn't we be able to use properties on elements that are not inserted in a
document? I would think so and the trivial fix would be to replace GetDocument
in those functions with GetOwnerDoc.
Comment on attachment 156996 [details] [diff] [review]
support properties for documents, as well

r+sr=jst
Attachment #156996 - Flags: superreview+
Attachment #156996 - Flags: review?(jst)
Attachment #156996 - Flags: review+
This seems to have caused bug 275574 (because the property code just drops the
aDtorData on the floor).
should have been marked fixed when checked in
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: