Bug 79820

Summary: [BlackBerry] Implement features for find-in-page
Product: WebKit Reporter: Andy Chen <andchen>
Component: New BugsAssignee: Andy Chen <andchen>
Status: RESOLVED FIXED    
Severity: Normal CC: mifenton, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
none
patch
tonikitoo: review-, tonikitoo: commit-queue-
patch
none
patch
none
patch
none
patch
tonikitoo: review+, tonikitoo: commit-queue-
patch none

Description Andy Chen 2012-02-28 11:56:01 PST
- 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.
Comment 1 Andy Chen 2012-02-28 12:25:28 PST
Created attachment 129306 [details]
patch
Comment 2 WebKit Review Bot 2012-02-28 12:28:16 PST
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.
Comment 3 Andy Chen 2012-02-28 12:34:11 PST
Created attachment 129308 [details]
patch
Comment 4 Andy Chen 2012-02-28 13:39:25 PST
Created attachment 129321 [details]
patch
Comment 5 Mike Fenton 2012-02-28 13:47:59 PST
All namespace usages of WebCore:: should be removed from the .cpp's that have using namespace WebCore.

// TODO should be replaced by // FIXME:
Comment 6 Andy Chen 2012-02-28 14:48:40 PST
Created attachment 129334 [details]
patch
Comment 7 Andy Chen 2012-02-28 14:57:24 PST
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::
Comment 8 Andy Chen 2012-02-28 14:58:12 PST
Created attachment 129335 [details]
patch
Comment 9 Antonio Gomes 2012-02-28 19:28:14 PST
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?
Comment 10 Mike Fenton 2012-02-29 07:07:36 PST
(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.
Comment 11 Mike Fenton 2012-02-29 07:10:41 PST
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.
Comment 12 Antonio Gomes 2012-02-29 07:18:00 PST
> > > 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.
Comment 13 Andy Chen 2012-02-29 07:19:39 PST
(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.
Comment 14 Mike Fenton 2012-02-29 07:21:07 PST
(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.
Comment 15 Andy Chen 2012-02-29 07:26:37 PST
(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.
Comment 16 Andy Chen 2012-02-29 11:07:05 PST
(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?
Comment 17 Andy Chen 2012-02-29 11:09:18 PST
(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.
Comment 18 Andy Chen 2012-02-29 13:34:23 PST
Created attachment 129506 [details]
patch
Comment 19 Antonio Gomes 2012-02-29 13:51:28 PST
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?
Comment 20 Mike Fenton 2012-02-29 13:53:13 PST
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.
Comment 21 Andy Chen 2012-02-29 14:00:21 PST
(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.
Comment 22 Andy Chen 2012-02-29 14:09:54 PST
(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.
Comment 23 Mike Fenton 2012-02-29 14:13:36 PST
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?
Comment 24 Andy Chen 2012-02-29 15:15:27 PST
Created attachment 129528 [details]
patch
Comment 25 WebKit Review Bot 2012-02-29 15:18:29 PST
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.
Comment 26 Andy Chen 2012-02-29 15:23:52 PST
Created attachment 129534 [details]
patch
Comment 27 Andy Chen 2012-02-29 15:37:36 PST
Created attachment 129538 [details]
patch
Comment 28 Antonio Gomes 2012-02-29 15:43:02 PST
Comment on attachment 129538 [details]
patch

Mike, please cq+ if you do not have any further requests
Comment 29 Mike Fenton 2012-03-01 04:51:14 PST
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.
Comment 30 Andy Chen 2012-03-01 07:38:55 PST
Created attachment 129703 [details]
patch
Comment 31 WebKit Review Bot 2012-03-01 07:41:45 PST
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.
Comment 32 WebKit Review Bot 2012-03-01 22:10:48 PST
Comment on attachment 129703 [details]
patch

Clearing flags on attachment: 129703

Committed r109506: <http://trac.webkit.org/changeset/109506>
Comment 33 WebKit Review Bot 2012-03-01 22:10:56 PST
All reviewed patches have been landed.  Closing bug.