Bug 52763

Summary: generated unexpected_results.html from unexpected_results.json
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: New BugsAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, mihaip, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
screenshot
none
Patch eric: review+

Description Ojan Vafai 2011-01-19 16:14:09 PST
generated unexpected_results.html from unexpected_results.json
Comment 1 Ojan Vafai 2011-01-19 16:17:29 PST
Created attachment 79513 [details]
Patch
Comment 2 Ojan Vafai 2011-01-19 16:18:31 PST
Created attachment 79514 [details]
screenshot
Comment 3 Eric Seidel (no email) 2011-01-20 03:07:19 PST
Comment on attachment 79513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79513&action=review

> Tools/ChangeLog:15
> +        Unfortunately, we can't just have unexpected_results.html use unexpected_results.json
> +        because Chrome doesn't allow XHR to local files.

This seems lame.
Comment 4 Ojan Vafai 2011-01-20 09:56:33 PST
(In reply to comment #3)
> (From update of attachment 79513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79513&action=review
> 
> > Tools/ChangeLog:15
> > +        Unfortunately, we can't just have unexpected_results.html use unexpected_results.json
> > +        because Chrome doesn't allow XHR to local files.
> 
> This seems lame.

I agree. Blame abarth. He seems to think security is important or something like that. :)
Comment 5 Mihai Parparita 2011-01-20 19:03:56 PST
(In reply to comment #3)
> Tools/ChangeLog:15
> +        Unfortunately, we can't just have unexpected_results.html use unexpected_results.json
> +        because Chrome doesn't allow XHR to local files.

One way around this is to use a local HTTP server, the way the rebaseline server does. 

In fact, is there a reason why we wouldn't want to share the UI code between that and the (unexpected_)results.html display? Should NRWT just launch the server on failures?
Comment 6 Eric Seidel (no email) 2011-01-21 02:56:16 PST
Entertainingly enough, KSVG/KDOM used a server for displaying the results.html, so that they could have effectively "rebasline" links which would set the new resutlts as the expected results. :)
Comment 7 Ojan Vafai 2011-01-25 17:31:19 PST
(In reply to comment #5)
> (In reply to comment #3)
> > Tools/ChangeLog:15
> > +        Unfortunately, we can't just have unexpected_results.html use unexpected_results.json
> > +        because Chrome doesn't allow XHR to local files.
> 
> One way around this is to use a local HTTP server, the way the rebaseline server does. 
> 
> In fact, is there a reason why we wouldn't want to share the UI code between that and the (unexpected_)results.html display? Should NRWT just launch the server on failures?

It probably makes sense for them to share UI code. I don't like needing to run a local http server though. For one thing, it's just more complicated than is worthwhile for what this is. Inlining doesn't seem like that big of a deal to me. The other, more important thing, is that this page should work on the bots. I guess we could make that work. If you're loading it from a file URL we do something different than an http URL?

In either case, I'll remove the review flag for now. I don't have time to finish this in the next week or two. So feel free to take this over if you feel motivated.
Comment 8 Ojan Vafai 2011-04-13 14:40:55 PDT
Comment on attachment 79513 [details]
Patch

remarking for review as per discussion on #webkit
Comment 9 Eric Seidel (no email) 2011-04-13 14:44:56 PDT
Comment on attachment 79513 [details]
Patch

abarth points out that our *.json files are JSONP files.  We don't need to XHR Them.  WE just include them via a script tag afte defining an ADD_RESULTS function:  http://en.wikipedia.org/wiki/JSONP  The python code here isn't needed.
Comment 10 Ojan Vafai 2011-04-13 16:42:28 PDT
Created attachment 89492 [details]
Patch
Comment 11 Mihai Parparita 2011-04-13 16:59:36 PDT
Comment on attachment 89492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89492&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results.html:53
> +var test_base_path = 'http://trac.webkit.org/export/76053/trunk/LayoutTests/';

Pretty sure you don't want to pin this at r76053.
Comment 12 Eric Seidel (no email) 2011-04-13 17:00:22 PDT
Comment on attachment 89492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89492&action=review

I think this is a great first step.  I look forward to further iteration.  I think you should fix the hard-coded layout test results path though. :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results.html:53
> +var test_base_path = 'http://trac.webkit.org/export/76053/trunk/LayoutTests/';

There is a symlink for you in the results directory iirc.  There is an option in old-run-webkit-tests to generate a results.html with remote links.
Comment 13 Ojan Vafai 2011-04-13 18:13:50 PDT
Committed r83799: <http://trac.webkit.org/changeset/83799>
Comment 14 Dirk Pranke 2011-04-13 19:14:20 PDT
Committed r83806: <http://trac.webkit.org/changeset/83806>
Comment 15 Dirk Pranke 2011-04-13 19:24:29 PDT
FYI ... this patch broke the python unit tests (test-webkitpy). The results.html file doesn't exist if the filesystem object you're using is a mock filesystem. I added a check for existence, but it's unclear what the right thing to do is if the file doesn't exist. Fortunately, this shouldn't happen in practice.