[webkit-reviews] review denied: [Bug 59267] Add single column mode to results.html : [Attachment 90827] reveted unintended change

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 59267: Add single column mode to results.html
https://bugs.webkit.org/show_bug.cgi?id=59267

Attachment 90827: reveted unintended change
https://bugs.webkit.org/attachment.cgi?id=90827&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.