| Summary: | Implement LayoutTestController::setValueForUser on Windows | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Jessie Berlin <jberlin> |
| Component: | Tools / Tests | Assignee: | Ilya Sherman <isherman> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | abarth, aroben, darin, eric, isherman, jberlin, lynn.neir, webkit.review.bot |
| Priority: | P2 | Keywords: | InRadar, LayoutTestFailure, PlatformOnly |
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Attachments: | |||
|
Description
Jessie Berlin
2011-03-05 15:58:37 PST
(In reply to comment #0) > http://trac.webkit.org/changeset/80412 > > http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/10084 > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r80412%20(10087)/results.html > > http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/26028 > http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r80412%20(26029)/results.html > > Until the failure can be investigated, the failing results should be landed as expected results for platform win. Oops, this is my bad for not landing the expectations with the patch that added the test. The test depends on a LayoutTestController method that's implemented only on Mac. Created attachment 85007 [details]
Patch
Comment on attachment 85007 [details]
Patch
Jessie said the failing results should be landed. Not that the test should be skipped.
(In reply to comment #3) > (From update of attachment 85007 [details]) > Jessie said the failing results should be landed. Not that the test should be skipped. I'd like to understand: What makes the one better than the other? (In reply to comment #4) > (In reply to comment #3) > > Jessie said the failing results should be landed. Not that the test should be skipped. > > I'd like to understand: What makes the one better than the other? The basics: If test results are landed, we can easily track if any change in behavior occurs, or if a crash is introduced. If we simply skip a test, then the code covered by the test is not exercised at all, and future patches may change behavior without anyone noticing. (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Jessie said the failing results should be landed. Not that the test should be skipped. > > > > I'd like to understand: What makes the one better than the other? > > The basics: If test results are landed, we can easily track if any change in behavior occurs, or if a crash is introduced. If we simply skip a test, then the code covered by the test is not exercised at all, and future patches may change behavior without anyone noticing. Hmm... that makes sense, but I'm not sure how it applies to this particular bug. This particular test depends on a LayoutTestController method that is not implemented on Windows. Without this method, I wouldn't expect the test to provide any additional code coverage. Is the idea to run the test just in case it happens to provide additional code coverage unrelated to the main functionality that it's meant to test? Created attachment 85024 [details]
Set expectations
Created attachment 85025 [details]
Set expectations, valid diff
(copy pasting diff output because webkit-patch upload is misbehaving... hopefully without errors this time)
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > Jessie said the failing results should be landed. Not that the test should be skipped. > > > > > > I'd like to understand: What makes the one better than the other? > > > > The basics: If test results are landed, we can easily track if any change in behavior occurs, or if a crash is introduced. If we simply skip a test, then the code covered by the test is not exercised at all, and future patches may change behavior without anyone noticing. > > Hmm... that makes sense, but I'm not sure how it applies to this particular bug. This particular test depends on a LayoutTestController method that is not implemented on Windows. Without this method, I wouldn't expect the test to provide any additional code coverage. Is the idea to run the test just in case it happens to provide additional code coverage unrelated to the main functionality that it's meant to test? In this case, where the LayoutTestController method is not implemented on Windows at all, I think we usually add it to the skip list because, as you point out, it doesn't really provide any additional code coverage as is. When I originally suggested that failing expectations be landed, I did not realize that it was failing due to a LayoutTestController method being implemented on Windows. Created attachment 85122 [details]
Patch
*** Bug 56104 has been marked as a duplicate of this bug. *** Comment on attachment 85122 [details] Patch I think checking in expected failure results is in fact the right thing to do. In general our rule is: if the test is failing the same way every time, and isn't crashing or timing out, we should check in expected failure results. See http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10017 for some discussion of this policy. Sorry for all the confusion. I'll take care of fixing the results. Added Windows expected failure results Committed r80730: <http://trac.webkit.org/changeset/80730> Created attachment 128797 [details]
implementation for LayoutTestController::setValueForUser in LayoutTestControllerWin.cpp
I have implementation for this method. Please review attached code. The test /fast/forms/onchange-setvalueforuser.html now passes with this code (at least on wincairo port).
I have attached the modified method LayoutTestController::setValueForUser in Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp
Sorry if I submitted patch incorrectly, I am not sure how this is done.
Thanks,
Lynn
Comment on attachment 128797 [details] implementation for LayoutTestController::setValueForUser in LayoutTestControllerWin.cpp Thanks for the patch! It looks like this isn't actually a patch file. <http://www.webkit.org/coding/contributing.html> describes how to make a patch file. You should also set patches you wish to be reviewed to "r?", not "r+". Created attachment 129992 [details]
patch file with code to provide fix for this bug.
Created attachment 129993 [details]
updated Layout test for patch fix.
Attachment 129992 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/w..." exit_code: 1
Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:800: Tab found; better to use spaces [whitespace/tab] [1]
Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:802: Tab found; better to use spaces [whitespace/tab] [1]
Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:804: Tab found; better to use spaces [whitespace/tab] [1]
Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:805: Tab found; better to use spaces [whitespace/tab] [1]
Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:807: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 5 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 129993 [details]
updated Layout test for patch fix.
This should not be a separate from the patch that adds support to DRT. You should be landing the test changes at the same time as the DRT changes.
You are going to need to land these results as win-wk2 failing results because you are not adding WKTR support.
Comment on attachment 129992 [details] patch file with code to provide fix for this bug. View in context: https://bugs.webkit.org/attachment.cgi?id=129992&action=review Looks good to me, but you should fix the style issues and combine it with the patches to change the layout test expected results. >> Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:805 >> + valueBSTR.Attach(JSStringCopyBSTR(value)); > > Tab found; better to use spaces [whitespace/tab] [1] You should be able do this one line: // The false parameter tells the _bstr_t constructor to adopt the BSTR we pass it. _bstr_t valueBSTR(JSStringCopyBSTR(value), false); Created attachment 130443 [details]
patch with code fix and updated test case
- patch file that combines changes for source code fix and update to layout test.
- fix style issue (i.e., removed tabs).
- update _bstr_t constructor to simplify code.
(In reply to comment #23) > Created an attachment (id=130443) [details] > patch with code fix and updated test case > > - patch file that combines changes for source code fix and update to layout test. > - fix style issue (i.e., removed tabs). > - update _bstr_t constructor to simplify code. Looks like you lost your ChangeLogs? Did you run prepare-ChangeLog at top level? (In reply to comment #24) > (In reply to comment #23) > > Created an attachment (id=130443) [details] [details] > > patch with code fix and updated test case > > > > - patch file that combines changes for source code fix and update to layout test. > > - fix style issue (i.e., removed tabs). > > - update _bstr_t constructor to simplify code. > > Looks like you lost your ChangeLogs? > > Did you run prepare-ChangeLog at top level? I didn't know I had to that. I just ran prepare-ChangeLog at top level and it produced changes to Tools/ChangeLog and LayoutTest/ChangeLog. What do I do with these changeLog files? LayoutTests/ChangeLog 2012-03-07 Lynn Neir <lynn.neir@skype.net> Need a short description and bug URL (OOPS!) Reviewed by NOBODY (OOPS!). * platform/win/fast/forms/onchange-setvalueforuser-expected.txt: Removed . Tools/ChangeLog: 2012-03-07 Lynn Neir <lynn.neir@skype.net> Need a short description and bug URL (OOPS!) Reviewed by NOBODY (OOPS!). * DumpRenderTree/win/LayoutTestControllerWin.cpp: (LayoutTestController::setValueForUser): Lynn (In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > Created an attachment (id=130443) [details] [details] [details] > > > patch with code fix and updated test case > > > > > > - patch file that combines changes for source code fix and update to layout test. > > > - fix style issue (i.e., removed tabs). > > > - update _bstr_t constructor to simplify code. > > > > Looks like you lost your ChangeLogs? > > > > Did you run prepare-ChangeLog at top level? > > I didn't know I had to that. I just ran prepare-ChangeLog at top level and it produced changes to Tools/ChangeLog and LayoutTest/ChangeLog. What do I do with these changeLog files? > > LayoutTests/ChangeLog > 2012-03-07 Lynn Neir <lynn.neir@skype.net> > > Need a short description and bug URL (OOPS!) Replace this line (and the one in Tools/ChangeLog) with the title and URL of the bug: Implement LayoutTestController::setValueForUser on Windows https://bugs.webkit.org/show_bug.cgi?id=55834 > > Reviewed by NOBODY (OOPS!). When I r+ your patch (which I am holding off on doing because you don't have the ChangeLogs included yet and you didn't move the expected results to the win-wk2 directory), this line will become: Reviewed by Jessie Berlin. However, you should leave it alone (Reviewed by NOBODY (OOPS!)) when you post another version of your patch to this bugzilla. Below that, you should add a brief summary of your changes. In this case, when you move the expected results to the win-wk2 directory because you are only fixing DRT and not WKTR, that summary would be something like: "Moved expected failing results to win-wk2 because the test should now pass on DRT". > > * platform/win/fast/forms/onchange-setvalueforuser-expected.txt: Removed The reason I want you to move the expected failing results to LayoutTests/platform/win-wk2/fast/forms/onchange-setvalueforuser-expected.txt is that the change you are making is only to DumpRenderTree, which only runs the tests in WebKit1. In order to make this test pass in WebKit2, you would need to add similar support to WebKitTestRunner. While that would be great to do, it doesn't have to be done in this patch. However, we need to be expecting the failing results when we run the test in WebKit2 on Windows. Does that make sense? http://www.webkit.org/coding/contributing.html has more information about how to use prepare-ChangeLog and how to submit a patch in general. Created attachment 131888 [details]
implementation for setValueForUser in DumpRenderTree and WebKitTestRunner
Comment on attachment 131888 [details] implementation for setValueForUser in DumpRenderTree and WebKitTestRunner View in context: https://bugs.webkit.org/attachment.cgi?id=131888&action=review I think it is really great that you are going ahead and adding a WebKitTestRunner implementation! How did you test this fix out for WebKitTestRunner? It doesn't look like you have taken the fast/forms/onchange-setvalueforuser.html off the WK2 Skipped list (found in LayoutTests/platform/wk2/Skipped), so this test still won't run in WK2. > Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:437 > + WKBundleNodeHandleSetHTMLInputElementValueForUser(WKBundleNodeHandleCreate(context, const_cast<JSObjectRef>(element)), toWK(value).get()); You are leaking the WKBundleNodeHandle here. You should use a WKRetainPtr for it: WKRetainPtr<WKBundleNodeHandle> nodeHandle(AdoptWK, WKBundleNodeHandleCreate(context, const_cast<JSObjectRef>(element)), toWK(value).get())); (In reply to comment #29) > (From update of attachment 131888 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131888&action=review > > I think it is really great that you are going ahead and adding a WebKitTestRunner implementation! > > How did you test this fix out for WebKitTestRunner? It doesn't look like you have taken the fast/forms/onchange-setvalueforuser.html off the WK2 Skipped list (found in LayoutTests/platform/wk2/Skipped), so this test still won't run in WK2. > I did see that skipped there but wasn't sure that Skipped files was related to platform/win-wk2/Skipped. The item is not present in later Skipped file. I guess somehow the wk2/Skipped is parent skip file??? So should I remove from platform/wk2/Skipped. > > Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:437 > > + WKBundleNodeHandleSetHTMLInputElementValueForUser(WKBundleNodeHandleCreate(context, const_cast<JSObjectRef>(element)), toWK(value).get()); > > You are leaking the WKBundleNodeHandle here. > > You should use a WKRetainPtr for it: > > WKRetainPtr<WKBundleNodeHandle> nodeHandle(AdoptWK, WKBundleNodeHandleCreate(context, const_cast<JSObjectRef>(element)), toWK(value).get())); Sorry, I'll fix that up. I had it like that then streamlined code but wrongly! (In reply to comment #30) > (In reply to comment #29) > > (From update of attachment 131888 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=131888&action=review > > > > I think it is really great that you are going ahead and adding a WebKitTestRunner implementation! > > > > How did you test this fix out for WebKitTestRunner? It doesn't look like you have taken the fast/forms/onchange-setvalueforuser.html off the WK2 Skipped list (found in LayoutTests/platform/wk2/Skipped), so this test still won't run in WK2. > > > > I did see that skipped there but wasn't sure that Skipped files was related to platform/win-wk2/Skipped. The item is not present in later Skipped file. I guess somehow the wk2/Skipped is parent skip file??? Yes. The Skipped files for win-wk2, mac-wk2, and gtk-wk2 all "inherit" from the wk2 Skipped file. That way it is possible to skip a file for all of WK2 once instead of having to add it to each Skipped file individually. > > So should I remove from platform/wk2/Skipped. Yes (and then run the test in WK2 to make sure it passes: run-webkit-tests -2 LayoutTests/fast/forms/onchange-setvalueforuser.html > > > > Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:437 > > > + WKBundleNodeHandleSetHTMLInputElementValueForUser(WKBundleNodeHandleCreate(context, const_cast<JSObjectRef>(element)), toWK(value).get()); > > > > You are leaking the WKBundleNodeHandle here. > > > > You should use a WKRetainPtr for it: > > > > WKRetainPtr<WKBundleNodeHandle> nodeHandle(AdoptWK, WKBundleNodeHandleCreate(context, const_cast<JSObjectRef>(element)), toWK(value).get())); > > Sorry, I'll fix that up. I had it like that then streamlined code but wrongly! (In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #29) > > > (From update of attachment 131888 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=131888&action=review > > > > > > I think it is really great that you are going ahead and adding a WebKitTestRunner implementation! > > > > > > How did you test this fix out for WebKitTestRunner? It doesn't look like you have taken the fast/forms/onchange-setvalueforuser.html off the WK2 Skipped list (found in LayoutTests/platform/wk2/Skipped), so this test still won't run in WK2. > > > > > > > I did see that skipped there but wasn't sure that Skipped files was related to platform/win-wk2/Skipped. The item is not present in later Skipped file. I guess somehow the wk2/Skipped is parent skip file??? > > Yes. The Skipped files for win-wk2, mac-wk2, and gtk-wk2 all "inherit" from the wk2 Skipped file. That way it is possible to skip a file for all of WK2 once instead of having to add it to each Skipped file individually. > > > > > So should I remove from platform/wk2/Skipped. > > Yes (and then run the test in WK2 to make sure it passes: > > run-webkit-tests -2 LayoutTests/fast/forms/onchange-setvalueforuser.html > yes that the same command line I ran previously and said passed. I guess maybe it ignores skip file in this case? In any case, i'll remove from skipped file and resubmit patch shortly. thanks. Created attachment 131900 [details]
new patch - fixed leak and updated wk2 skipped file.
Comment on attachment 131900 [details]
new patch - fixed leak and updated wk2 skipped file.
r=me!
Do you need me to set commit-queue+ as well? It doesn't look like you are a committer ..
(In reply to comment #34) > (From update of attachment 131900 [details]) > r=me! > > Do you need me to set commit-queue+ as well? It doesn't look like you are a committer .. yes. i am not a committer. Comment on attachment 131900 [details] new patch - fixed leak and updated wk2 skipped file. Rejecting attachment 131900 [details] from review queue. jberlin@webkit.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights. Comment on attachment 131900 [details]
new patch - fixed leak and updated wk2 skipped file.
Yes, I actually do have reviewership.
(In reply to comment #37) > (From update of attachment 131900 [details]) > Yes, I actually do have reviewership. Anything I need to do here? Looks like code has not been committed yet. (In reply to comment #38) > (In reply to comment #37) > > (From update of attachment 131900 [details] [details]) > > Yes, I actually do have reviewership. > > Anything I need to do here? Looks like code has not been committed yet. The commit queue seems be taking quite a long time getting to this patch ... http://queues.webkit.org/queue-status/commit-queue Sorry, the commit-queue has had 3 troubles in the last 3 days. :( 1. some hangs due to some svn.webkit.org outage. 2. the feeder-queue (which is what cq-'s when someone's review-bit isn't set) was failing to update its working copy (which abarth fixed last night). 3. Finally, the cq's were all hung this morning due to bug 81251. I'm very sorry for the outages, and I'm happy to land the patch on Lynn's behalf. I expect the CQ's should catch up sometime after 5PM tonight (when the input rate comes down). The CQs are currently 40 patches behind due to volume after last night's outage (#3 on the above list). We're working on robustness more today, so that hopefully this won't happen again. thanks again for your patience. Comment on attachment 131900 [details] new patch - fixed leak and updated wk2 skipped file. Clearing flags on attachment: 131900 Committed r110909: <http://trac.webkit.org/changeset/110909> All reviewed patches have been landed. Closing bug. Thanks for implementing this, Lynn! :) |