Bug 77307 – [chromium] Have WebFrameImpl::selectionAsMarkup create interchange markup.

• Component: New Bugs
• Status: RESOLVED FIXED

[Peter Collingbourne]

[chromium] Have WebFrameImpl::selectionAsMarkup create interchange markup.

[Peter Collingbourne]

Created attachment 124476 [details]
Patch

[Daniel Cheng]

Comment on attachment 124476 [details]
Patch

Can you please add the "why" of this change to the ChangeLog? From this patch alone, it's not clear what you're trying to do--for example, why do we need to use AnnotateForInterchange now?

[Peter Collingbourne]

Created attachment 126666 [details]
Patch

[Nico Weber]

Tony, can you look at this?

[Nico Weber]

(the patch, that is)

[Tony Chang]

Comment on attachment 126666 [details]
Patch

This change should have a layout test associated with it.  Since WebFrame::selectionAsMarkup is part of the WebKit API, you can just call it directly from layoutTestController. The test would be chromium specific since it's the only port that has this function.

[Peter Collingbourne]

Created attachment 127606 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127606 [details]
Patch

Attachment 127606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542469

[Peter Collingbourne]

Created attachment 127656 [details]
Patch

[Tony Chang]

Comment on attachment 127656 [details]
Patch

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."

[Tony Chang]

This code and test is fine overall, just some style nits.  Sorry for the slowness in reviewing.

[Peter Collingbourne]

Created attachment 128112 [details]
Patch

[Tony Chang]

Comment on attachment 128112 [details]
Patch

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

> LayoutTests/platform/chromium/http/tests/misc/selectionAsMarkup.html:11
> +This test makes sure that the markup used by print selection
> +contains absolute urls and pushed down styles. This test depends
> +on layoutTestController.

Sorry, it occurs to me that this text will not be visible because of the redirect.  I had meant for the text to be visible when loading the test in a browser (maybe by moving this text to the html file we redirect to).

[Tony Chang]

Comment on attachment 128112 [details]
Patch

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

> LayoutTests/http/tests/misc/resources/chromium-selectionAsMarkup.html:21
> +	if (sel.indexOf("not selection") != -1)
> +		errors += "FAIL: non-selection text found\n";

Are these tabs?  Please use 4 space indents.

[Peter Collingbourne]

Created attachment 128618 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128618 [details]
Patch

Clearing flags on attachment: 128618

Committed r108821: <http://trac.webkit.org/changeset/108821>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.