Consider copying more properties when changing ownerDocument

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bz, Assigned: peterv)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

12 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

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

12 years ago
Created attachment 238425 [details] [diff] [review]
v1

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

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

12 years ago
Created attachment 238842 [details] [diff] [review]
Patch that was checked in
Attachment #238425 - Attachment is obsolete: true
Attachment #238842 - Flags: superreview+
Attachment #238842 - Flags: review+
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.