[webkit-reviews] review denied: [Bug 82461] [Chromium] Implement a Layout Test for editing/SurroundingText : [Attachment 149339] 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 149339: Patch
https://bugs.webkit.org/attachment.cgi?id=149339&action=review

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