[webkit-reviews] review denied: [Bug 91228] imagemap without a "name" attribute doesn't work (affects xkcd.com) : [Attachment 165869] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Akash Vaswani
<avaswani at apple.com>'s request for review:
Bug 91228: imagemap without a "name" attribute doesn't work (affects xkcd.com)
https://bugs.webkit.org/show_bug.cgi?id=91228

Attachment 165869: Patch
https://bugs.webkit.org/attachment.cgi?id=165869&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165869&action=review


I think that this patch can use another iteration, primarily for ChangeLog, but
also potentially for some of the questions.

> Source/WebCore/ChangeLog:13
> +	   Bug: Imagemap only looks for maps with natching names and disregards
maps with matching
> +		id attributes.
> +
> +	   Fix: Run the mapping code for cases in which the attribute is id as
well. Additionally,
> +		in the case that name and id of different maps match the usemap
argument, we map 
> +		to the first one encountered. This is the same strategy used by
Opera and Firefox.

I think that this explanation can be shorter, and go into per-funtiton comment
section below.

You say that this matches Opera and Firefox - what about IE?

Is this a change in HTML5 from earlier HTML specs?

> Source/WebCore/html/HTMLMapElement.cpp:107
> -	   if (isIdAttributeName(attribute.name())) {
> +	   if (isIdAttributeName(attribute.name()))

You still need the braces, as there is more than one line inside. We do count
the comments.

> LayoutTests/ChangeLog:10
> +	   * fast/loader/resources/beach-scene.png: Added.

What is the license for this image?

> LayoutTests/fast/loader/map-images-to-id-and-name.html:12
> +		   eventSender.mouseMoveTo(400, 130);
> +		   eventSender.mouseDown();
> +		   eventSender.mouseUp();

Do we have to use EventSender? The downside is that the test won't work in
browser. Did you check existing image map tests for techniques?

If EventSender is a must, please add an explanation to the test about how to
run it manually.