[webkit-reviews] review denied: [Bug 85282] Extra line-breaks added when copying from source. : [Attachment 139832] Patch

Wed May 2 23:06:11 PDT 2012

Ryosuke Niwa <rniwa at webkit.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 85282: Extra line-breaks added when copying from source.
https://bugs.webkit.org/show_bug.cgi?id=85282

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139832&action=review

r- due to nits.

> Source/WebCore/ChangeLog:9
> +	   Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/ChangeLog:11
> +	   Test:
editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html

Please add this test to LayoutTests/platform/win/editing/pasteboard instead so
that you don't have to add them to Skipped lists.

> Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:48
>  void replaceNewlinesWithWindowsStyleNewlines(String& str)

Is there anyway we can share this function with Windows port?

> Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:54
> +    int index = 0;
> +    int strLength = static_cast<int>(str.length());

We should probably use size_t for these two variables.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:306
> +    int index = 0;
> +    int strLength = static_cast<int>(str.length());

Ditto.

>
LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:
1
> +<html>

Missing DOCTYPE.

>
LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:
9
> +  var data = event.clipboardData.getData('text/plain');
> +  document.getElementById("result").textContent += data.replace(/\r/g,
"\\r").replace(/\n/g, "\\n");
> +  event.preventDefault();

Please use 4-space indentation.

>
LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:
22
> +  var selection = window.getSelection();

I don't think this variable is buying us anything.

>
LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:
25
> +  selection.addRange(range);

s/selection/getSelection()/

>
LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:
27
> +  selection.removeAllRanges();

Ditto.