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

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Chen]

- Make it be able to search text around the whole page instead of single frame.
- Make it be able to start new search from active selection and last active match.

[Andy Chen]

Created attachment 129306 [details]
patch

[WebKit Review Bot]

Attachment 129306 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:25:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Chen]

Created attachment 129308 [details]
patch

[Andy Chen]

Created attachment 129321 [details]
patch

[Mike Fenton]

All namespace usages of WebCore:: should be removed from the .cpp's that have using namespace WebCore.

// TODO should be replaced by // FIXME:

[Andy Chen]

Created attachment 129334 [details]
patch

[Andy Chen]

Comment on attachment 129334 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129334&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:5588
> +void WebPagePrivate::frameUnloaded(const WebCore::Frame* frame)

Forgot to remove WebCore::

[Andy Chen]

Created attachment 129335 [details]
patch

[Antonio Gomes]

Comment on attachment 129335 [details]
patch

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?

[Mike Fenton]

(In reply to comment #9)
> (From update of attachment 129335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review

> > 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?

Correct, it's only needed in the cases with a selection.  No real harm in setting it though if the Range provided is empty.

> 
> > 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.
> 

Would that allow the removal of m_currentActiveMatchFrame entirely?  If so that is preferred.

> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:125
> > +        && m_activeSearchString == text.substring(0, m_activeSearchString.length()))
> 
> prefix only? or sufix or a substring in general?
> 

Prefix only, this case is when adding an additional character to a non-matched string.  No match is possible.

> > 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?

Yes, we aren't unloading the instance so on page navigation we need to clear the cached objects.

[Mike Fenton]

View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review

> Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:407
> +{

Agreed, this was pulled from Page.cpp though so I'm in favor of keeping it but we should include a comment as to the source.

[Antonio Gomes]

> > > 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?
>
> Yes, we aren't unloading the instance so on page navigation we need to clear the cached objects.

I mean, if the frame being unloaded is the main frame, it are in page shutdown process, because Page holds the same Frame reference cross page load.

[Andy Chen]

(In reply to comment #9)
> (From update of attachment 129335 [details])
> 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?
> 
If we have a ActiveSearchData class for those info, there will be only one instance of it in InPageSearchManager. I don't see the point of adding it.

[Mike Fenton]

(In reply to comment #12)
> > Yes, we aren't unloading the instance so on page navigation we need to clear the cached objects.
> 
> I mean, if the frame being unloaded is the main frame, it are in page shutdown process, because Page holds the same Frame reference cross page load.

WebPage* is not destroyed when the main frame is unloaded, so this object lives on.

[Andy Chen]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 129335 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review
> 

> > 
> > > 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.
> > 
> 
> Would that allow the removal of m_currentActiveMatchFrame entirely?  If so that is preferred.
> 

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 129335 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review

> > 
> > > 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.
> > 
> 
> Would that allow the removal of m_currentActiveMatchFrame entirely?  If so that is preferred.
> 

I don't think m_currentActiveMatchFrame could be removed entirely. For this function, it could be called by passing in the pointer.

[Andy Chen]

(In reply to comment #9)
> (From update of attachment 129335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review
> 
> > 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?
> 

Yes, I just need Range* here but I want make ranger.get() to be inside of the function and if I use RefPtr, it does not pass check-webkit-style.

Should I just use raw pointer instead?

[Andy Chen]

(In reply to comment #16)
> (In reply to comment #9)
> > (From update of attachment 129335 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review
> > 
> > > 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?
> > 
> 
> Yes, I just need Range* here but I want make ranger.get() to be inside of the function and if I use RefPtr, it does not pass check-webkit-style.
> 
> Should I just use raw pointer instead?

I mean use raw pointer for m_activeMatch. It is RefPtr<Range> now.

[Andy Chen]

Created attachment 129506 [details]
patch

[Antonio Gomes]

Comment on attachment 129506 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:794
>  void FrameLoaderClientBlackBerry::dispatchWillClose()
>  {
> -    m_webPagePrivate->m_inputHandler->frameUnloaded(m_frame);
> +    m_webPagePrivate->frameUnloaded(m_frame);
>  }

maybe you should also update FrameLoaderClientBB::detachedFromParent2()?

> Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:412
> +// This function is copied from WebCore/page/Page.cpp.
> +Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag)
> +{
> +    return forward
> +        ? curr->tree()->traverseNextWithWrap(wrapFlag)
> +        : curr->tree()->traversePreviousWithWrap(wrapFlag);
> +}

I would rather make it local in PageFindManage till someone needs it.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:25
> +#include <wtf/PassRefPtr.h>

unneeded now?

[Mike Fenton]

Comment on attachment 129506 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:98
> +        currentActiveMatchFrame = m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame();

Why not initialize the variable with this logic?  Or make it. 
Frame* currentActiveMatchFrame = !selection.isNone() && m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame();

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:100
> +    Frame* startFrame = currentActiveMatchFrame;

Move this above the Do loop.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:124
> +        && m_activeSearchString.length()

Should we include a check to make sure that text.length() is greater than m_activeSearchString.length() to avoid the substring?  Or us text.startsWith()

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:158
> +    Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame();

if (!m_activeMatch)
    return;

Would be beneficial here.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:159
> +    if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) {

Why is the mainframe special cased?  If the mainFrame is unloaded so are the subframes.

[Andy Chen]

(In reply to comment #19)
> (From update of attachment 129506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review
> 
> > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:794
> >  void FrameLoaderClientBlackBerry::dispatchWillClose()
> >  {
> > -    m_webPagePrivate->m_inputHandler->frameUnloaded(m_frame);
> > +    m_webPagePrivate->frameUnloaded(m_frame);
> >  }
> 
> maybe you should also update FrameLoaderClientBB::detachedFromParent2()?
Will do.

> 
> > Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:412
> > +// This function is copied from WebCore/page/Page.cpp.
> > +Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag)
> > +{
> > +    return forward
> > +        ? curr->tree()->traverseNextWithWrap(wrapFlag)
> > +        : curr->tree()->traversePreviousWithWrap(wrapFlag);
> > +}
> 
> I would rather make it local in PageFindManage till someone needs it.
It was there until Mike suggested to move over.

> 
> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:25
> > +#include <wtf/PassRefPtr.h>
> 
> unneeded now?

Will remove.

[Andy Chen]

(In reply to comment #20)
> (From update of attachment 129506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review
> 
> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:98
> > +        currentActiveMatchFrame = m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame();
> 
> Why not initialize the variable with this logic?  Or make it. 
> Frame* currentActiveMatchFrame = !selection.isNone() && m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame();
> 
I'll change the logic.

> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:100
> > +    Frame* startFrame = currentActiveMatchFrame;
> 
> Move this above the Do loop.
Will do.
> 
> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:124
> > +        && m_activeSearchString.length()
> 
> Should we include a check to make sure that text.length() is greater than m_activeSearchString.length() to avoid the substring?  Or us text.startsWith()
> 
I'll check.

> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:158
> > +    Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame();
> 
> if (!m_activeMatch)
>     return;
> 
> Would be beneficial here.
> 
If m_activeMatch==0, we don't need to clear other values?

> > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:159
> > +    if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) {
> 
> Why is the mainframe special cased?  If the mainFrame is unloaded so are the subframes.

In both cases we need to clear those values.

[Mike Fenton]

Comment on attachment 129506 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review

>>> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:158
>>> +    Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame();
>> 
>> if (!m_activeMatch)
>>     return;
>> 
>> Would be beneficial here.
> 
> If m_activeMatch==0, we don't need to clear other values?

If currentActiveMatchFrame == 0 you don't go into the block.

>>> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:159
>>> +    if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) {
>> 
>> Why is the mainframe special cased?  If the mainFrame is unloaded so are the subframes.
> 
> In both cases we need to clear those values.

So it's really that if there is no activeMatch you need to reset m_activeSearchString?

[Andy Chen]

Created attachment 129528 [details]
patch

[WebKit Review Bot]

Attachment 129528 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:157:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Chen]

Created attachment 129534 [details]
patch

[Andy Chen]

Created attachment 129538 [details]
patch

[Antonio Gomes]

Comment on attachment 129538 [details]
patch

Mike, please cq+ if you do not have any further requests

[Mike Fenton]

Comment on attachment 129538 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129538&action=review

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:103
> +        if (findAndMarkText(text, m_activeMatch.get(), currentActiveMatchFrame, findOptions))

m_activeMatch is always null in the case, why not pass 0 explicitly?

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:162
> +    Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame();

Remove this check against !m_activeMatch, it was just checked with an early return.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:163
> +    if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) {

The extra check inside the early return removed the need for m_webPage->mainFrame() == frame

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:47
>      void setMarkerActive(WebCore::Range*, bool active);

active parameter should not be included.

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:49
> +    bool shouldSearchForText(const String& text);

text parameter should not be included.

[Andy Chen]

Created attachment 129703 [details]
patch

[WebKit Review Bot]

Attachment 129703 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h"
Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 192 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 129703 [details]
patch

Clearing flags on attachment: 129703

Committed r109506: <http://trac.webkit.org/changeset/109506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.