Don't use nsIDOMHTMLMapElement in nsHTMLDocument::GetImageMap

RESOLVED FIXED in mozilla5

Status

()

defect
--
minor
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Updated

9 years ago
Depends on: 587469
Assignee

Comment 1

9 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #466143 - Flags: review?(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?
Attachment #466143 - Flags: review?(peterv) → review-
(In reply to comment #2)
> make GetImageMap return a nsIContent

Or Element.
Assignee

Comment 4

9 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #466143 - Attachment is obsolete: true
Attachment #468129 - Flags: review?(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.
Attachment #468129 - Flags: review?(peterv) → review+
Assignee

Comment 6

9 years ago
Posted patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #468129 - Attachment is obsolete: true
Please remove #include "nsIDOMHTMLMapElement.h" from nsHTMLDocument.h also.
Assignee

Comment 8

9 years ago
Posted patch Patch v4 (obsolete) — Splinter Review
Done, thanks.
Attachment #471077 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Whiteboard: [needs landing]
Assignee

Updated

9 years ago
Depends on: post2.0
Assignee

Updated

8 years ago
Blocks: 557768
Assignee

Comment 9

8 years ago
Attachment #483422 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
The patch no longer applies correctly. Could you update it?
Keywords: checkin-needed
My bad, I didn't apply the deps before this patch...
Keywords: checkin-needed

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9df21e557f71
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.