Bug 82461 - [Chromium] Implement a Layout Test for editing/SurroundingText
Summary: [Chromium] Implement a Layout Test for editing/SurroundingText
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on: 82460
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-03-28 06:33 PDT by Leandro Graciá Gil
Modified: 2012-07-02 17:53 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 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.
Comment 1 Leandro Graciá Gil 2012-05-31 09:53:39 PDT
Created attachment 145109 [details]
Patch
Comment 2 Kent Tamura 2012-05-31 18:35:44 PDT
Comment on attachment 145109 [details]
Patch

Please consider using window.internals instead of layoutTestController.
Comment 3 Ryosuke Niwa 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');
Comment 4 Ryosuke Niwa 2012-06-19 01:12:33 PDT
Please use internals as tkent suggested.
Comment 5 Leandro Graciá Gil 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.
Comment 6 Ryosuke Niwa 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.
Comment 7 Leandro Graciá Gil 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.
Comment 8 Ryosuke Niwa 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.
Comment 9 Leandro Graciá Gil 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.
Comment 10 Leandro Graciá Gil 2012-06-25 12:29:12 PDT
Created attachment 149339 [details]
Patch
Comment 11 Leandro Graciá Gil 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.
Comment 12 Leandro Graciá Gil 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.
Comment 13 Ryosuke Niwa 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.
Comment 14 Leandro Graciá Gil 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.
Comment 15 Ryosuke Niwa 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?
Comment 16 Ryosuke Niwa 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?
Comment 17 Leandro Graciá Gil 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.
Comment 18 Ryosuke Niwa 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.
Comment 19 Leandro Graciá Gil 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.
Comment 20 Leandro Graciá Gil 2012-06-26 12:26:33 PDT
Created attachment 149586 [details]
Patch
Comment 21 WebKit Review Bot 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.
Comment 22 Ryosuke Niwa 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.
Comment 23 Ryosuke Niwa 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.
Comment 24 Leandro Graciá Gil 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.
Comment 25 Leandro Graciá Gil 2012-06-27 04:25:56 PDT
Created attachment 149724 [details]
Patch
Comment 26 Leandro Graciá Gil 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.
Comment 27 Adam Barth 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.
Comment 28 Adam Barth 2012-07-02 10:15:34 PDT
Created attachment 150441 [details]
Patch for landing
Comment 29 Leandro Graciá Gil 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".
Comment 30 Leandro Graciá Gil 2012-07-02 10:22:07 PDT
Created attachment 150444 [details]
Patch
Comment 31 Leandro Graciá Gil 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.
Comment 32 Ryosuke Niwa 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.
Comment 33 Adam Barth 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.
Comment 34 Ryosuke Niwa 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.
Comment 35 Leandro Graciá Gil 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.
Comment 36 Leandro Graciá Gil 2012-07-02 11:16:31 PDT
Created attachment 150453 [details]
Patch
Comment 37 WebKit Review Bot 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.
Comment 38 Leandro Graciá Gil 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?
Comment 39 Ryosuke Niwa 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
Comment 40 Ryosuke Niwa 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.
Comment 41 Adam Barth 2012-07-02 14:49:20 PDT
Created attachment 150478 [details]
Patch for landing
Comment 42 WebKit Review Bot 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>
Comment 43 WebKit Review Bot 2012-07-02 17:53:52 PDT
All reviewed patches have been landed.  Closing bug.