[webkit-reviews] review denied: [Bug 89950] Classify form control states by their owner forms : [Attachment 149829] Patch 2

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 89950: Classify form control states by their owner forms
https://bugs.webkit.org/show_bug.cgi?id=89950

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

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=149829&action=review


> Source/WebCore/html/FormController.cpp:74
> +    static P* emptyValue() { return reinterpret_cast<P*>(-2); }

Cannot we define a hand-crafted key for null form instead of having this
special traits?
For example, can an empty string be used for that purpose considering that we
could always append an index to each key.
Then we could easily handle the null-case as an early return path.

> Source/WebCore/html/FormController.cpp:78
> +    WTF_MAKE_NONCOPYABLE(FormKeyGenerator);

could be WTF_MAKE_FAST_ALLOCATED

> Source/WebCore/html/FormController.cpp:96
> +	   return "No owner form";

Better be a static variable intead of literal to save extra computation.

> LayoutTests/ChangeLog:1
> +2012-06-27  Kent Tamura  <tkent at chromium.org>

I thinks it would be better to have unit-test like test which verifies
generated keys for various form controls.

> LayoutTests/ChangeLog:9
> +	   Added. This contains some FAIL lines. They are expected.

Could you mentions any bug ids which is going to adress these FAIL if you have?