253888 - Support for content node properties

Status: RESOLVED FIXED

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

Description

[Brian Ryner (not reading)]

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

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... Don't box objects do this already, in effect?

Comment 3

[Johnny Stenback (:jst)]

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

Comment 4

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

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 5

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

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.

Comment 6

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

oh, and:
 3. Providing DeleteProperty on nsIFrame/nsIContent in addition to
GetAndRemoveProperty and using it to fix the leaks

Comment 7

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

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

Comment 8

[Brian Ryner (not reading)]

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

Comment 9

[Brian Ryner (not reading)]

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.

Comment 10

[Brian Ryner (not reading)]

Attachment: patch #2

Fixed the leaks by splitting RemoveProperty into DeleteProperty and
UnsetProperty.

Comment 11

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

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.

Comment 12

[Brian Ryner (not reading)]

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.

Comment 13

[rbs]

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;
     }

Comment 14

[Brian Ryner (not reading)]

Attachment: more fixes

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

Comment 15

[Brian Ryner (not reading)]

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.

Comment 16

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

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.

Comment 17

[Brian Ryner (not reading)]

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

Comment 18

[jag (Peter Annema)]

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.

Comment 19

[jag (Peter Annema)]

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

Comment 20

[Brian Ryner (not reading)]

Attachment: support properties for documents, as well

This is a trivial follow-on patch to implement support for properties on the
document itself.

Comment 21

[Peter Van der Beken [:peterv]]

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 22

[Johnny Stenback (:jst)]

Comment on attachment 156996 [details] [diff] [review]
support properties for documents, as well

r+sr=jst

Comment 23

[Boris Zbarsky [:bzbarsky]]

This seems to have caused bug 275574 (because the property code just drops the
aDtorData on the floor).

Comment 24

[Brian Ryner (not reading)]

should have been marked fixed when checked in