[webkit-reviews] review denied: [Bug 72281] Fix find on web pages with -webkit-user-select: none for Chromium : [Attachment 117625] 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: Fix find on web pages with -webkit-user-select: none for Chromium
https://bugs.webkit.org/show_bug.cgi?id=72281

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

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


New code has way too much comment! The preferred way of documenting code in
WebKit is to name functions, variables, etc... to be self-descriptive; e.g. by
introducing inline helper functions. It appears that you might want to
restructure your code so that you wouldn't need so much comments. Also you
might want to consider replacing some comments by assertions to check the
conditions.

> Source/WebCore/ChangeLog:4
> +

This blank line should be removed.

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

This comment is no longer accurate. Please update.

> Source/WebKit/chromium/ChangeLog:4
> +

Ditto.

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

Why is range being inside the shadow DOM or not important? That seems like a
strange condition to check here.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1519
> +	   // Active match is changing.

Again, this comment seems useless. Please remove.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1537
> +    const FindOptions findOptions = (options.forward ? 0 : Backwards) |
> +	   (options.matchCase ? 0 : CaseInsensitive) |
> +	   (wrapWithinFrame ? WrapAround : 0) |
> +	   (!options.findNext ? StartInSelection : 0);

WebKit style is to put at the beginning of next line.

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

Seems strange to put these arguments in separate lines. Since WebKit doesn't
have 80-column restriction, you should put them all in one line.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1545
> +	   // Erase all previous tickmarks and highlighting.
> +	   invalidateArea(InvalidateAll);

The comment and the code does two different things. I suggest you remove the
comment.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1551
> +    // Store which frame was active. This will come in handy later when we
> +    // change the active match ordinal below.

This is self-evident from the code. No need to add a comment.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1553
> +    // Set this frame as the active frame (the one with the active
highlight).

Again, this comment repeats the code. Please remove.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1562
> +	   // This is either a Find operation or a Find-next from a new start
point
> +	   // due to a selection, so we set the flag to ask the scoping effort
> +	   // to find the active rect for us so we can update the ordinal (n of
m).

What are you referring to by the ordinal (n of m) ?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1574
> +	       // We are still the active frame, so increment (or decrement)
the

This is so self-evident from the code.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1575
> +	       // |m_activeMatchIndex|, wrapping if needed (on single frame
pages).

The problem is with this m_activeMatchIndex. It's not obvious that this index
is per frame. So you probably want to rename this variable to be self-evident.