Bug 82461 – [Chromium] Implement a Layout Test for editing/SurroundingText

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Leandro Graciá Gil]

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]

Created attachment 145109 [details]
Patch

[Kent Tamura]

Comment on attachment 145109 [details]
Patch

Please consider using window.internals instead of layoutTestController.

[Ryosuke Niwa]

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
> [phone]
> +
> +6789. 
> +57th Street and Lake Shore Drive
> +Chicago IL 60637 .
> +12345abc
> +6789
> +12345
> +
> [phone].
> +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]

Please use internals as tkent suggested.

[Leandro Graciá Gil]

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]

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

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

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

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

Created attachment 149339 [details]
Patch

[Leandro Graciá Gil]

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 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 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 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 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 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 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 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 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]

Created attachment 149586 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Ryosuke Niwa]

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 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 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]

Created attachment 149724 [details]
Patch

[Leandro Graciá Gil]

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 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]

Created attachment 150441 [details]
Patch for landing

[Leandro Graciá Gil]

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]

Created attachment 150444 [details]
Patch

[Leandro Graciá Gil]

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 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]

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

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

Created attachment 150453 [details]
Patch

[WebKit Review Bot]

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]

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

Created attachment 150478 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 150478 [details]
Patch for landing

Clearing flags on attachment: 150478

Committed r121713: <http://trac.webkit.org/changeset/121713>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.