Bug 55834 – Implement LayoutTestController::setValueForUser on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jessie Berlin]

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.

[Ilya Sherman]

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

[Ilya Sherman]

Created attachment 85007 [details]
Patch

[Darin Adler]

Comment on attachment 85007 [details]
Patch

Jessie said the failing results should be landed. Not that the test should be skipped.

[Ilya Sherman]

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

[Darin Adler]

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

[Ilya Sherman]

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

[Ilya Sherman]

Created attachment 85024 [details]
Set expectations

[Ilya Sherman]

Created attachment 85025 [details]
Set expectations, valid diff

(copy pasting diff output because webkit-patch upload is misbehaving... hopefully without errors this time)

[Jessie Berlin]

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

[Ilya Sherman]

Created attachment 85122 [details]
Patch

[Adam Roben (:aroben)]

*** Bug 56104 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

Sorry for all the confusion. I'll take care of fixing the results.

[Adam Roben (:aroben)]

Added Windows expected failure results

Committed r80730: <http://trac.webkit.org/changeset/80730>

[Jessie Berlin]

<rdar://problem/9114972>

[Lynn Neir]

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

[Adam Roben (:aroben)]

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

[Lynn Neir]

Created attachment 129992 [details]
patch file with code to provide fix for this bug.

[Lynn Neir]

Created attachment 129993 [details]
updated Layout test for patch fix.

[WebKit Review Bot]

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.

[Jessie Berlin]

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.

[Jessie Berlin]

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);

[Lynn Neir]

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.

[Jessie Berlin]

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

[Lynn Neir]

(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  <[e-mail]>

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

        Need a short description and bug URL (OOPS!)

        Reviewed by NOBODY (OOPS!).

        * DumpRenderTree/win/LayoutTestControllerWin.cpp:
        (LayoutTestController::setValueForUser):


Lynn

[Jessie Berlin]

(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  <[e-mail]>
> 
>         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?

[Adam Roben (:aroben)]

http://www.webkit.org/coding/contributing.html has more information about how to use prepare-ChangeLog and how to submit a patch in general.

[Lynn Neir]

Created attachment 131888 [details]
implementation for setValueForUser in DumpRenderTree and WebKitTestRunner

[Jessie Berlin]

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()));

[Lynn Neir]

(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!

[Jessie Berlin]

(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!

[Lynn Neir]

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

[Lynn Neir]

Created attachment 131900 [details]
new patch - fixed leak and updated wk2 skipped file.

[Jessie Berlin]

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

[Lynn Neir]

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

[WebKit Review Bot]

Comment on attachment 131900 [details]
new patch - fixed leak and updated wk2 skipped file.

Rejecting attachment 131900 [details] from review queue.

[e-mail] 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.

[Jessie Berlin]

Comment on attachment 131900 [details]
new patch - fixed leak and updated wk2 skipped file.

Yes, I actually do have reviewership.

[Lynn Neir]

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

[Jessie Berlin]

(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

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ilya Sherman]

Thanks for implementing this, Lynn! :)