[webkit-reviews] review denied: [Bug 72281] [Chromium] fix searching for text on web pages that prevent text selection : [Attachment 115627] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Martin Kosiba
<mkosiba at chromium.org>'s request for review:
Bug 72281: [Chromium] fix searching for text on web pages that prevent text
selection
https://bugs.webkit.org/show_bug.cgi?id=72281

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

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


The overall approach looks good but needs more polishing.

> Source/WebCore/ChangeLog:6
> +	   Adding findStringUsingMarkers to Editor. This new method uses
markers
> +	   to indicate the active find result.

These descriptions should appear below "Reviewed by"

> Source/WebCore/ChangeLog:8
> +	   https://bugs.webkit.org/show_bug.cgi?id=72281

This line should appear immediately below the bug title.

> Source/WebKit/chromium/ChangeLog:14
> +	   [Chromium] fix searching for text on web pages that prevent text
selection
> +
> +	   This will make it possible to search for text that has
> +	   selection disabled (via the webkit-user-select attribute).
> +
> +	   WebFrameImpl::find will use findStringUsingMarkers, which is similar

> +	   to findString except that it manipulates markers directly rather
> +	   than passing the find result in the active selection.
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=72281
> +
> +	   Reviewed by NOBODY (OOPS!).

Ditto about the format.

> Source/WebCore/editing/Editor.cpp:2776
> +	   if (!nextMatch
> +	       || (nextMatch->collapsed(ec)
> +		   && !nextMatch->startContainer()->isInShadowTree()))

It seems like this fits in one line.

> Source/WebCore/editing/Editor.cpp:2783
> +	       scrollRectToVisible(nextMatch->boundingBox(),

We don't wrap lines after -> line this.

> Source/WebCore/editing/Editor.cpp:2785
> +				   ScrollAlignment::alignCenterIfNeeded,
> +				   ScrollAlignment::alignCenterIfNeeded);

Wrong indentation these lines should be indented by exactly 8 spaces (4 spaces
from where nextMatch is).

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1471
> +	       (options.matchCase ? 0 : CaseInsensitive) |
> +	       (wrapWithinFrame ? WrapAround : 0) |
> +	       (!options.findNext ? StartInSelection : 0);

Wrong indentation.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1474
> +    m_activeMatch = frame()->editor()->findStringUsingMarkers(searchText,
> +								
m_activeMatch.get(),
> +								 findOptions);

Ditto.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1818
> +    const bool findResult = frame->find(0,
> +				       cppVariantToWebString(arguments[0]),
> +				       findOptions,
> +				       wrapAround,
> +				       0);

Wrong indentation.

> LayoutTests/ChangeLog:9
> +	   [Chromium] fix searching for text on web pages that prevent text
selection
> +
> +	   Adding a layout test to verify that find works correctly on pages
> +	   that prevent text selection.
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=72281
> +

Wrong format. See above.

>
LayoutTests/editing/text-iterator/findString-selection-disabled-expected.txt:1
> +Searching for ‘o’ in ‘Lorem ipsum dolor sit amet’ and with selection
enabled:

Please use normal sentence instead of some latin.

>
LayoutTests/editing/text-iterator/findString-selection-disabled-expected.txt:8
> +Searching for ‘ipsum’ in ‘Lorem ipsum dolor sit amet’ and with
selection enabled:
> +PASS: Got true as expected.
> +
> +Searching for ‘x’ in ‘Lorem ipsum dolor sit amet’ and with selection
enabled:
> +PASS: Got false as expected.

It's hard to tell what's been tested by just looking at this result.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:12
> +    function log(message)
> +    {
> +	  
document.getElementById("console").appendChild(document.createTextNode(message
+ "\n"));
> +    }

We don't normally indent scripts inside script element. Please outdent them.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:18
> +	   log("Searching for \u2018" + target + "\u2019 in \u2018" + text 
> +	       + "\u2019 and with " + selectionStatus);

These Unicode characters don't show up properly in the patch so you should
avoid using them.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:30
> +	   if (!window.internals || !window.layoutTestController) {
> +	       var found = expectedResult;
> +	   } else {
> +	       var found = layoutTestController.findString(target, []);
> +	   }

I don't see any point in checking window.internals since we're not accessing
internals in this test.

Also, instead of declaring found here, you can just do:
assertTrue("layoutTestController.findString('" + target + "', [])",
expectedResult)

This makes the test output much more descriptive.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:35
> +	   if (found != expectedResult)
> +	       log("FAIL: Got " + found + " but expected " + expectedResult +
".");
> +	   else
> +	       log("PASS: Got " + found + " as expected.");

It seems like this test should be a script test.