[webkit-reviews] review granted: [Bug 120328] Don't keep unassociated elements in the past names map : [Attachment 209840] Reverted unintentional changes to HTMLObjectElement.h

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 120328: Don't keep unassociated elements in the past names map
https://bugs.webkit.org/show_bug.cgi?id=120328

Attachment 209840: Reverted unintentional changes to HTMLObjectElement.h
https://bugs.webkit.org/attachment.cgi?id=209840&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209840&action=review


> Source/WebCore/html/HTMLFormElement.cpp:520
> +    removeFromPastNamesMap(e->asFormNamedItem());

The call to asFormNamedItem should not be needed here. Passing the variable e
directly should work; HTMLImageElement is derived from FormNamedItem.

> Source/WebCore/html/HTMLFormElement.cpp:632
> +{ }

I’d prefer to see these on consecutive lines.

> Source/WebCore/html/HTMLFormElement.cpp:665
> +	       it->value = 0; // Don't break. Single element can have multiple
names.

I think “break” is a little unclear in this comment. I would say “Keep looping”
rather than “Don't break”.

> Source/WebCore/html/HTMLFormElement.cpp:682
> +	  
addToPastNamesMap(toHTMLElement(namedItems.first().get())->asFormNamedItem(),
name); // FIXME: Use RefPtr<Element> in namedItems

What guarantees that namedItems.first() is an HTMLElement?

Also, what does the FIXME mean? If we are going to use Element, then maybe
asFormNamedItem should be moved to Element instead of put in HTMLElement.