If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support for content node properties

RESOLVED FIXED

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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).
(Assignee)

Comment 1

13 years ago
Created attachment 154901 [details] [diff] [review]
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).
(Assignee)

Updated

13 years ago
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+.
(Assignee)

Comment 8

13 years ago
> 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?
(Assignee)

Comment 9

13 years ago
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.
(Assignee)

Comment 10

13 years ago
Created attachment 155751 [details] [diff] [review]
patch #2

Fixed the leaks by splitting RemoveProperty into DeleteProperty and
UnsetProperty.
Attachment #154901 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
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+
(Assignee)

Comment 12

13 years ago
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

13 years ago
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;
     }
(Assignee)

Comment 14

13 years ago
Created attachment 156840 [details] [diff] [review]
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.
Attachment #155751 - Attachment is obsolete: true
(Assignee)

Comment 15

13 years ago
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+
(Assignee)

Comment 17

13 years ago
@@ -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

13 years ago
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

13 years ago
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 :-)
(Assignee)

Comment 20

13 years ago
Created attachment 156996 [details] [diff] [review]
support properties for documents, as well

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

Updated

13 years ago
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).
(Assignee)

Comment 24

12 years ago
should have been marked fixed when checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.