Bug 59267 – Add single column mode to results.html

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Ryosuke Niwa]

Add a new mode where each box has width:100% rather than width: 800px. Also embed each text file into a pre so that iframe doesn't occupy a lot of space when the text file contains little text.

[Ryosuke Niwa]

Created attachment 90826 [details]
implements single column mode

[Ryosuke Niwa]

Created attachment 90827 [details]
reveted unintended change

[Ojan Vafai]

Comment on attachment 90827 [details]
reveted unintended change

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

> LayoutTests/ChangeLog:10
> +        have 100% width as supposed to 800px.

s/supposed/opposed

> LayoutTests/ChangeLog:15
> +        (runTests.runTest.):
> +        (runTests.runTest):
> +        ():

you can delete these. they're just noise.

> LayoutTests/fast/harness/results.html:53
> +.single-column-mode .results-row iframe {

This can just be ".single-column-mode iframe {"

> LayoutTests/fast/harness/results.html:202
> +        var r = new XMLHttpRequest();

s/r/request or s/r/xhr.

> LayoutTests/fast/harness/results.html:213
> +            container.lastChild.innerText = r.responseText;

s/innerText/textContent

> LayoutTests/fast/harness/results.html:216
> +            r.open('GET', src + '?format=txt', true);

You can move the "src + '?format=txt'" into a variable since it's done in two places.

> LayoutTests/fast/harness/results.html:218
> +        } catch (exception) {

You can remove this try/catch if you just don't show the text box in the situations where there would be an exception.

> LayoutTests/fast/harness/results.html:305
> +function reexpandExpectations()

s/reexpand/convert

> LayoutTests/fast/harness/results.html:312
> +            if (expandLink.textContent != '+') {

How about moving this if-check outside of the async. No need to queue up a function that's not going to do anything. Also, you can also check that the src has a ".txt" in it since we only need to convert the txt results.

> LayoutTests/fast/harness/results.html:316
> +                // FIXME: this is inefficient; implement actual conversion.
> +                var result = parentOfType(expandLink, 'tbody').querySelector('.results-row');
> +                result.parentNode.removeChild(result);
> +                expandExpectations(expandLink);

If you factor out the code that creates the result element from appendResultIframe into a function call, you should be able to just create the new result and replaceChild (https://developer.mozilla.org/En/DOM/Node.replaceChild).

You can get all the children that need replacing with document.querySelectorAll('.results-row [src*=".txt"]').

> LayoutTests/fast/harness/results.html:653
> +    html += '<label><input class="single-column" type=checkbox onchange="reexpandExpectations()">Single column</label>';

You shouldn't show this checkbox if we're loading this in a file: url in Chrome given that it doesn't work in that case.

> LayoutTests/fast/harness/resources/results-test.js:312
> +        // Single column mode is initially tunred off

typo: turned

Also, this comment isn't really useful.

[Ryosuke Niwa]

Comment on attachment 90827 [details]
reveted unintended change

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

>> LayoutTests/fast/harness/results.html:653
>> +    html += '<label><input class="single-column" type=checkbox onchange="reexpandExpectations()">Single column</label>';
> 
> You shouldn't show this checkbox if we're loading this in a file: url in Chrome given that it doesn't work in that case.

How do we detect that?

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 90827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90827&action=review
> 
> >> LayoutTests/fast/harness/results.html:653
> >> +    html += '<label><input class="single-column" type=checkbox onchange="reexpandExpectations()">Single column</label>';
> > 
> > You shouldn't show this checkbox if we're loading this in a file: url in Chrome given that it doesn't work in that case.
> 
> How do we detect that?

I think checking the userAgent is fine for now. Could also try doing an XHR, but it's lame to always do a network request for this.

Long-term, it might make sense to add something to the platform to feature detect file access from file urls. abarth, wdyt?

[Ryosuke Niwa]

Thanks for the review!  I've addressed most of your comments.

(In reply to comment #3)
> > LayoutTests/fast/harness/results.html:316
> > +                // FIXME: this is inefficient; implement actual conversion.
> > +                var result = parentOfType(expandLink, 'tbody').querySelector('.results-row');
> > +                result.parentNode.removeChild(result);
> > +                expandExpectations(expandLink);
> 
> If you factor out the code that creates the result element from appendResultIframe into a function call, you should be able to just create the new result and replaceChild (https://developer.mozilla.org/En/DOM/Node.replaceChild).
> 
> You can get all the children that need replacing with document.querySelectorAll('.results-row [src*=".txt"]').

While it's nice to do at some point, I'd stick with the current approach for now.

[Ryosuke Niwa]

Created attachment 91354 [details]
Fixed per Ojan's comments

[Ryosuke Niwa]

Created attachment 91355 [details]
Fixed results.html test

[Ojan Vafai]

(In reply to comment #6)
> (In reply to comment #3)
> > > LayoutTests/fast/harness/results.html:316
> > > +                // FIXME: this is inefficient; implement actual conversion.
> > > +                var result = parentOfType(expandLink, 'tbody').querySelector('.results-row');
> > > +                result.parentNode.removeChild(result);
> > > +                expandExpectations(expandLink);
> > 
> > If you factor out the code that creates the result element from appendResultIframe into a function call, you should be able to just create the new result and replaceChild (https://developer.mozilla.org/En/DOM/Node.replaceChild).
> > 
> > You can get all the children that need replacing with document.querySelectorAll('.results-row [src*=".txt"]').
> 
> While it's nice to do at some point, I'd stick with the current approach for now.

Have you tried the current approach with 50-100 failing tests? It would be super slow.

The change from the current code to one that only replaces the elements it needs to replace seems very small to me. Am I missing something?

[Eric Seidel (no email)]

Ojan, could you please review?  This has been up for nearly a month.

[Ryosuke Niwa]

Comment on attachment 91355 [details]
Fixed results.html test

I'm sure this is out of date now.

[Ryosuke Niwa]

Too many other things to work on.  Will file a new bug if needed.