Full Text Bug Listing

Leandro Graciá Gil

Reported 2012-03-28 06:33:41 PDT

Once the WebKit embedder is available, update DumpRenderTree in order to provide a new Layout Test for the SurroundingText feature in WebCore.

Leandro Graciá Gil

Comment 1 2012-05-31 09:53:39 PDT

Created attachment 145109 [details] Patch

Kent Tamura

Comment 2 2012-05-31 18:35:44 PDT

Comment on attachment 145109 [details] Patch Please consider using window.internals instead of layoutTestController.

Ryosuke Niwa

Comment 3 2012-06-19 01:12:17 PDT

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');

Ryosuke Niwa

Comment 4 2012-06-19 01:12:33 PDT

Please use internals as tkent suggested.

Leandro Graciá Gil

Comment 5 2012-06-19 05:44:14 PDT

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.

Ryosuke Niwa

Comment 6 2012-06-19 10:27:56 PDT

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

Leandro Graciá Gil

Comment 7 2012-06-19 10:37:43 PDT

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

Ryosuke Niwa

Comment 8 2012-06-19 10:41:27 PDT

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

Leandro Graciá Gil

Comment 9 2012-06-19 10:48:16 PDT

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

Leandro Graciá Gil

Comment 10 2012-06-25 12:29:12 PDT

Created attachment 149339 [details] Patch

Leandro Graciá Gil

Comment 11 2012-06-25 12:29:44 PDT

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.

Leandro Graciá Gil

Comment 12 2012-06-25 12:31:51 PDT

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.

Ryosuke Niwa

Comment 13 2012-06-25 13:31:55 PDT

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.

Leandro Graciá Gil

Comment 14 2012-06-25 14:23:56 PDT

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.

Ryosuke Niwa

Comment 15 2012-06-25 14:50:18 PDT

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?

Ryosuke Niwa

Comment 16 2012-06-25 14:50:22 PDT

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?

Leandro Graciá Gil

Comment 17 2012-06-25 16:25:54 PDT

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.

Ryosuke Niwa

Comment 18 2012-06-25 16:33:35 PDT

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.

Leandro Graciá Gil

Comment 19 2012-06-26 12:20:03 PDT

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.

Leandro Graciá Gil

Comment 20 2012-06-26 12:26:33 PDT

Created attachment 149586 [details] Patch

WebKit Review Bot

Comment 21 2012-06-26 12:29:31 PDT

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.

Ryosuke Niwa

Comment 22 2012-06-26 13:17:56 PDT

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.

Ryosuke Niwa

Comment 23 2012-06-26 13:18:04 PDT

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.

Leandro Graciá Gil

Comment 24 2012-06-26 13:25:52 PDT

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.

Leandro Graciá Gil

Comment 25 2012-06-27 04:25:56 PDT

Created attachment 149724 [details] Patch

Leandro Graciá Gil

Comment 26 2012-06-27 04:27:39 PDT

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.

Adam Barth

Comment 27 2012-07-02 10:01:33 PDT

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.

Adam Barth

Comment 28 2012-07-02 10:15:34 PDT

Created attachment 150441 [details] Patch for landing

Leandro Graciá Gil

Comment 29 2012-07-02 10:21:03 PDT

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

Leandro Graciá Gil

Comment 30 2012-07-02 10:22:07 PDT

Created attachment 150444 [details] Patch

Leandro Graciá Gil

Comment 31 2012-07-02 10:22:42 PDT

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.

Ryosuke Niwa

Comment 32 2012-07-02 10:26:20 PDT

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.

Adam Barth

Comment 33 2012-07-02 10:30:10 PDT

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

Ryosuke Niwa

Comment 34 2012-07-02 11:13:11 PDT

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

Leandro Graciá Gil

Comment 35 2012-07-02 11:16:09 PDT

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.

Leandro Graciá Gil

Comment 36 2012-07-02 11:16:31 PDT

Created attachment 150453 [details] Patch

WebKit Review Bot

Comment 37 2012-07-02 11:18:39 PDT

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.

Leandro Graciá Gil

Comment 38 2012-07-02 11:20:23 PDT

(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?

Ryosuke Niwa

Comment 39 2012-07-02 11:57:58 PDT

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

Ryosuke Niwa

Comment 40 2012-07-02 12:19:25 PDT

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.

Adam Barth

Comment 41 2012-07-02 14:49:20 PDT

Created attachment 150478 [details] Patch for landing

WebKit Review Bot

Comment 42 2012-07-02 17:53:44 PDT

Comment on attachment 150478 [details] Patch for landing Clearing flags on attachment: 150478 Committed r121713: <http://trac.webkit.org/changeset/121713>

WebKit Review Bot

Comment 43 2012-07-02 17:53:52 PDT

All reviewed patches have been landed. Closing bug.

Attachments
Patch (11.02 KB, patch) 2012-05-31 09:53 PDT, Leandro Graciá Gil	no flags	Details Formatted Diff Diff
Patch (12.00 KB, patch) 2012-06-25 12:29 PDT, Leandro Graciá Gil	no flags	Details Formatted Diff Diff
Patch (14.93 KB, patch) 2012-06-26 12:26 PDT, Leandro Graciá Gil	no flags	Details Formatted Diff Diff
Patch (14.88 KB, patch) 2012-06-27 04:25 PDT, Leandro Graciá Gil	no flags	Details Formatted Diff Diff
Patch for landing (14.25 KB, patch) 2012-07-02 10:15 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (14.93 KB, patch) 2012-07-02 10:22 PDT, Leandro Graciá Gil	no flags	Details Formatted Diff Diff
Patch (13.58 KB, patch) 2012-07-02 11:16 PDT, Leandro Graciá Gil	no flags	Details Formatted Diff Diff
Patch for landing (13.85 KB, patch) 2012-07-02 14:49 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Bug 82461