[Webkit-unassigned] [Bug 72281] [Chromium] fix searching for text on web pages that prevent text selection

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72281


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #115627|review?                     |review-
               Flag|                            |




--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org>  2011-11-17 11:40:49 PST ---
(From update of attachment 115627)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.