Last Comment Bug 847195 - Make NamedNodeMap only deal with Attrs
: Make NamedNodeMap only deal with Attrs
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on: 858344 849612
Blocks: AttrExodus
  Show dependency treegraph
 
Reported: 2013-03-03 10:35 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-07-01 16:43 PDT (History)
5 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (63.57 KB, patch)
2013-03-03 10:35 PST, :Ms2ger (⌚ UTC+1/+2)
khuey: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2013-03-03 10:35:25 PST
Created attachment 720439 [details] [diff] [review]
Patch v1

Once Attr no longer inherits from Node, we won't we able to QI back and forth like the callers here do.
Comment 1 Mounir Lamouri (:mounir) 2013-03-05 04:26:01 PST
Comment on attachment 720439 [details] [diff] [review]
Patch v1

Don't have time nor motivation for that.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-03-08 14:57:34 PST
Comment on attachment 720439 [details] [diff] [review]
Patch v1

Review of attachment 720439 [details] [diff] [review]:
-----------------------------------------------------------------

Please mark nsIDOMAttr as builtinclass.

::: content/base/src/nsDOMAttributeMap.cpp
@@ +200,3 @@
>  {
>    ErrorResult rv;
> +  *aReturn = SetNamedItemInternal(aAttr, false, rv).get();

Wow this code is ugly

::: dom/interfaces/core/Makefile.in
@@ +23,5 @@
>  	nsIDOMDocument.idl			\
>  	nsIDOMDocumentFragment.idl		\
>  	nsIDOMDocumentType.idl			\
>  	nsIDOMElement.idl			\
> +	nsIDOMMozNamedAttrMap.idl			\

Line up the \ please.

::: editor/libeditor/base/nsEditor.cpp
@@ +2241,3 @@
>      // always remove item number 0 (first item in list)
> +    if (NS_SUCCEEDED(destAttributes->Item(0, getter_AddRefs(attr))) && attr) {
> +      nsAutoString str;

Don't use nsAutoString here.  The stack buffer is wasted.  nsString is just fine.  (Yes I realize this was here before).

@@ +2258,3 @@
>    {
> +    if (NS_SUCCEEDED(sourceAttributes->Item(i, getter_AddRefs(attr))) && attr) {
> +      nsAutoString sourceAttrName;

Same here.

@@ +2258,5 @@
>    {
> +    if (NS_SUCCEEDED(sourceAttributes->Item(i, getter_AddRefs(attr))) && attr) {
> +      nsAutoString sourceAttrName;
> +      if (NS_SUCCEEDED(attr->GetName(sourceAttrName))) {
> +        nsAutoString sourceAttrValue;

And here.

@@ +2278,1 @@
>  #endif

You can remove #ifdef DEBUG_personiveneverheardof code.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2013-03-10 04:15:09 PDT
https://hg.mozilla.org/mozilla-central/rev/b1a08130fae6
Comment 5 Masatoshi Kimura [:emk] 2013-07-01 16:11:12 PDT
(In reply to Kohei Yoshino [:kohei] from comment #4)
> https://developer.mozilla.org/en-US/docs/Web/API/NamedNodeMap
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

The interface has been renamed to MozNamedAttrMap, not MozNamedNodeMap. (Because this interface is only used for attributes.)
Comment 6 Kohei Yoshino [:kohei] 2013-07-01 16:43:52 PDT
(In reply to Masatoshi Kimura [:emk] from comment #5)
> The interface has been renamed to MozNamedAttrMap, not MozNamedNodeMap.
> (Because this interface is only used for attributes.)

Thank you for pointing it out. I just fixed the documents.

Note You need to log in before you can comment on or make changes to this bug.