[webkit-reviews] review denied: [Bug 77307] [chromium] Have WebFrameImpl::selectionAsMarkup create interchange markup. : [Attachment 127656] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Peter Collingbourne
<peter at pcc.me.uk>'s request for review:
Bug 77307: [chromium] Have WebFrameImpl::selectionAsMarkup create interchange
markup.
https://bugs.webkit.org/show_bug.cgi?id=77307

Attachment 127656: Patch
https://bugs.webkit.org/attachment.cgi?id=127656&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127656&action=review


> LayoutTests/http/tests/misc/resources/chromium-selectionAsMarkup.html:20
> +	    sel.indexOf("resources/chromium-selectionAsMarkup.html") != -1)
> +		document.body.innerHTML = "PASS";

This indenting is weird.  && is also supposed to go on a new line to make it
easier to see what is part of the condition and what is the if body.  You could
also just unwrap the if condition.

Also, it would be nice to make each of these checks separate so if one of them
starts failing, it's easier to tell from the test output.

> LayoutTests/platform/chromium/http/tests/misc/selectionAsMarkup.html:3
> +layoutTestController.dumpAsText();
> +layoutTestController.waitUntilDone();

We normally wrap layoutTestController call with a if
(window.layoutTestController).

> LayoutTests/platform/chromium/http/tests/misc/selectionAsMarkup.html:6
> +document.location.href =
"http://localhost:8080/misc/resources/chromium-selectionAsMarkup.html";
> +</script>

I would add some text that says what this test is testing. E.g., "This test
makes sure that the markup used by print selection contains absolute urls and
pushed down styles. This test depends on layoutTestController."