WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
138161
webkitpy needs a results.html file to render the results.json file for human eyes.
https://bugs.webkit.org/show_bug.cgi?id=138161
Summary
webkitpy needs a results.html file to render the results.json file for human ...
Jake Nielsen
Reported
2014-10-28 17:16:03 PDT
The results.json file enumerates the test results of test-webkitpy, but it's not human readable.
Attachments
Adds results.html and results-stylesheet.css
(12.33 KB, patch)
2014-10-29 18:34 PDT
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Removes jquery
(12.39 KB, patch)
2014-10-30 17:27 PDT
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Cleaner implementation
(9.13 KB, patch)
2014-11-03 18:36 PST
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Self-contained results page
(4.97 KB, text/html)
2014-11-04 11:50 PST
,
Daniel Bates
no flags
Details
[Alternative Proposal] DRT-like self-contained results page
(6.19 KB, text/html)
2014-11-04 13:39 PST
,
Daniel Bates
no flags
Details
Patch
(10.56 KB, patch)
2015-01-21 14:12 PST
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Patch
(10.44 KB, patch)
2015-01-21 14:16 PST
,
Jake Nielsen
beidson
: review-
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Jake Nielsen
Comment 1
2014-10-29 18:34:43 PDT
Created
attachment 240646
[details]
Adds results.html and results-stylesheet.css We'll probably need to make sure people are happy with the look of results.html before CQ+ing this.
Alexey Proskuryakov
Comment 2
2014-10-29 23:48:34 PDT
Comment on
attachment 240646
[details]
Adds results.html and results-stylesheet.css View in context:
https://bugs.webkit.org/attachment.cgi?id=240646&action=review
> Tools/Scripts/webkitpy/resources/results.html:4 > +<script src="
http://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js
"></script>
I don't think that we should use 3rd party servers here. One of the reasons is that it would leak Apple internal URLs via Referer/Origin headers.
Jake Nielsen
Comment 3
2014-10-30 17:27:15 PDT
Created
attachment 240715
[details]
Removes jquery
Jake Nielsen
Comment 4
2014-10-31 11:50:54 PDT
I you folks know who would care about this change, please CC them
Daniel Bates
Comment 5
2014-10-31 15:16:08 PDT
Comment on
attachment 240715
[details]
Removes jquery View in context:
https://bugs.webkit.org/attachment.cgi?id=240715&action=review
The approach of the patch seems reasonable. I have some questions on the implementation. Out of curiosity, did you make use of any documents when writing the patches for this bug?
> Tools/Scripts/webkitpy/resources/results-stylesheet.css:3 > + font-size:14px; > + font-family:verdana;
Nit: This is OK as-is. We seem to be alternating between having space character after the ':' or not throughout this file. We should pick one style and stick with it throughout this file for consistency.
> Tools/Scripts/webkitpy/resources/results-stylesheet.css:74 > +}
Please add a newline to the end of this file.
> Tools/Scripts/webkitpy/resources/results.html:2 > +<link rel="stylesheet" type="text/css" href="results-stylesheet.css">
Nit: This is OK as-is. The HTML type attribute is unnecessary per <
https://html.spec.whatwg.org/#attr-link-type
>. And all modern web browsers support CSS.
> Tools/Scripts/webkitpy/resources/results.html:8 > +var g_state;
Nit: This variable is unused. I suggest we remove it.
> Tools/Scripts/webkitpy/resources/results.html:23 > +function popTableID(){ > + g_tableIDs++; > + var poppedTableID = "table" + (g_tableIDs - 1); > + return poppedTableID; > +} > + > +function popTestRowID(){ > + g_testRowIDs++; > + var poppedTestRowIDs = "testRow" + (g_testRowIDs - 1); > + return poppedTestRowIDs; > +}
I do not see the benefit of either of these functions. It is unnecessary to assign unique IDs to each <table>, <thead>, and <tbody> so as to implement the expand/collapse user interface because we can identify the element that was clicked by using the information provided in DOM Click event passed to the click handler of the element.
> Tools/Scripts/webkitpy/resources/results.html:41 > +function toggleAttribute(elementID, attributeName){ > + var element = document.getElementById(elementID); > + > + if (!element.hasAttribute(attributeName)) > + { > + element.setAttribute(attributeName,"true"); > + } > + > + if (element.getAttribute(attributeName) === "true") > + { > + element.setAttribute(attributeName,"false"); > + } else { > + element.setAttribute(attributeName,"true"); > + } > + > + return element.getAttribute(attributeName) === "true"; > +}
From my understanding our use of this function is to ultimately show/hide content (such a test result) by causing the appropriate CSS attribute selector (e.g. tbody[expanded="false"] tr.test-result td) to be applied to the element. This seems like an an indirect approach to show/hide content. It seems more straightforward to define a function that toggles applying a CSS class selector using the Element.classList API (*). (*) Or modifying Element.className if we need to support viewing the test results page in a browser or WebKit nightly without Element.classList support (why would we have to?)
> Tools/Scripts/webkitpy/resources/results.html:60 > + var resultingTableHTML = ""; > + if (listOfErrorsOrFailures.length > 0){ > + var currentTableID = popTableID(); > + resultingTableHTML = "<table class='non-passing-table' id=\"" + currentTableID + "\" " + g_ExpandedAttributeName + "=\"true\">"; > + resultingTableHTML += "<thead onClick=\"toggleAttribute('" + currentTableID + "', '" + g_ExpandedAttributeName + "')\"> <tr> <th>" + tableTitle + "</th> </tr> </thead>"; > + resultingTableHTML += "<tbody> <tr> <td> <strong>" + tableMessage + "</strong> </td> </tr> </tbody>"; > + var index; > + for (index = 0; index < listOfErrorsOrFailures.length; index++) { > + var currentTestRowID = popTestRowID(); > + resultingTableHTML += "<tbody id=\"" + currentTestRowID + "\" " + g_ExpandedAttributeName + "=\"false\"> <tr class='test-name' onClick=\"toggleAttribute('" + currentTestRowID + "', '" + g_ExpandedAttributeName + "')\"> <td>" + listOfErrorsOrFailures[index].name + "</td> </tr>"; > + resultingTableHTML += "<tr class='test-result'> <td> <pre>" + listOfErrorsOrFailures[index].result + "</pre> </td> </tr> </tbody>"; > + } > + > + resultingTableHTML += "</table>"; > + } > + > + return resultingTableHTML;
Is there a better way to build up this markup? One idea is to using single-quoted literals instead of double-quoted literals. Then we can use double quote characters inside the string literal without escaping them. Another idea is to use DOM operations to build up some or all of the markup (e.g. document.createElement()) (*) though DOM API tends to be more verbose.
> Tools/Scripts/webkitpy/resources/results.html:75 > + var currentTableID = popTableID(); > + var resultingTableHTML = "<table class='passing-table' id=\"" + currentTableID + "\" " + g_ExpandedAttributeName + "=\"false\">"; > + resultingTableHTML += "<thead onClick=\"toggleAttribute('" + currentTableID + "', '" + g_ExpandedAttributeName + "')\"> <tr> <th>" + tableTitle + "</th> </tr> </thead>"; > + resultingTableHTML += "<tbody> <tr> <td> <strong>" + tableMessage + "</strong> </td> </tr> </tbody>"; > + var index; > + for (index = 0; index < listOfTests.length; index++) { > + resultingTableHTML += "<tbody> <tr> <td>" + listOfTests[index] + "</td> </tr> </tbody>"; > + } > + > + resultingTableHTML += "</table>"; > + > + return resultingTableHTML;
This code in this function is similar to the code in constructNonPassingTestTable(). In particular, lines 64-67 (inclusive) are almost identical to lines 46-49 (inclusive). Can we share more code between these functions?
> Tools/Scripts/webkitpy/resources/results.html:82 > + // TODO: We should really pull a name from the results.json file. "Python Unittest" or some such, > + // and set the main-heading element to something less generic.
Notice that we do not write out such a header (DumpRenderTree Tests) in the results.html file produced by run-webkit-tests. An example of the results.html can be seen at <
https://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r175405%20(9041)/results.html
>. I take you feel such a header would be beneficial? Or envision displaying the results from different test runners on the same page? At the very least, this comment implies that you envision making use of this HTML file and associated CSS to display the results from other test runners (e.g. test-webkitperl - our Perl test runner). If so, then I suggest that we move this file and the associated CSS from directory Tools/Scripts/webkitpy/resources to some non-webkitpy-specific directory since we will need to copy these files when distributing the JSON results produced by other test runners (e.g. test-webkitperl) and it seems weird that non-test-webkitpy test runners would need to know about/reference files in the Python code directory (Tools/Scripts/webkitpy/resources).
> Tools/Scripts/webkitpy/resources/results.html:114 > +</script>
You want to consider want to consider moving this <script> before the </body> to avoid blocking the parsing of the page and hence minimize the amount of time we show a blank page. Then we can remove the HTML attribute onload from the HTML body element (line 116).
> Tools/Scripts/webkitpy/resources/results.html:116 > +<body onload="generatePage()">
The markup up to this point in the document is invalid per section "Optional tags" of the WHATWG HTML living standard (30 October 2014): <
https://html.spec.whatwg.org/multipage/syntax.html#optional-tags
>. In particular, a </head> is needed before the <body>.
> Tools/Scripts/webkitpy/resources/results.html:117 > + <div id="main-content">
We aren't making use of the HTML id attribute for this element. Please remove this attribute.
> Tools/Scripts/webkitpy/resources/results.html:118 > + <source id="testtest" src="results.json"></source>
Can you elaborate on what you are trying to accomplish by using this markup? This is markup is invalid per definition of the HTML source element, <
https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element
>, and section "The source element when used with the picture element, <
https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element-when-used-with-the-picture-element
>, of the WHATWG HTML living standard (30 October 2014). Paraphrasing the aforementioned definition and section, the source element can only appear as a child of <media> or <picture>. That is, it cannot appear as a child of a <div>.
> Tools/Scripts/webkitpy/resources/results.html:119 > + <h1 id="main-heading">Test Results</h1>
We aren't making use of the HTML id attribute for this element. Please remove this attribute.
Jake Nielsen
Comment 6
2014-11-03 18:34:45 PST
(In reply to
comment #5
)
> Comment on
attachment 240715
[details]
> Removes jquery > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=240715&action=review
> > The approach of the patch seems reasonable. I have some questions on the > implementation. Out of curiosity, did you make use of any documents when > writing the patches for this bug?
At the time of the comment, yes (in particular:
http://www.askyb.com/javascript/load-json-file-locally-by-js-without-jquery/
), after I update the patch, only general references were used (
https://html.spec.whatwg.org/multipage/syntax.html#optional-tags
,
http://www.w3.org/TR/XMLHttpRequest/
,
http://www.w3schools.com/html/
,
https://developer.mozilla.org/en-US/docs/Web/API/Element.classList
)
> > > Tools/Scripts/webkitpy/resources/results-stylesheet.css:3 > > + font-size:14px; > > + font-family:verdana; > > Nit: This is OK as-is. We seem to be alternating between having space > character after the ':' or not throughout this file. We should pick one > style and stick with it throughout this file for consistency.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results-stylesheet.css:74 > > +} > > Please add a newline to the end of this file.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:2 > > +<link rel="stylesheet" type="text/css" href="results-stylesheet.css"> > > Nit: This is OK as-is. The HTML type attribute is unnecessary per > <
https://html.spec.whatwg.org/#attr-link-type
>. And all modern web browsers > support CSS.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:8 > > +var g_state; > > Nit: This variable is unused. I suggest we remove it.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:23 > > +function popTableID(){ > > + g_tableIDs++; > > + var poppedTableID = "table" + (g_tableIDs - 1); > > + return poppedTableID; > > +} > > + > > +function popTestRowID(){ > > + g_testRowIDs++; > > + var poppedTestRowIDs = "testRow" + (g_testRowIDs - 1); > > + return poppedTestRowIDs; > > +} > > I do not see the benefit of either of these functions. It is unnecessary to > assign unique IDs to each <table>, <thead>, and <tbody> so as to implement > the expand/collapse user interface because we can identify the element that > was clicked by using the information provided in DOM Click event passed to > the click handler of the element.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:41 > > +function toggleAttribute(elementID, attributeName){ > > + var element = document.getElementById(elementID); > > + > > + if (!element.hasAttribute(attributeName)) > > + { > > + element.setAttribute(attributeName,"true"); > > + } > > + > > + if (element.getAttribute(attributeName) === "true") > > + { > > + element.setAttribute(attributeName,"false"); > > + } else { > > + element.setAttribute(attributeName,"true"); > > + } > > + > > + return element.getAttribute(attributeName) === "true"; > > +} > > From my understanding our use of this function is to ultimately show/hide > content (such a test result) by causing the appropriate CSS attribute > selector (e.g. tbody[expanded="false"] tr.test-result td) to be applied to > the element. This seems like an an indirect approach to show/hide content. > It seems more straightforward to define a function that toggles applying a > CSS class selector using the Element.classList API (*).
Using classList.toggle made everything quite a bit cleaner.
> > (*) Or modifying Element.className if we need to support viewing the test > results page in a browser or WebKit nightly without Element.classList > support (why would we have to?) > > > Tools/Scripts/webkitpy/resources/results.html:60 > > + var resultingTableHTML = ""; > > + if (listOfErrorsOrFailures.length > 0){ > > + var currentTableID = popTableID(); > > + resultingTableHTML = "<table class='non-passing-table' id=\"" + currentTableID + "\" " + g_ExpandedAttributeName + "=\"true\">"; > > + resultingTableHTML += "<thead onClick=\"toggleAttribute('" + currentTableID + "', '" + g_ExpandedAttributeName + "')\"> <tr> <th>" + tableTitle + "</th> </tr> </thead>"; > > + resultingTableHTML += "<tbody> <tr> <td> <strong>" + tableMessage + "</strong> </td> </tr> </tbody>"; > > + var index; > > + for (index = 0; index < listOfErrorsOrFailures.length; index++) { > > + var currentTestRowID = popTestRowID(); > > + resultingTableHTML += "<tbody id=\"" + currentTestRowID + "\" " + g_ExpandedAttributeName + "=\"false\"> <tr class='test-name' onClick=\"toggleAttribute('" + currentTestRowID + "', '" + g_ExpandedAttributeName + "')\"> <td>" + listOfErrorsOrFailures[index].name + "</td> </tr>"; > > + resultingTableHTML += "<tr class='test-result'> <td> <pre>" + listOfErrorsOrFailures[index].result + "</pre> </td> </tr> </tbody>"; > > + } > > + > > + resultingTableHTML += "</table>"; > > + } > > + > > + return resultingTableHTML; > > Is there a better way to build up this markup? One idea is to using > single-quoted literals instead of double-quoted literals. Then we can use > double quote characters inside the string literal without escaping them.
After cleaning up everything else and using single quotes instead of escaped double quotes I think it's clean enough. I didn't look into using DOM operations, but I'm curious to come back to this and see how it fares.
> > Another idea is to use DOM operations to build up some or all of the markup > (e.g. document.createElement()) (*) though DOM API tends to be more verbose. > > > Tools/Scripts/webkitpy/resources/results.html:75 > > + var currentTableID = popTableID(); > > + var resultingTableHTML = "<table class='passing-table' id=\"" + currentTableID + "\" " + g_ExpandedAttributeName + "=\"false\">"; > > + resultingTableHTML += "<thead onClick=\"toggleAttribute('" + currentTableID + "', '" + g_ExpandedAttributeName + "')\"> <tr> <th>" + tableTitle + "</th> </tr> </thead>"; > > + resultingTableHTML += "<tbody> <tr> <td> <strong>" + tableMessage + "</strong> </td> </tr> </tbody>"; > > + var index; > > + for (index = 0; index < listOfTests.length; index++) { > > + resultingTableHTML += "<tbody> <tr> <td>" + listOfTests[index] + "</td> </tr> </tbody>"; > > + } > > + > > + resultingTableHTML += "</table>"; > > + > > + return resultingTableHTML; > > This code in this function is similar to the code in > constructNonPassingTestTable(). In particular, lines 64-67 (inclusive) are > almost identical to lines 46-49 (inclusive). Can we share more code between > these functions?
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:82 > > + // TODO: We should really pull a name from the results.json file. "Python Unittest" or some such, > > + // and set the main-heading element to something less generic. > > Notice that we do not write out such a header (DumpRenderTree Tests) in the > results.html file produced by run-webkit-tests. An example of the > results.html can be seen at > <
https://build.webkit.org/results/
> Apple%20Mavericks%20Release%20WK2%20(Tests)/r175405%20(9041)/results.html>. > I take you feel such a header would be beneficial? Or envision displaying > the results from different test runners on the same page?
I think using this for displaying different test results would be jarring if the title didn't indicate which results pay you're currently looking at.
> > At the very least, this comment implies that you envision making use of this > HTML file and associated CSS to display the results from other test runners > (e.g. test-webkitperl - our Perl test runner). If so, then I suggest that we > move this file and the associated CSS from directory > Tools/Scripts/webkitpy/resources to some non-webkitpy-specific directory > since we will need to copy these files when distributing the JSON results > produced by other test runners (e.g. test-webkitperl) and it seems weird > that non-test-webkitpy test runners would need to know about/reference files > in the Python code directory (Tools/Scripts/webkitpy/resources).
I tend to agree. It's been move to Tools/Scripts/resources. I think the only things that will need to know about it are things within the Scripts directory, so I think this is a good choice.
> > > Tools/Scripts/webkitpy/resources/results.html:114 > > +</script> > > You want to consider want to consider moving this <script> before the > </body> to avoid blocking the parsing of the page and hence minimize the > amount of time we show a blank page. Then we can remove the HTML attribute > onload from the HTML body element (line 116).
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:116 > > +<body onload="generatePage()"> > > The markup up to this point in the document is invalid per section "Optional > tags" of the WHATWG HTML living standard (30 October 2014): > <
https://html.spec.whatwg.org/multipage/syntax.html#optional-tags
>. In > particular, a </head> is needed before the <body>.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:117 > > + <div id="main-content"> > > We aren't making use of the HTML id attribute for this element. Please > remove this attribute.
Fixed in next patch
> > > Tools/Scripts/webkitpy/resources/results.html:118 > > + <source id="testtest" src="results.json"></source> > > Can you elaborate on what you are trying to accomplish by using this markup?
Not without sounding quite dumb. It's been removed.
> > This is markup is invalid per definition of the HTML source element, > <
https://html.spec.whatwg.org/multipage/embedded-content.html#the-source
- > element>, and section "The source element when used with the picture > element, > <
https://html.spec.whatwg.org/multipage/embedded-content.html#the-source
- > element-when-used-with-the-picture-element>, of the WHATWG HTML living > standard (30 October 2014). Paraphrasing the aforementioned definition and > section, the source element can only appear as a child of <media> or > <picture>. That is, it cannot appear as a child of a <div>. > > > Tools/Scripts/webkitpy/resources/results.html:119 > > + <h1 id="main-heading">Test Results</h1> > > We aren't making use of the HTML id attribute for this element. Please > remove this attribute.
Fixed in next patch
Jake Nielsen
Comment 7
2014-11-03 18:36:54 PST
Created
attachment 240897
[details]
Cleaner implementation
Daniel Bates
Comment 8
2014-11-04 11:50:52 PST
Created
attachment 240937
[details]
Self-contained results page For convenience, I extracted files results.html and results-stylesheet.css from
attachment #240897
[details]
and created a self-contained web page with some dummy data to make it straightforward to visualize the proposed results page.
Jake Nielsen
Comment 9
2014-11-04 12:05:17 PST
(In reply to
comment #8
)
> Created
attachment 240937
[details]
> Self-contained results page > > For convenience, I extracted files results.html and results-stylesheet.css > from
attachment #240897
[details]
and created a self-contained web page with > some dummy data to make it straightforward to visualize the proposed results > page.
Sweet! Thanks Dan.
Daniel Bates
Comment 10
2014-11-04 13:23:12 PST
How did you come to the decision to come up with a test results layout that differs from the layout we use for DRT tests? After looking at appearance of the proposed results page (
attachment #240937
[details]
), there is a lot of bolded text on the page. I'm unclear of the need to bold the names of tests and the phrases: "Errored tests are tests that hit exceptions during their execution", "Failed tests are tests that violated one or more assertions", and "Passing tests neither hit exceptions, nor violated assertions." Is it necessary to bold such text? Usually bold text is used either for emphasis or to convey a hierarchy (e.g. title of a chapter). And overuse of bold text could desensitize this embellishment when used for the aforementioned familiar uses. I suggest that we bold text sparingly.
Daniel Bates
Comment 11
2014-11-04 13:39:36 PST
Created
attachment 240944
[details]
[Alternative Proposal] DRT-like self-contained results page For contrast, I put together a rough web page with dummy data that shows the Python results using a layout that is more consistent to the layout we use for the DRT results, <
https://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/results.html?rev=167631
>. I chose to not display the list of passing tests and initially hide the actual results of each test to match the look-and-feel of the DRT results page. Feel free to make use of this web page or scrap it.
Jake Nielsen
Comment 12
2014-11-04 22:09:43 PST
(In reply to
comment #10
)
> How did you come to the decision to come up with a test results layout that > differs from the layout we use for DRT tests? > > After looking at appearance of the proposed results page (
attachment #240937
[details]
> [details]), there is a lot of bolded text on the page. I'm unclear of the > need to bold the names of tests and the phrases: "Errored tests are tests > that hit exceptions during their execution", "Failed tests are tests that > violated one or more assertions", and "Passing tests neither hit exceptions, > nor violated assertions." Is it necessary to bold such text? Usually bold > text is used either for emphasis or to convey a hierarchy (e.g. title of a > chapter). And overuse of bold text could desensitize this embellishment when > used for the aforementioned familiar uses. I suggest that we bold text > sparingly.
Essentially I decided there was a lot of machinery that didn't need to be there, and didn't really like the look and feel, so I thought I'd give it a shot. Unfortunately the end result still looks like something out of 1995 to me, but at least it doesn't have so many moving parts. The bold components are the collapsable ones, with the exception of the phrases you mentioned. Perhaps the phrases shouldn't be bold, I was looking at the passing tests section when I decided to bold it. I thought it should look different from the test names. Maybe that doesn't make sense now that the collapsable test names are bold anyway.
Alexey Proskuryakov
Comment 13
2014-11-07 16:21:24 PST
For what it's worth, I think that I like the alternative proposal better, except that the <pre> content looks too heavy to me.
Jake Nielsen
Comment 14
2015-01-21 14:12:02 PST
Created
attachment 245084
[details]
Patch
Jake Nielsen
Comment 15
2015-01-21 14:15:07 PST
^ Uses Dan's file proposal with a few changes: Puts CSS into results-stylesheet.css Actually loads results.json Adds a passed tests section (Also I just noticed something silly I should change. I'll post another patch shortly.)
Jake Nielsen
Comment 16
2015-01-21 14:16:53 PST
Created
attachment 245085
[details]
Patch
Daniel Bates
Comment 17
2015-01-22 11:58:55 PST
Comment on
attachment 245085
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=245085&action=review
> Tools/ChangeLog:21 > + Adds the script_resources_directory function to allow runtests to copy
Nit: runtests => "the RunTests step"
> Tools/Scripts/resources/results-stylesheet.css:1 > +body {
You removed a FIXME comment that I had placed at the top of the stylesheet section in
attachment #240944
[details]
. It's unfortunate that this file duplicates some of the CSS styles in LayoutTests/fast/harness/results.html. We should look share the common CSS styles between the unit test result page and layout test result page to so as to help ensure a consistent look-and-feel between them. We should add a FIXME comment at the top of this file that remarks about the styles duplicated from LayoutTests/fast/harness/results.html and suggests that we look to share the CSS styles. Maybe something of the form: /* FIXME: This file duplicates some of the styles used in LayoutTests/fast/harness/results.html. We should look to share such CSS styles. */
> Tools/Scripts/resources/results.html:32 > +<script>
I don't see the benefit of demarcating the XMLHttpRequest logic into its own <script>. We should merge the contents of this <script> and the contents of the <script> that begins on line 43 into a single HTML script element.
> Tools/Scripts/resources/results.html:35 > +fileRequest.open('GET', "results.json", false);
This loads results.json synchronously. We should load results.json asynchronously so as to avoid blocking the main thread for an HTTP response. We may also want to consider showing and hiding a loading indicator while waiting for the results.json file to load and once the results.json file is loaded, respectively. We do not need to implement the loading indicator in this patch. It can be done in another bug. I would prefer that we implemented asynchronous loading of results.json in this patch. One way to do this is to move the logic in lines [97, 156] into a function that takes a JavaScript dictionary, say generatePageWithResults(...), that is called from the onload handler of the XMLHttpRequest object with argument fileRequest.response. Nit: Please use double quotes for string literals.
> Tools/Scripts/resources/results.html:36 > +fileRequest.responseType = 'json';
Nit: Please use double quotes for string literals.
> Tools/Scripts/resources/results.html:39 > +var g_jsonp = fileRequest.response
Nit: Missing semicolon at the end of this line.
Brady Eidson
Comment 18
2017-04-24 19:04:29 PDT
Comment on
attachment 245085
[details]
Patch This patch has been pending review since 2015 with no recent activity. It seems unlikely that it would even still apply to trunk in its current form. Clearing from the review queue. Feel free to update and resubmit if the patch is still relevant.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug