Bug 84892 – [BlackBerry] Find-in-page fails to deactivate the old active match when moving backwards

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Andy Chen]

searchStartingPoint was incorrectly initialized.

[Andy Chen]

Created attachment 138888 [details]
patch

[Mike Fenton]

Comment on attachment 138888 [details]
patch

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

> Source/WebKit/blackberry/ChangeLog:8
> +        Find in page fails to deactivatee the old active match when moving backwards

Typo deactivate

> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:107
> +        searchStartingPoint->setEnd(m_activeMatch->endPosition());

This seems unnecessary.  It should use     static PassRefPtr<Range> create(PassRefPtr<Document>, const Position&, const Position&); to constructor the range rather can create one for the entire page and reset both start and end.

Can you explain why the copy of the range is invalid, but using the details is?  Is the problem the use of RefPtr?

[Andy Chen]

Comment on attachment 138888 [details]
patch

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

>> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:107
>> +        searchStartingPoint->setEnd(m_activeMatch->endPosition());
> 
> This seems unnecessary.  It should use     static PassRefPtr<Range> create(PassRefPtr<Document>, const Position&, const Position&); to constructor the range rather can create one for the entire page and reset both start and end.
> 
> Can you explain why the copy of the range is invalid, but using the details is?  Is the problem the use of RefPtr?

I can create it in that way.

RefPtr<Range> searchStartingPoint(m_activeMatch) seems like not making a copy of Range, but making a copy of the RefPtr itself.
Later, when we call this line(122): searchStartingPoint->setEnd(searchStartingPoint->startPosition()); it actually changes the end position of m_activeMatch, so if we try to deactivate it, the range has been changed so marker was not found.

[Mike Fenton]

(In reply to comment #3)
> RefPtr<Range> searchStartingPoint(m_activeMatch) seems like not making a copy of Range, but making a copy of the RefPtr itself.
> Later, when we call this line(122): searchStartingPoint->setEnd(searchStartingPoint->startPosition()); it actually changes the end position of m_activeMatch, so if we try to deactivate it, the range has been changed so marker was not found.

Yes, if you want a copy of the actual object you need to use m_activeMatch.get()

Please give that a try.

[Andy Chen]

Created attachment 139011 [details]
patch

[Andy Chen]

Created attachment 139079 [details]
patch

[WebKit Review Bot]

Comment on attachment 139079 [details]
patch

Clearing flags on attachment: 139079

Committed r115424: <http://trac.webkit.org/changeset/115424>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.