296237 - remove assert in getElementById, use the console service to provide feedback

Status: RESOLVED FIXED

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

Description

[Mike Connor [:mconnor]]

document.getElementById("") asserts currently, and going into a "asserts crash"
future, this would seem to be a Bad Thing.  Instead of asserting, or a warning
that's only of value to debug builds, we should output a message to the console
service.  Patch upcoming.

Comment 1

[Mike Connor [:mconnor]]

Attachment: remove asserts, add console warning

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 185033 [details] [diff] [review]
remove asserts, add console warning

I guess it'd be nice to split this code into a global function (i.e. PRBool
CheckGetElementByIdArg(const nsAString& aId)) in some file, say nsDocument.cpp
and just call that from all these places. If not for code size, for easier code
maintainability.

r+sr=jst if you do that.

Comment 3

[Alex Vincent [:WeirdAl]]

What's the status on this bug?

Comment 4

[Martijn Wargers (dead)]

*** Bug 324151 has been marked as a duplicate of this bug. ***

Comment 5

[Mike Connor [:mconnor]]

Need to clean up and land this patch

Comment 6

[Erik Fabert]

*** Bug 161855 has been marked as a duplicate of this bug. ***

Comment 7

[Nickolay_Ponomarev]

Attachment: updated patch

Comment 8

[Nickolay_Ponomarev]

(Need to fix the line endings before checking in.)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 241640 [details] [diff] [review]
updated patch

- In nsDocument.cpp:

+PRBool CheckGetElementByIdArg(const nsAString& aId)

This should IMO be a static member method in the class nsDocument, it's only relevant to document objects. And put the return type declaration on its own line to follow the rest of the code around here. r+sr=jst with that fixed.

Comment 10

[Nickolay_Ponomarev]

Attachment: patch v1.1 (checked in)

Fixed the review comment.

Comment 11

[(not currently active) Ted Mielczarek]

Checked in on trunk:

Checking in base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.685; previous revision: 3.684
done
Checking in base/src/nsDocument.h;
/cvsroot/mozilla/content/base/src/nsDocument.h,v  <--  nsDocument.h
new revision: 3.316; previous revision: 3.315
done
Checking in html/document/src/nsHTMLDocument.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v  <--  nsHTMLDocu
ment.cpp
new revision: 3.698; previous revision: 3.697
done
Checking in xml/document/src/nsXMLDocument.cpp;
/cvsroot/mozilla/content/xml/document/src/nsXMLDocument.cpp,v  <--  nsXMLDocumen
t.cpp
new revision: 1.250; previous revision: 1.249
done
Checking in xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocumen
t.cpp
new revision: 1.735; previous revision: 1.734
done

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

why's this new message not localizable?

Comment 13

[Nickolay_Ponomarev]

Because it wasn't localizable in mconnor's patch and I didn't remember it should be made localizable. I'll fix it.

Comment 14

[Nickolay_Ponomarev]

Attachment: make the message localizable

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 242323 [details] [diff] [review]
make the message localizable

nit re dom.properties, fix the newline? I'll let jst cover the nsDocument part.

Comment 16

[Nickolay_Ponomarev]

Yes, biesi also mentioned it on IRC. I am going to fix it before the checkin.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 242323 [details] [diff] [review]
make the message localizable

r+sr=jst with the missing newline added.

Comment 18

[Nickolay_Ponomarev]

Attachment: make the message localizable, v1.1

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 242708 [details] [diff] [review]
make the message localizable, v1.1

Checked in on trunk:
Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.687; previous revision: 3.686
done
Checking in dom/locales/en-US/chrome/dom/dom.properties;
/cvsroot/mozilla/dom/locales/en-US/chrome/dom/dom.properties,v  <--  dom.properties
new revision: 1.7; previous revision: 1.6
done