[Webkit-unassigned] [Bug 79820] [BlackBerry] Implement features for find-in-page

[bugzilla-daemon at webkit.org]

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


Antonio Gomes <tonikitoo at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #129335|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #9 from Antonio Gomes <tonikitoo at webkit.org>  2012-02-28 19:28:14 PST ---
(From update of attachment 129335)
View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review

Some questions, mainly:

Maybe you could add a ActiveSearchData class to centralize m_activeSearch{String,MatchFrame,MatchCount} in a follow up?

> Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:406
> +Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag)

I am not a fan of this name 'incrementFrame', but I do not have a better suggestion.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:62
> +    if (m_activeMatch && !m_activeMatch.get()->boundaryPointsValid())

you can call m_activeMatch-> directly without the .get().

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:69
> +        m_activeMatchCount = m_webPage->m_page->markAllMatchesForText(text, CaseInsensitive, true, TextMatchMarkerLimit);

add a /* whatIsThisBoolFor */-like comment here.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:75
> +        setMarkerActive(m_activeMatch, false);

Ditto

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:95
> +        | StartInSelection;

do you need the StartInSelection bit, if you just cleared any possible selection in the block above?

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:108
> +        m_currentActiveMatchFrame = DOMSupport::incrementFrame(m_currentActiveMatchFrame, forward, true);
> +        if (findAndMarkText(text, m_activeMatch.get(), findOptions))
> +            return true;
> +    } while (m_currentActiveMatchFrame && startFrame != m_currentActiveMatchFrame);

I think it could be "better" here to actually pass the Frame* you want to search text at, instead of the setting a class member m_currentActiveMatchFrame, and use it from findAndMarkText. 

That is how I would do it, but if you feel strong about how it is now, that is fine with me.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:125
> +        && m_activeSearchString == text.substring(0, m_activeSearchString.length()))

prefix only? or sufix or a substring in general?

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:147
> -void InPageSearchManager::setMarkerActive(WebCore::Range* range, bool active)
> +void InPageSearchManager::setMarkerActive(PassRefPtr<Range> range, bool active)

I think you want Range* here. You are not passing any ownership, right?

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:164
> +        if (frame == m_webPage->mainFrame()) // Don't need to unmark and notify client because the page will be destroyed.

if it is the mainframe, do we need to do any of the above anyways?

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