Closed
Bug 253888
Opened 20 years ago
Closed 18 years ago
Support for content node properties
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: bryner)
Details
Attachments
(2 files, 2 obsolete files)
92.00 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
3.36 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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•20 years ago
|
Attachment #154901 -
Flags: superreview?(dbaron)
Attachment #154901 -
Flags: review?(jst)
Comment 2•20 years ago
|
||
So... Don't box objects do this already, in effect?
Comment 3•20 years ago
|
||
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•20 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•20 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•20 years ago
|
||
Fixed the leaks by splitting RemoveProperty into DeleteProperty and UnsetProperty.
Attachment #154901 -
Attachment is obsolete: true
Assignee | ||
Updated•20 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•20 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•20 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•20 years ago
|
||
- 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•20 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•20 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•20 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•20 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•20 years ago
|
||
This is a trivial follow-on patch to implement support for properties on the document itself.
Assignee | ||
Updated•20 years ago
|
Attachment #156996 -
Flags: review?(jst)
Comment 21•20 years ago
|
||
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•20 years ago
|
||
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+
Comment 23•20 years ago
|
||
This seems to have caused bug 275574 (because the property code just drops the aDtorData on the floor).
Assignee | ||
Comment 24•18 years ago
|
||
should have been marked fixed when checked in
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•