Bug 89950 - Classify form control states by their owner forms
Summary: Classify form control states by their owner forms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 89951
Blocks: 23346
  Show dependency treegraph
 
Reported: 2012-06-25 23:59 PDT by Kent Tamura
Modified: 2012-07-12 21:30 PDT (History)
3 users (show)

See Also:


Attachments
WIP (25.22 KB, patch)
2012-06-26 22:28 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch (21.08 KB, patch)
2012-06-27 04:02 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (22.64 KB, patch)
2012-06-27 17:19 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (24.52 KB, patch)
2012-06-27 22:45 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (567.75 KB, application/zip)
2012-06-27 23:32 PDT, WebKit Review Bot
no flags Details
Patch 4 (24.49 KB, patch)
2012-06-27 23:49 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-06-25 23:59:59 PDT
Store saved form control values per owner-forms
Comment 1 Kent Tamura 2012-06-26 22:28:03 PDT
Created attachment 149679 [details]
WIP
Comment 2 Kent Tamura 2012-06-27 04:02:08 PDT
Created attachment 149722 [details]
Patch
Comment 3 Build Bot 2012-06-27 07:01:50 PDT
Comment on attachment 149722 [details]
Patch

Attachment 149722 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13093796
Comment 4 Kent Tamura 2012-06-27 17:19:23 PDT
Created attachment 149829 [details]
Patch 2

attempt to fix Windows build
Comment 5 Hajime Morrita 2012-06-27 20:02:09 PDT
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
> +2012-06-27  Kent Tamura  <tkent@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?
Comment 6 Kent Tamura 2012-06-27 21:49:39 PDT
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
>> +2012-06-27  Kent Tamura  <tkent@chromium.org>
> 
> 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.
Comment 7 Kent Tamura 2012-06-27 22:45:08 PDT
Created attachment 149874 [details]
Patch 3

Addressed review comments
Comment 8 WebKit Review Bot 2012-06-27 23:32:03 PDT
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
Comment 9 WebKit Review Bot 2012-06-27 23:32:07 PDT
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
Comment 10 Kent Tamura 2012-06-27 23:49:38 PDT
Created attachment 149887 [details]
Patch 4

Update a test result
Comment 11 WebKit Review Bot 2012-06-28 01:48:37 PDT
Comment on attachment 149887 [details]
Patch 4

Clearing flags on attachment: 149887

Committed r121420: <http://trac.webkit.org/changeset/121420>
Comment 12 WebKit Review Bot 2012-06-28 01:48:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Kent Tamura 2012-07-12 21:30:05 PDT
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.