581644 - Don't use nsIDOMHTMLMapElement in nsHTMLDocument::GetImageMap

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

See bug 564001 comment 7.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 466143 [details] [diff] [review]
Patch v1

I think you should also make GetImageMap return a nsIContent, and let callers do the QI if needed. Also, why are you removing the error checking for the GetAttr calls?

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> make GetImageMap return a nsIContent

Or Element.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Now returns Element. In the GetAttr call, I'm not actually changing behavior here, as GetId and GetName just return the empty string if the attribute is absent, like GetAttr.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 468129 [details] [diff] [review]
Patch v2

>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp

>+    map->GetAttr(kNameSpaceID_None, nsGkAtoms::id, name);
>+    PRBool match = name.Equals(aMapName);

I'd just use AttrValueIs instead.

>+
>     if (!match) {
>-      rv = map->GetName(name);
>-      NS_ENSURE_SUCCESS(rv, nsnull);
>-
>+      map->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name);
>       match = name.Equals(aMapName, nsCaseInsensitiveStringComparator());

Same here.

>-#ifndef nsIHTMLDocument_h___
>-#define nsIHTMLDocument_h___
>+
>+#ifndef nsIHTMLDocument_h
>+#define nsIHTMLDocument_h

No need for this change, and afaict it makes these inconsistent with the rest of the tree. Please revert.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Now with AttrValueIs... This function is actually starting to look sane now.

> >-#ifndef nsIHTMLDocument_h___
> >-#define nsIHTMLDocument_h___
> >+
> >+#ifndef nsIHTMLDocument_h
> >+#define nsIHTMLDocument_h
> 
> No need for this change, and afaict it makes these inconsistent with the rest
> of the tree. Please revert.

As was mentioned in bug 514661 comment 25, all names with two consecutive
underscores are reserved to the implementation, and the rest of the tree uses roughly any style you can imagine. Your call, though; I'll revert if you prefer.

Comment 7

[Mats Palmgren (inactive)]

Please remove #include "nsIDOMHTMLMapElement.h" from nsHTMLDocument.h also.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v4

Done, thanks.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 10

[Mounir Lamouri (:mounir)]

The patch no longer applies correctly. Could you update it?

Comment 11

[Mounir Lamouri (:mounir)]

My bad, I didn't apply the deps before this patch...

Comment 12

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/9df21e557f71

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9df21e557f71