Bug 89950

Created attachment 149679 [details] WIP

Created attachment 149722 [details] Patch

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

Created attachment 149829 [details] Patch 2 attempt to fix Windows build

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

Created attachment 149874 [details] Patch 3 Addressed review comments

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

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

Created attachment 149887 [details] Patch 4 Update a test result

Comment on attachment 149887 [details] Patch 4 Clearing flags on attachment: 149887 Committed r121420: <http://trac.webkit.org/changeset/121420>

All reviewed patches have been landed. Closing bug.

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.