184686 - convert HTMLFormElement.cpp to use nsTHashtable

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

convert HTMLFormElement.cpp to use the nsTHashtable code instead of nsHashtable.
This depends on the patch in bug 180264.

Comment 1

[Benjamin Smedberg]

Attachment: diff -u

Comment 2

[Benjamin Smedberg]

Attachment: diff -u ; convert nsHTMLFormElement.cpp to use new nsTHashtable

Comment 3

[Benjamin Smedberg]

Comment on attachment 111426 [details] [diff] [review]
diff -u ; convert nsHTMLFormElement.cpp to use new nsTHashtable

this patch is dependent on the new code in bug 180264, which has r=jkeiser

Comment 4

[John Keiser (jkeiser)]

Comment on attachment 111426 [details] [diff] [review]
diff -u ; convert nsHTMLFormElement.cpp to use new nsTHashtable

+  if( NS_FAILED( rv ) )
+  {
+    delete mControls;
+    mControls = nsnull;
+    return rv;
+  }
+  

It is fine to null out mControls, except you'd have to check for its existence
in every single function that uses it (this code will make us crash in OOM). 
May I suggest, make mControls a non-pointer member (an inline member) and make
the methods in mControls itself check whether the hashtable is null to see if
initialization was successful?

>   if( NS_FAILED( rv ) )	
>   {

if() style again.  Please check
http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl.  Every error listed there
looks legitimate except the declaration of mSelectedRadioButtons.  Also the
excessive spaces thing is going on in this patch.

> if(!mNameLookupTable.Init(NS_FORM_CONTROL_LIST_HASHTABLE_SIZE,PR_FALSE)) 
>     return NS_ERROR_OUT_OF_MEMORY;

NS_ENSURE_TRUE() is better here, it will print out when there is a problem (to
help catch problems earlier).

Comment 5

[John Keiser (jkeiser)]

Comment on attachment 111426 [details] [diff] [review]
diff -u ; convert nsHTMLFormElement.cpp to use new nsTHashtable

hmm, thinking further, if Init() returns an error value no one should use the
object anyway, so the fault would be with the caller.  Scratch what I said
there.

Also scratch what I said about mControls.  It's a COM object, we don't want to
make those inline.

Comment 6

[Benjamin Smedberg]

Attachment: diff -u convert nsHTMLFormElement.cpp to use nsTHashtable

version 2 to fix jkeiser's style stuff.  Old code-formatting habits die hard,
I'm afraid.

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 111460 [details] [diff] [review]
diff -u convert nsHTMLFormElement.cpp to use nsTHashtable

r=jkeiser, but please change if (...)
{
to
if (...) {

(put { on the same line as the if)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 111460 [details] [diff] [review]
diff -u convert nsHTMLFormElement.cpp to use nsTHashtable

I'm not a big fan of the pattern (here and elsewhere in the patch) of putting
complicated expressions inside NS_ENSURE_* macros, but if jkeiser doesn't mind
it's ok with me.

>+  NS_ENSURE_TRUE(mSelectedRadioButtons.Init(4, PR_FALSE),
>+                 NS_ERROR_OUT_OF_MEMORY);

(It would be nice if nsInterfaceHashtable.h had documentation on what an
nsInterfaceHashtable was -- in particular, that it owns the values, which is
hidden in the template parameters to the base class.)

sr=dbaron  (Sorry for the delay.)

Comment 9

[Peter Van der Beken [:peterv]]

Is this ready for checkin?

Comment 10

[John Keiser (jkeiser)]

With comment 7, I think so.

Comment 11

[Benjamin Smedberg]

fixed-on-trunk