[webkit-reviews] review denied: [Bug 112126] Implement overtype mode for editable content : [Attachment 194823] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 112126: Implement overtype mode for editable content
https://bugs.webkit.org/show_bug.cgi?id=112126

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

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


> LayoutTests/editing/execCommand/overtype-support.html:12
> +if (document.queryCommandSupported("OverWrite"))
> +    testFailed("'OverWrite' command exposed to JavaScript");
> +else
> +    testPassed("'OverWrite' command not exposed to JavaScript");

It's probably better to use shouldBe instead as in:
shouldBe('document.queryCommandSupported("OverWrite")', 'false');

> LayoutTests/editing/execCommand/overtype-support.html:14
> +if (document.queryCommandState("OverWrite") == false)

Nit: Use === or simply !document.queryCommandState("OverWrite") per WebKit
style.
But really, we should be using shouldBe instead.

> LayoutTests/editing/execCommand/overtype.html:21
> +if (!window.internals) {
> +  log("FAILED: this test requires the 'internals' object.");
> +} else {

Nit: No curly brackets around a single line statement.

> LayoutTests/editing/execCommand/overtype.html:22
> +  Markup.description('This is a test for Overwrite mode');

Nit: Please indent by 4 spaces, not 2 spaces.

> LayoutTests/editing/execCommand/overtype.html:50
> +  element.innerHTML="webkit";

Nit: spaces around =.

> LayoutTests/editing/execCommand/overtype.html:51
> +  selection.setPosition(element, 0);

There's a standard equivalent collapse(element, 0).

> LayoutTests/editing/execCommand/overtype.html:55
> +    moveSelectionForwardByCharacterCommand();
> +  for (i = 0; i < 2; i++)
> +    extendSelectionForwardByCharacterCommand();

Nit: Indent by 4 spaces.

> LayoutTests/editing/execCommand/overtype.html:61
> +  element.innerHTML="物价预期";

Why don't we use entity reference here?

> LayoutTests/editing/execCommand/overtype.html:66
> +  document.execCommand("InsertText", false, '本本本本');

Ditto.

> LayoutTests/editing/execCommand/overtype.html:69
> +  element.innerHTML="<div id=\"rtl-div\" style=\"direction:
rtl;\">1234שדגכ:</div>"

Ditto.

> LayoutTests/editing/execCommand/overtype.html:78
> +  // Do not forget to disable Ovewrite mode
> +  internals.toggleOverwriteModeEnabled(document);

It seems like this should be automatically reset by the test runner. Tests
shouldn't have to do this manually. r- because of this.