Bug 89950 – Classify form control states by their owner forms

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Store saved form control values per owner-forms

[Kent Tamura]

Created attachment 149679 [details]
WIP

[Kent Tamura]

Created attachment 149722 [details]
Patch

[Build Bot]

Comment on attachment 149722 [details]
Patch

Attachment 149722 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13093796

[Kent Tamura]

Created attachment 149829 [details]
Patch 2

attempt to fix Windows build

[Hajime Morrita]

Comment on attachment 149829 [details]
Patch 2

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
> [phone]  Kent Tamura  <[e-mail]>

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?

[Kent Tamura]

Comment on attachment 149829 [details]
Patch 2

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.

ok, we can avoid the custom HashTraits by handling NULL key outside of the hash.

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

will do.

>> Source/WebCore/html/FormController.cpp:96
>> +        return "No owner form";
> 
> Better be a static variable intead of literal to save extra computation.

will do.

>> LayoutTests/ChangeLog:1
>> [phone]  Kent Tamura  <[e-mail]>
> 
> I thinks it would be better to have unit-test like test which verifies generated keys for various form controls.

state-restore-per-form.html produces a complex stateVector. Dumping it in the test will cover such verification.

>> 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?

will do.

[Kent Tamura]

Created attachment 149874 [details]
Patch 3

Addressed review comments

[WebKit Review Bot]

Comment on attachment 149874 [details]
Patch 3

Attachment 149874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13111008

New failing tests:
fast/forms/state-restore-per-form.html

[WebKit Review Bot]

Created attachment 149881 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

Created attachment 149887 [details]
Patch 4

Update a test result

[WebKit Review Bot]

Comment on attachment 149887 [details]
Patch 4

Clearing flags on attachment: 149887

Committed r121420: <http://trac.webkit.org/changeset/121420>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

I found using action-URL + sequence number to distinguish form was not enough.

http://code.google.com/p/chromium/issues/detail?id=34351#c10
In this case, action-URLs are identical and some forms disappear after navigating back.