Bug 193034 – Add support for using the current text selection as the find string on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

<rdar://problem/45138739>

[Wenson Hsieh]

Created attachment 358074 [details]
WIP patch

[Wenson Hsieh]

Created attachment 358076 [details]
WIP patch

[Wenson Hsieh]

Created attachment 358082 [details]
Patch

[Wenson Hsieh]

Created attachment 358085 [details]
Fix 32-bit macOS build

[Tim Horton]

The name is ever so slightly weird to me; I think I would expect it to be the string you’re currently finding. Which it sometimes is, but not if you edit it in the UI or start a find some way other than Command-E?

[Tim Horton]

Comment on attachment 358085 [details]
Fix 32-bit macOS build

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

> Source/WebKit/UIProcess/Cocoa/FindInPageHelpers.mm:27
> +#import "FindInPageHelpers.h"

"Helpers" is pretty unusual.

Who usually talks to FindController? Maybe it should have a UI-side compatriot and these messages should go through there?

[Wenson Hsieh]

(In reply to Tim Horton from comment #5)
> The name is ever so slightly weird to me; I think I would expect it to be
> the string you’re currently finding. Which it sometimes is, but not if you
> edit it in the UI or start a find some way other than Command-E?

That makes sense! I think a better name for what I currently have here would be _initialStringForFindInPage; that being said, the purpose of this string is to stand in for the system-wide find pasteboard that's present on macOS but not iOS. To this end, it should probably behave like it does on macOS and either allow external clients to update it (e.g. whenever the user edits the find string), or teach WebKit find-in-page API to automatically update it when the find string changes.

As such, I think _stringForFindInPage is actually the right name of the SPI we should expose — only, WebKit should also be updating it as the find string changes. What do you think about this?

[Wenson Hsieh]

Comment on attachment 358085 [details]
Fix 32-bit macOS build

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

>> Source/WebKit/UIProcess/Cocoa/FindInPageHelpers.mm:27
>> +#import "FindInPageHelpers.h"
> 
> "Helpers" is pretty unusual.
> 
> Who usually talks to FindController? Maybe it should have a UI-side compatriot and these messages should go through there?

The common flow of information for find-in-page seems to be: (Views or other UI-side entities…) → WebPageProxy → (IPC) → WebPage → FindController.

 I opted for a "Helper"-file approach because _stringForFindInPage is global state shared throughout the process, so it didn't make sense to tie it to any of the usual suspects (i.e. WebPageProxy, or even something like a FindControllerProxy). We could introduce a FindControllerProxy and add these as static functions on it, but it seems odd to introduce a new FindControllerProxy and either (1) not actually create any instance of it because all we need are the static helpers, or (2) create and keep track of FindControllerProxy, but not actually use it for anything.

Does this seem reasonable?

[Tim Horton]

Sounds good on both counts!

[Wenson Hsieh]

Created attachment 358097 [details]
Patch v2

[Wenson Hsieh]

Created attachment 358099 [details]
Patch v2 (fix macOS 10.12 builds)

[Tim Horton]

Comment on attachment 358099 [details]
Patch v2 (fix macOS 10.12 builds)

This patch introduces the first (and first bunch) of the use of the phrase "find in page" in actual code (vs. tests and changelog entries). Other SPI just calls it "find". I wonder if this patch should follow suit.

[Wenson Hsieh]

(In reply to Tim Horton from comment #12)
> Comment on attachment 358099 [details]
> Patch v2 (fix macOS 10.12 builds)
> 
> This patch introduces the first (and first bunch) of the use of the phrase
> "find in page" in actual code (vs. tests and changelog entries). Other SPI
> just calls it "find". I wonder if this patch should follow suit.

Good point! I'll follow suit and use "find" instead of "findInPage".

[Wenson Hsieh]

Created attachment 358242 [details]
Rename FindInPage to Find

[WebKit Commit Bot]

Comment on attachment 358242 [details]
Rename FindInPage to Find

Clearing flags on attachment: 358242

Committed r239584: <https://trac.webkit.org/changeset/239584>