Once the WebKit embedder is available, update DumpRenderTree in order to provide a new Layout Test for the SurroundingText feature in WebCore.
Created attachment 145109 [details] Patch
Comment on attachment 145109 [details] Patch Please consider using window.internals instead of layoutTestController.
Comment on attachment 145109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145109&action=review > Tools/DumpRenderTree/chromium/WebViewHost.cpp:884 > +WebKit::WebString WebViewHost::textSurroundingElement(const WebKit::WebString& elementId, unsigned offset, unsigned maxLength) > +{ > + // To make testing easier, access an element by its id first. > + WebDocument document = webView()->mainFrame()->document(); > + WebElement element = document.getElementById(elementId); > + if (element.isNull()) > + return WebString(); > + > + // Then try to get its first child if present. Otherwise use is next sibling. > + // In any case it should be the text node we're referring to for content search. > + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); > + if (node.isNull() || !node.isTextNode() || offset >= node.nodeValue().length()) > + return WebString(); > + > + WebSurroundingText surroundingText; > + surroundingText.initialize(node, offset, maxLength); > + return surroundingText.textContent(); > +} r- because we shouldn't be adding this much code just for testing purpose. > LayoutTests/platform/chromium/fast/text/surrounding-text-expected.txt:15 > +.12345 > +6789 12345 > + > +6789. > +57th Street and Lake Shore Drive > +Chicago IL 60637 . > +12345abc > +6789 > +12345 > + > +67890123. > +This is a test example . > +012345678901234567890123456789 > +. 12345test 12345test > +Test the extraction of the text surrounding an element. Please hide this. > LayoutTests/platform/chromium/fast/text/surrounding-text-expected.txt:31 > +PASS surroundingText("test1", 0, 100) is "12345 6789 12345 6789" > +PASS surroundingText("test1", 5, 6) is "89 123" > +PASS surroundingText("test1", 5, 0) is "" > +PASS surroundingText("test1", 5, 1) is "1" > +PASS surroundingText("test1", 6, 2) is "12" > +PASS surroundingText("test2", 0, 100) is "57th Street and Lake Shore Drive Chicago IL 60637" > +PASS surroundingText("test3", 0, 100) is "6789 12345 6789" > +PASS surroundingText("test4", 0, 100) is "This is a test example" > +PASS surroundingText("test5", 15, 12) is "901234567890" > +PASS surroundingText("test6", 0, 100) is "" > +PASS surroundingText("test7", 0, 100) is "" > +PASS successfullyParsed is true This output isn't helpful because it doesn't tell us what they're testing. > LayoutTests/platform/chromium/fast/text/surrounding-text.html:8 > +<!-- Test exploration (limited by form control elements) --!> All these comments are pure noise to me. > LayoutTests/platform/chromium/fast/text/surrounding-text.html:49 > + shouldBeEqualToString('surroundingText("test1", 0, 100)', '12345 6789 12345 6789'); > + shouldBeEqualToString('surroundingText("test1", 5, 6)', '89 123'); > + shouldBeEqualToString('surroundingText("test1", 5, 0)', ''); > + shouldBeEqualToString('surroundingText("test1", 5, 1)', '1'); > + shouldBeEqualToString('surroundingText("test1", 6, 2)', '12'); > + shouldBeEqualToString('surroundingText("test2", 0, 100)', '57th Street and Lake Shore Drive Chicago IL 60637'); > + shouldBeEqualToString('surroundingText("test3", 0, 100)', '6789 12345 6789'); > + shouldBeEqualToString('surroundingText("test4", 0, 100)', 'This is a test example'); > + shouldBeEqualToString('surroundingText("test5", 15, 12)', '901234567890'); > + shouldBeEqualToString('surroundingText("test6", 0, 100)', ''); > + shouldBeEqualToString('surroundingText("test7", 0, 100)', ''); For these test cases and expectations to make sense, we need to also dump the markup or the test markup needs to be dynamically created within shouldBeEqualToString as in: shouldBeEqualToString('surroundingText("<button>.</button>12345<p id="test1">6789 12345</p>6789<button>.</button>")', '12345 6789 12345 6789');
Please use internals as tkent suggested.
Comment on attachment 145109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145109&action=review Taking a look to all other comments. Thanks for reviewing. >> Tools/DumpRenderTree/chromium/WebViewHost.cpp:884 >> +} > > r- because we shouldn't be adding this much code just for testing purpose. Any suggestions about how to test the feature without adding this code? We need to provide a valid text node + offset in order to retrieve the contents.
(In reply to comment #5) > (From update of attachment 145109 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145109&action=review > > Taking a look to all other comments. Thanks for reviewing. > > >> Tools/DumpRenderTree/chromium/WebViewHost.cpp:884 > >> +} > > > > r- because we shouldn't be adding this much code just for testing purpose. > > Any suggestions about how to test the feature without adding this code? We need to provide a valid text node + offset in order to retrieve the contents. Then the function should take a text node, offset, and maximum length.
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 145109 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=145109&action=review > > > > Taking a look to all other comments. Thanks for reviewing. > > > > >> Tools/DumpRenderTree/chromium/WebViewHost.cpp:884 > > >> +} > > > > > > r- because we shouldn't be adding this much code just for testing purpose. > > > > Any suggestions about how to test the feature without adding this code? We need to provide a valid text node + offset in order to retrieve the contents. > > Then the function should take a text node, offset, and maximum length. Are you ok if what this code does (getting the text node from an adjacent node by its id) is moved to js? Otherwise pointing what to test can become quite obscure.
(In reply to comment #7) > Are you ok if what this code does (getting the text node from an adjacent node by its id) is moved to js? Otherwise pointing what to test can become quite obscure. That's fine.
(In reply to comment #8) > (In reply to comment #7) > > Are you ok if what this code does (getting the text node from an adjacent node by its id) is moved to js? Otherwise pointing what to test can become quite obscure. > > That's fine. I'll try that. Thanks.
Created attachment 149339 [details] Patch
Comment on attachment 145109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145109&action=review >> LayoutTests/platform/chromium/fast/text/surrounding-text-expected.txt:15 >> +Test the extraction of the text surrounding an element. > > Please hide this. Done. >> LayoutTests/platform/chromium/fast/text/surrounding-text-expected.txt:31 >> +PASS successfullyParsed is true > > This output isn't helpful because it doesn't tell us what they're testing. Fixed. >> LayoutTests/platform/chromium/fast/text/surrounding-text.html:8 >> +<!-- Test exploration (limited by form control elements) --!> > > All these comments are pure noise to me. Removed. >> LayoutTests/platform/chromium/fast/text/surrounding-text.html:49 >> + shouldBeEqualToString('surroundingText("test7", 0, 100)', ''); > > For these test cases and expectations to make sense, we need to also dump the markup or the test markup needs to be dynamically created within shouldBeEqualToString as in: > shouldBeEqualToString('surroundingText("<button>.</button>12345<p id="test1">6789 12345</p>6789<button>.</button>")', '12345 6789 12345 6789'); Done.
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 > + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose.
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2331 > + WebString elementId = WebString::fromUTF8(arguments[0].toString()); > + WebElement element = webFrame->document().getElementById(elementId); > + if (element.isNull()) { > + result->setNull(); > + return; > + } What!? I thought I made clear that this function should take a Text node as an argument. r- because of this. >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 >> + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); > > Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose. What why do we need to fallback to nextSibling when element doesn't have any children? We shouldn't be doing these crazy black magic behind the scene especially for the testing APIs.
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2331 >> + } > > What!? I thought I made clear that this function should take a Text node as an argument. r- because of this. WebViewHost::textSurroundingElement is the one taking a text node now. I'd gladly do the same here and move these operations to javascript, but I cannot convert an argument to a node. That forces me to use strings with the id as the rest of the code in this file does. I'm open to alternatives. >>> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 >>> + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); >> >> Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose. > > What why do we need to fallback to nextSibling when element doesn't have any children? > We shouldn't be doing these crazy black magic behind the scene especially for the testing APIs. Because since we cannot pass a node we need to pass its id. Therefore, we cannot directly pass text nodes but nodes that are by them or containing them. If you have any suggestions to easily point specific text nodes from the test I'll be happy to follow it.
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review >>>> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 >>>> + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); >>> >>> Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose. >> >> What why do we need to fallback to nextSibling when element doesn't have any children? >> We shouldn't be doing these crazy black magic behind the scene especially for the testing APIs. > > Because since we cannot pass a node we need to pass its id. Therefore, we cannot directly pass text nodes but nodes that are by them or containing them. If you have any suggestions to easily point specific text nodes from the test I'll be happy to follow it. Why you can't pass a node?
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review >>>>>> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 >>>>>> + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); >>>>> >>>>> Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose. >>>> >>>> What why do we need to fallback to nextSibling when element doesn't have any children? >>>> We shouldn't be doing these crazy black magic behind the scene especially for the testing APIs. >>> >>> Because since we cannot pass a node we need to pass its id. Therefore, we cannot directly pass text nodes but nodes that are by them or containing them. If you have any suggestions to easily point specific text nodes from the test I'll be happy to follow it. >> >> Why you can't pass a node? > > Why you can't pass a node? CppVariant (CppArgumentList is a vector of CppVariants) doesn't implement toObject or any other method that would allow me to retrieve the node from the argument. Please correct me if I'm wrong.
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review >>>>>>> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 >>>>>>> + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); >>>>>> >>>>>> Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose. >>>>> >>>>> What why do we need to fallback to nextSibling when element doesn't have any children? >>>>> We shouldn't be doing these crazy black magic behind the scene especially for the testing APIs. >>>> >>>> Because since we cannot pass a node we need to pass its id. Therefore, we cannot directly pass text nodes but nodes that are by them or containing them. If you have any suggestions to easily point specific text nodes from the test I'll be happy to follow it. >>> >>> Why you can't pass a node? >> >> Why you can't pass a node? > > CppVariant (CppArgumentList is a vector of CppVariants) doesn't implement toObject or any other method that would allow me to retrieve the node from the argument. Please correct me if I'm wrong. There is WebBinding::GetElement. You should be able to create a similar function here. Take a look at getElementImpl.
Comment on attachment 149339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149339&action=review >>>>>>>> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2333 >>>>>>>> + WebNode node = element.hasChildNodes() ? element.firstChild() : element.nextSibling(); >>>>>>> >>>>>>> Couldn't move this further into JS since elements cannot be directly retrieved from arguments. Other methods in this file use the id string for this same purpose. >>>>>> >>>>>> What why do we need to fallback to nextSibling when element doesn't have any children? >>>>>> We shouldn't be doing these crazy black magic behind the scene especially for the testing APIs. >>>>> >>>>> Because since we cannot pass a node we need to pass its id. Therefore, we cannot directly pass text nodes but nodes that are by them or containing them. If you have any suggestions to easily point specific text nodes from the test I'll be happy to follow it. >>>> >>>> Why you can't pass a node? >>> >>> Why you can't pass a node? >> >> CppVariant (CppArgumentList is a vector of CppVariants) doesn't implement toObject or any other method that would allow me to retrieve the node from the argument. Please correct me if I'm wrong. > > There is WebBinding::GetElement. You should be able to create a similar function here. Take a look at getElementImpl. Thanks for the suggestion. I'm doing that.
Created attachment 149586 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 149586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149586&action=review > Source/WebKit/chromium/public/WebBindings.h:158 > + // Return true (success) if the given npobj is a node. > + // If so, return that element as a WebNode object. > + WEBKIT_EXPORT static bool getNode(NPObject* element, WebNode*); > + I don't think you need to add this function. Just put the implementation in LayoutTestController.cpp > LayoutTests/platform/chromium/fast/text/surrounding-text.html:17 > + var test = document.getElementById('test'); > + test.innerHTML = html; It seems like we don't nee this local variable. > LayoutTests/platform/chromium/fast/text/surrounding-text.html:22 > + if (here == null) { > + throw 'Test case needs an element with id "here"'; > + } No curly brackets around single line statements. > LayoutTests/platform/chromium/fast/text/surrounding-text.html:27 > + if (node == null) { > + throw 'No node after "here" element'; > + } Ditto.
Comment on attachment 149586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149586&action=review Thanks for the review. I'll apply the changes in your comments tomorrow. >>> Source/WebKit/chromium/public/WebBindings.h:158 >>> + >> >> I don't think you need to add this function. Just put the implementation in LayoutTestController.cpp > > I don't think you need to add this function. Just put the implementation in LayoutTestController.cpp I'll do that. In that case there won't be any change to the WebKit API after the next patch.
Created attachment 149724 [details] Patch
Comment on attachment 149586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149586&action=review >>>> Source/WebKit/chromium/public/WebBindings.h:158 >>>> + >>> >>> I don't think you need to add this function. Just put the implementation in LayoutTestController.cpp >> >> I don't think you need to add this function. Just put the implementation in LayoutTestController.cpp > > I'll do that. In that case there won't be any change to the WebKit API after the next patch. It seems this was not a good idea after all. Moving this functionality to LayoutTestController introduces a layering violation as it will depend on NPV8Objects, V8Nodes and other classes that are defined in WebCore. In fact trying to include them doesn't work. I think adding this function to WebBindings is the best and cleanest way to solve the issue. >>> LayoutTests/platform/chromium/fast/text/surrounding-text.html:17 >>> + test.innerHTML = html; >> >> It seems like we don't nee this local variable. > > It seems like we don't nee this local variable. Fixed. >>> LayoutTests/platform/chromium/fast/text/surrounding-text.html:22 >>> + } >> >> No curly brackets around single line statements. > > No curly brackets around single line statements. Fixed. >>> LayoutTests/platform/chromium/fast/text/surrounding-text.html:27 >>> + } >> >> Ditto. > > Ditto. Fixed.
Comment on attachment 149724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149724&action=review Thanks for addressing rniwa's comments. > Source/WebKit/chromium/public/WebBindings.h:157 > + // Return true (success) if the given npobj is a node. > + // If so, return that element as a WebNode object. > + WEBKIT_EXPORT static bool getNode(NPObject* element, WebNode*); This is a sensible function to expose here. It's analogous to getElement below. > LayoutTests/platform/chromium/fast/text/surrounding-text.html:15 > +function surroundingText(html, offset, maxLength) { You're mixing your indent level a bit here. We generally prefer four-space indent.
Created attachment 150441 [details] Patch for landing
Comment on attachment 150441 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=150441&action=review > Source/WebKit/chromium/public/WebBindings.h:156 > + // If so, return that element as a WebNode object. There's a minor nit here I was going to fix. It should say "return that element" when it should be "return that node".
Created attachment 150444 [details] Patch
Comment on attachment 150441 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=150441&action=review >> Source/WebKit/chromium/public/WebBindings.h:156 >> + // If so, return that element as a WebNode object. > > There's a minor nit here I was going to fix. It should say "return that element" when it should be "return that node". Fixed.
Comment on attachment 150444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150444&action=review > Source/WebKit/chromium/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). This line should appear before the description. > Tools/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). Ditto. > Tools/DumpRenderTree/chromium/WebViewHost.h:132 > + WebKit::WebString textSurroundingElement(const WebKit::WebNode&, unsigned offset, unsigned maxLength); > + Why are we adding this to WebView? > LayoutTests/ChangeLog:11 > + * platform/chromium/fast/text/surrounding-text-expected.txt: Added. > + * platform/chromium/fast/text/surrounding-text.html: Added. This test doesn't belong in fast/text. It should be in editing.
(In reply to comment #32) > (From update of attachment 150444 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150444&action=review > > > Source/WebKit/chromium/ChangeLog:8 > > + Reviewed by NOBODY (OOPS!). > > This line should appear before the description. You keep complaining about this issue, but I'm not sure it matters. > > Tools/DumpRenderTree/chromium/WebViewHost.h:132 > > + WebKit::WebString textSurroundingElement(const WebKit::WebNode&, unsigned offset, unsigned maxLength); > > + > > Why are we adding this to WebView? Do you mean WebViewHost? It looks like the code could go into LayoutTestController.cpp, but I'm not sure it matters much one way or another.
(In reply to comment #33) > > > Tools/DumpRenderTree/chromium/WebViewHost.h:132 > > > + WebKit::WebString textSurroundingElement(const WebKit::WebNode&, unsigned offset, unsigned maxLength); > > > + > > > > Why are we adding this to WebView? > > Do you mean WebViewHost? It looks like the code could go into LayoutTestController.cpp, but I'm not sure it matters much one way or another. I don't think we should be adding new methods on WebViewHost unless it's necessary.
Comment on attachment 150444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150444&action=review >>> Source/WebKit/chromium/ChangeLog:8 >>> + Reviewed by NOBODY (OOPS!). >> >> This line should appear before the description. > > You keep complaining about this issue, but I'm not sure it matters. Fixed. >> Tools/ChangeLog:9 >> + Reviewed by NOBODY (OOPS!). > > Ditto. Fixed. >>> Tools/DumpRenderTree/chromium/WebViewHost.h:132 >>> + >> >> Why are we adding this to WebView? > > Do you mean WebViewHost? It looks like the code could go into LayoutTestController.cpp, but I'm not sure it matters much one way or another. Fixed. >> LayoutTests/ChangeLog:11 >> + * platform/chromium/fast/text/surrounding-text.html: Added. > > This test doesn't belong in fast/text. It should be in editing. Fixed.
Created attachment 150453 [details] Patch
Attachment 150453 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Tools/DumpRenderTree/chromium/LayoutTestController.cpp:64: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #37) > Attachment 150453 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:64: Alphabetical sorting problem. [build/include_order] [4] > Total errors found: 1 in 9 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Looks indeed as a false positive caused by platform/ includes. Can you confirm?
Comment on attachment 150453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150453&action=review >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:64 >> +#include "WebSurroundingText.h" > > Alphabetical sorting problem. [build/include_order] [4] This isn't a false positive. "platform/WebSerializedScriptValue.h" and "platform/WebSize.h" need to be moved below v8/include/v8.h but above webkit/support/webkit_support.h
Comment on attachment 150453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150453&action=review Please fix the header include order before landing it. > LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html:6 > +<head> > +<meta charset="utf-8"> > +<script src="../../../../fast/js/resources/js-test-pre.js"></script> > +</head> Since we don't need to specify charset in this test, you can move the script element to body. > LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html:12 > +<script type="text/javascript"> Type attribute is unnecessary.
Created attachment 150478 [details] Patch for landing
Comment on attachment 150478 [details] Patch for landing Clearing flags on attachment: 150478 Committed r121713: <http://trac.webkit.org/changeset/121713>
All reviewed patches have been landed. Closing bug.