285597 - Implement Get/SetProperty for nsIAttribute

Status: RESOLVED FIXED

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

Description

[Allan Beaufour]

We should let nsIAttribute implement Get/SetProperty(), etc. just as nsIContent.

Comment 1

[Allan Beaufour]

Attachment: Patch

Here's a go at it. Doesn't seem to work for me though :(

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> Created an attachment (id=177015) [edit]
> Patch
> 
> Here's a go at it. Doesn't seem to work for me though :(

Had a chat with peterv about it:
1. the functions in nsIAttribute should abstract.
2. nsDOMAttributes are not unique.... we need to cache them in nsDOMAttributeMap

or: Big monster ate my little bug :)

Comment 3

[Allan Beaufour]

Attachment: Updated patch

Bug 235512 is ready, so it's time to get this one done too.

Comment 4

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

Comment on attachment 183822 [details] [diff] [review]
Updated patch

>+  nsISupports *thisSupports = NS_STATIC_CAST(nsIDOMAttr*, this);
>+#ifdef DEBUG
>+  printf("GetProperty(this=%p, aPropertyName = %p)\n",
>+         (void*) thisSupports,
>+         (void*) aPropertyName);
>+#endif
>+  return doc->PropertyTable()->GetProperty(thisSupports, aPropertyName,
>+                                           aStatus);

Why all this casting to nsISupports? Just use |this| as first argument
everywhere.

>+  nsIDocument *GetOwnerDoc() const
>+  {
>+    return mContent ? mContent->GetOwnerDoc() : mNodeInfo->GetDocument();
>+  }

You making this a virtual function once mContent dies? Actually, ideally we
should update the mNodeInfo member of all attrnodes when an element is moved
between documents. Then this could just use mNodeInfo->GetDocument(). But that
could be done in another bug.

r=me with the first fixed.

Comment 5

[Allan Beaufour]

Attachment: w/sicking's comment

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> Created an attachment (id=184913) [edit]
> w/sicking's comment

And |doc->PropertyTable()->DeleteAllPropertiesFor(this)| in the dtor btw.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 184913 [details] [diff] [review]
w/sicking's comment

>Index: content/base/public/nsIAttribute.h
>===================================================================

>+  /* Methods for manipulating content node properties.  For documentation on

Make that 

  /**
   * Methods...

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 184913 [details] [diff] [review]
w/sicking's comment

a=mkaply

Comment 9

[Allan Beaufour]

Attachment: Updated to current CVS

Updated to incorporate changes that came from bug 235512.

Comment 10

[Allan Beaufour]

Checked in.