[webkit-reviews] review denied: [Bug 79820] [BlackBerry] Implement features for find-in-page : [Attachment 129335] patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Andy Chen <andchen at rim.com>'s
request for review:
Bug 79820: [BlackBerry] Implement features for find-in-page
https://bugs.webkit.org/show_bug.cgi?id=79820

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

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
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?