[Webkit-unassigned] [Bug 77307] [chromium] Have WebFrameImpl::selectionAsMarkup create interchange markup.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77307


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #127656|review?                     |review-
               Flag|                            |




--- Comment #10 from Tony Chang <tony at chromium.org>  2012-02-21 11:02:46 PST ---
(From update of attachment 127656)
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."

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.