Closed
Bug 351823
Opened 18 years ago
Closed 18 years ago
Consider copying more properties when changing ownerDocument
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: peterv)
References
Details
Attachments
(1 file, 1 obsolete file)
36.22 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
Right now we only copy userdata. Should we perhaps allow nsGenericElement subclasses to answer for each property whether to copy or not? Should HTML copy baseHref and baseTarget, for example? Should XUL copy popup listeners? That sort of thing.
I think it should be done by sticking some sort of handler on the property. This is because a big usecase for properties is the ability to stick info on nodes without the nodes having to "support" that property. All we'd need to do is change the |NSPropertyDtorFunc aDtor| argument to point to a struct with a couple of function pointers.
As far as baseuri etc goes, i don't really feel strongly either way. In one sense I want to say that if you're doing DOM type things you really should make sure that the DOM is representative of your doc (i.e. that it shouldn't rely on magic hidden properties)
Reporter | ||
Comment 3•18 years ago
|
||
Sure, I'm happy with a callback fun on the prop itself. I agree that's a better idea than having the element class deal. And I have no strong feelings about the baseURI; personally I think we shouldn't move it, but I was just wondering.
Assignee | ||
Comment 4•18 years ago
|
||
So do we want a callback or just a simple flag? I did this with a flag and then AdoptNode can call DeleteOrTransferPropertiesFor which will either delete or transfer the properties (depending on whether the flag was set). Would we need to do fixup on the properties? My current use cases (DOM userdata and XUL popup listeners) don't.
Will UserData be able to use this very much anyway? Doesn't it in most cases call into the userdata handler?
Assignee | ||
Comment 6•18 years ago
|
||
(In reply to comment #5) > Will UserData be able to use this very much anyway? Right now UserData are the only properties that we copy. If we generalize the mechanism it'd be silly not to do it for UserData too. > Doesn't it in most cases call into the userdata handler? That's unrelated, no?
My question is, how much will userdata really be able to use this mechanism if we add it? Does it ever unconditionally copy properties into a clone rather than call into the userdata handler?
oh, wait, nm, for adoptions i guess we'd use it.
Reporter | ||
Comment 9•18 years ago
|
||
> So do we want a callback or just a simple flag?
You mean a flag passed to AddProperty or something? Sure, that would work too.
Blocks: 352149
Assignee | ||
Comment 10•18 years ago
|
||
The code added in the BindToTree implementations will go away when bug 47903 is fixed.
Assignee: general → peterv
Status: NEW → ASSIGNED
Attachment #238425 -
Flags: superreview?(bzbarsky)
Attachment #238425 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 11•18 years ago
|
||
Comment on attachment 238425 [details] [diff] [review] v1 >Index: content/base/public/nsContentUtils.h >Index: content/base/public/nsINode.h >+ PRBool aTransfer = PR_FALSE) Document what this means? For both functions? >Index: content/base/src/nsDocument.cpp >+ if (NS_FAILED(rv)) { ... >+ return rv; I think we want to press on and either keep doing the TransferOrDeleteAllPropertiesFor for the remaining nodes or just switch to deleting properties. In any case, we don't want properties left in the old doc for nodes that are now in the new doc. >Index: content/base/src/nsPropertyTable.cpp >+ PropertyList(PRUint16 aCategory, Shouldn't we also change the type of aCategory on nsINode and nsIFrame and such, then? It's PRUint32 over there... Alternately, we need code in SetProperty that throws if a too-big category is passed in. Or something. >+ return mCategory == aCategory && mName == aPropertyName; OK. So mCategory is just an integer, not a bitfield, right? >+ void* mDtorData; // pointer to pass to dtor Do we ever actually set this anywhere? That is, are the SetProperty variants that would use this ever called? Given that it has to be shared by all properties with this name regardless of what object they're set on, I somehow doubt it, but maybe people are passing in static data? We should probably remove it in a followup bug if we can... >+ PRPackedBool mTransfer; // whether to transfer "in TransferOrDeleteAllPropertiesFor", right? >+nsPropertyTable::TransferOrDeleteAllPropertiesFor(nsPropertyOwner aObject, >+ nsPropertyTable *aTable) Maybe s/aTable/aTargetTable/? Or aOtherTable? Or something like that? >Index: content/base/src/nsPropertyTable.h >+#define DOM_USER_DATA (1 << 0) >+#define DOM_USER_DATA_HANDLER (1 << 1) Given that they're integers, not bits, how about we just write "1" and "2"? >+ * table changes too). If|aTransfer| is PR_FALSE the property will Insert space in "If|". Same in the other comment here. >@@ -219,11 +233,21 @@ class nsPropertyTable >+ * |aTransfer| argument as PR_TRUE to |aTable|. Delete the other properties s/Delete/Deletes/ r+sr=bzbarsky with the nits picked.
Attachment #238425 -
Flags: superreview?(bzbarsky)
Attachment #238425 -
Flags: superreview+
Attachment #238425 -
Flags: review?(bzbarsky)
Attachment #238425 -
Flags: review+
Assignee | ||
Comment 12•18 years ago
|
||
(In reply to comment #11) > >+ void* mDtorData; // pointer to pass to dtor > > Do we ever actually set this anywhere? That is, are the SetProperty variants > that would use this ever called? Given that it has to be shared by all > properties with this name regardless of what object they're set on, I somehow > doubt it, but maybe people are passing in static data? We should probably > remove it in a followup bug if we can... AFAIK http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#5289 is the only place that uses this, it passes in the prescontext (for the property table owned by that prescontext).
Reporter | ||
Comment 13•18 years ago
|
||
Ah, ok. If it's used, ignore what I said about it. I was trying to see whether we could shrink the struct some, but I guess memory is not that huge an issue here. Speaking of which... we could make mCategory a PRUint32 and make mTransfer a 1-bit tag on mDtorData if we want.... Don't really care either way; could also make categories PRUint16 throughout.
Assignee | ||
Comment 14•18 years ago
|
||
Attachment #238425 -
Attachment is obsolete: true
Attachment #238842 -
Flags: superreview+
Attachment #238842 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: in-testsuite?
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
•