Summary: | Classify form control states by their owner forms | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||||||
Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dglazkov, morrita, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 89951 | ||||||||||||||||
Bug Blocks: | 23346 | ||||||||||||||||
Attachments: |
|
Description
Kent Tamura
2012-06-25 23:59:59 PDT
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. |