Bug 50737 - Implement "Use Selection for Find" in WebKit2
Summary: Implement "Use Selection for Find" in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-09 00:38 PST by Maciej Stachowiak
Modified: 2010-12-09 10:08 PST (History)
1 user (show)

See Also:

Attachments
Patch (6.10 KB, patch)
2010-12-09 00:48 PST, Maciej Stachowiak 		no flags Details | Formatted Diff | Diff
Patch (6.64 KB, patch)
2010-12-09 01:40 PST, Maciej Stachowiak 		no flags Details | Formatted Diff | Diff
Patch (6.66 KB, patch)
2010-12-09 02:12 PST, Maciej Stachowiak 		mitz: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2010-12-09 00:38:16 PST 
Implement "Use Selection for Find" in WebKit2
Comment 1 Maciej Stachowiak 2010-12-09 00:48:01 PST 
Created attachment 76022 [details]
Patch
Comment 2 Sam Weinig 2010-12-09 00:52:53 PST 
Comment on attachment 76022 [details]
Patch

Does it makes sense to move WebKit1 over to using this as well?
Comment 3 Maciej Stachowiak 2010-12-09 01:27:23 PST 
(In reply to comment #2)
> Does it makes sense to move WebKit1 over to using this as well?

I thought about it, but I'm not sure that would simplify the WebKit Classic implementation overall. It doesn't benefit much from being able to reuse editor command machinery, which in the WebKit2 case handles the magic of asynchronous menu validation.
Comment 4 Maciej Stachowiak 2010-12-09 01:40:26 PST 
Created attachment 76024 [details]
Patch
Comment 5 mitz 2010-12-09 01:46:00 PST 
Comment on attachment 76024 [details]
Patch

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

r=me if you change enabledCopy to enabledTake…

> WebCore/editing/EditorCommand.cpp:1510
> +        { "TakeFindStringFromSelection", { executeTakeFindStringFromSelection, supportedFromMenuOrKeyBinding, enabledCopy, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

I think you meant to use enabledTakeFindFromSelection here, not enabledCopy.

> WebCore/editing/mac/EditorMac.mm:200
> +        systemBeep();

Not sure it’s better to call the cross-platform function here, since this is Mac code, and you can just NSBeep() without incurring the #import cost.

> WebCore/editing/mac/EditorMac.mm:204
> +    NSString* nsSelectedText = m_frame->displayStringModifiedByEncoding(selectedText());

Misplaced star.
Comment 6 Maciej Stachowiak 2010-12-09 02:12:09 PST 
Created attachment 76026 [details]
Patch
Comment 7 Eric Seidel (no email) 2010-12-09 02:34:09 PST 
Attachment 76024 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6955005
Comment 8 mitz 2010-12-09 07:08:31 PST 
Comment on attachment 76026 [details]
Patch

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

> WebCore/editing/mac/EditorMac.mm:204
> +    NSString* nsSelectedText = m_frame->displayStringModifiedByEncoding(selectedText());

Should be NSString *nsSelectedText
Comment 9 Maciej Stachowiak 2010-12-09 10:08:14 PST 
Committed r73621: <http://trac.webkit.org/changeset/73621>