[Webkit-unassigned] [Bug 50038] Add word-prefix search options to the text search API

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #9 from Darin Adler <darin at apple.com>  2010-11-29 12:32:16 PST ---
(From update of attachment 75040)
View in context: https://bugs.webkit.org/attachment.cgi?id=75040&action=review

review- just because of those buffer under/overrun questions. I’m sure you also will get the Qt build working. I fear that your guess that it’s the FindOptions.h header may well be correct.

> WebCore/editing/TextIterator.cpp:2026
> +inline bool SearchBuffer::isWordStartMatch(size_t start, size_t length) const

This function might read more clearly decomposed a bit more. The names of the smaller functions could help make it clearer why it’s correct to do both of these checks (actual word starts are always word starts regardless of the medial capital flag).

It’s OK as is, though.

> WebCore/editing/TextIterator.cpp:2034
> +        U16_GET(m_buffer.data(), 0, offset, size, firstCharacter);

I assume that offset is guaranteed to be < size, so there is no problem here, but I did have to look twice at this to be sure.

> WebCore/editing/TextIterator.cpp:2036
> +        U16_PREV(m_buffer.data(), 0, offset, previousCharacter);

If offset is 0, this will read past the beginning of the buffer, which is not good.

> WebCore/editing/TextIterator.cpp:2051
> +            U16_FWD_1(m_buffer.data(), offset, size);
> +            U16_GET(m_buffer.data(), 0, offset, size, nextCharacter);

What guarantees this won’t read off the end of the buffer?

> WebCore/editing/TextIterator.cpp:2109
> +            int wordBoundaryContextStart = matchStart;
> +            U16_BACK_1(m_buffer.data(), 0, wordBoundaryContextStart);

Is matchStart guaranteed to be > 0? If not, then U16_BACK_1 will read off the start of the buffer.

> WebCore/editing/TextIterator.cpp:2124
> +    if (isBadMatch(m_buffer.data() + matchStart, matchedLength)
> +        || m_options & AtWordStarts && !isWordStartMatch(matchStart, matchedLength)) {

I would like this better all on one line.

> WebCore/editing/TextIterator.cpp:2478
> +    if (options & AtWordStarts) {
> +        RefPtr<Range> startRange = it.range();
> +        RefPtr<Range> beforeStartRange = startRange->ownerDocument()->createRange();
> +        ExceptionCode ec = 0;
> +        beforeStartRange->setEnd(startRange->startContainer(), startRange->startOffset(), ec);
> +        SimplifiedBackwardsTextIterator backwardsIterator(beforeStartRange.get(), TextIteratorDefaultBehavior);
> +        while (!backwardsIterator.atEnd()) {
> +            int wordBoundaryContextStart = backwardsIterator.length();
> +            U16_BACK_1(backwardsIterator.characters(), 0, wordBoundaryContextStart);
> +            wordBoundaryContextStart = startOfLastWordBoundaryContext(backwardsIterator.characters(), wordBoundaryContextStart);
> +            int contextLength = backwardsIterator.length() - wordBoundaryContextStart;
> +            if (!buffer.prependContext(backwardsIterator.characters() + wordBoundaryContextStart, contextLength) || wordBoundaryContextStart)
> +                break;
> +            backwardsIterator.advance();
> +        }
> +    }

You could consider putting the entire body of this if statement into a separate function. It needs the iterator, the search buffer, and the options. Giving it a name might help document what it does.

You could use a for loop, although it’s always annoying to do that when the initialization part of the for is long. It would be nice to have the call to the advance function explicitly part of the loop structure.

The U16_BACK_1 call may need a check to see if wordBoundaryContextStart is 0.

> WebCore/page/Page.cpp:522
> +        if (frame->editor()->findString(target, options & ~WrapAround | StartInSelection)) {

Some newer versions of gcc may warn about the mix of & and | without parentheses, correctly relying on operator precedence but confusing to some programmers. I suspect that could happening in the Qt build, or it’s possible it only applies to the && and || operators.

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