[webkit-reviews] review denied: [Bug 82461] [Chromium] Implement a Layout Test for editing/SurroundingText : [Attachment 145109] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Leandro Graciá Gil
<leandrogracia at chromium.org>'s request for review:
Bug 82461: [Chromium] Implement a Layout Test for editing/SurroundingText
https://bugs.webkit.org/show_bug.cgi?id=82461

Attachment 145109: Patch
https://bugs.webkit.org/attachment.cgi?id=145109&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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');