351823 - Consider copying more properties when changing ownerDocument

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Peter Van der Beken [:peterv]]

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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Will UserData be able to use this very much anyway? Doesn't it in most cases call into the userdata handler?

Comment 6

[Peter Van der Beken [:peterv]]

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

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, wait, nm, for adoptions i guess we'd use it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1

The code added in the BindToTree implementations will go away when bug 47903 is fixed.

Comment 11

[Boris Zbarsky [: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.

Comment 12

[Peter Van der Beken [:peterv]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in