Bug 84892 - [BlackBerry] Find-in-page fails to deactivate the old active match when moving backwards
Summary: [BlackBerry] Find-in-page fails to deactivate the old active match when movin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-25 14:47 PDT by Andy Chen
Modified: 2012-04-27 06:51 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.90 KB, patch)
2012-04-25 16:05 PDT, Andy Chen
no flags Details | Formatted Diff | Diff
patch (2.78 KB, patch)
2012-04-26 08:43 PDT, Andy Chen
no flags Details | Formatted Diff | Diff
patch (2.73 KB, patch)
2012-04-26 14:59 PDT, Andy Chen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Chen 2012-04-25 14:47:45 PDT
searchStartingPoint was incorrectly initialized.
Comment 1 Andy Chen 2012-04-25 16:05:53 PDT
Created attachment 138888 [details]
patch
Comment 2 Mike Fenton 2012-04-25 17:06:22 PDT
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?
Comment 3 Andy Chen 2012-04-25 17:26:38 PDT
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.
Comment 4 Mike Fenton 2012-04-26 04:47:45 PDT
(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.
Comment 5 Andy Chen 2012-04-26 08:43:28 PDT
Created attachment 139011 [details]
patch
Comment 6 Andy Chen 2012-04-26 14:59:32 PDT
Created attachment 139079 [details]
patch
Comment 7 WebKit Review Bot 2012-04-27 06:51:42 PDT
Comment on attachment 139079 [details]
patch

Clearing flags on attachment: 139079

Committed r115424: <http://trac.webkit.org/changeset/115424>
Comment 8 WebKit Review Bot 2012-04-27 06:51:48 PDT
All reviewed patches have been landed.  Closing bug.