[webkit-reviews] review denied: [Bug 31428] getElementsByTagName("div")['elementId'] lookup fails if two elements have the same id : [Attachment 43862] update test case based on Eric's review

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Chang Shu <Chang.Shu at nokia.com>'s
request for review:
Bug 31428: getElementsByTagName("div")['elementId'] lookup fails if two
elements have the same id
https://bugs.webkit.org/show_bug.cgi?id=31428

Attachment 43862: update test case based on Eric's review
https://bugs.webkit.org/attachment.cgi?id=43862&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I don't see why you use the global variable 'elem':
 var elem = elems['id1'];
 20 shouldBe('elem.getAttribute("name")', '"name1"');

would be more clear as:
shouldBe('elems["id2"].getAttribute("name")', '"name1"');

The old behavior was to return null from elems['id1'] correct?

generally we try to make comments into complete sentences with periods:

 134	     // in case multiple nodes with same name, fall through

// In the case of multiple nodes with the same name, just fall through.


Your ChangeLog comment isn't quite english. :)	Understandable as I'm assuming
english isn't your mother tongue.

5	  Continue search for matching node in case multiple nodes with
 6	   the same Id exist.

Continue to search for matching nodes in the case where multiple nodes have the
same id.

I think we should go one more round.  In general this looks great!