[chromium] Have WebFrameImpl::selectionAsMarkup create interchange markup.
Created attachment 124476 [details] Patch
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?
Created attachment 126666 [details] Patch
Tony, can you look at this?
(the patch, that is)
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.
Created attachment 127606 [details] Patch
Comment on attachment 127606 [details] Patch Attachment 127606 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11542469
Created attachment 127656 [details] Patch
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."
This code and test is fine overall, just some style nits. Sorry for the slowness in reviewing.
Created attachment 128112 [details] Patch
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).
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.
Created attachment 128618 [details] Patch
Comment on attachment 128618 [details] Patch Clearing flags on attachment: 128618 Committed r108821: <http://trac.webkit.org/changeset/108821>
All reviewed patches have been landed. Closing bug.