| Summary: | Add support for using the current text selection as the find string on iOS | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||
| Component: | WebKit Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | aestes, bdakin, commit-queue, megan_gardner, thorton, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Wenson Hsieh
2018-12-25 21:43:48 PST
Created attachment 358074 [details]
WIP patch
Created attachment 358076 [details]
WIP patch
Created attachment 358082 [details]
Patch
Created attachment 358085 [details]
Fix 32-bit macOS build
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? 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? (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? 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? Sounds good on both counts! Created attachment 358097 [details]
Patch v2
Created attachment 358099 [details]
Patch v2 (fix macOS 10.12 builds)
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.
(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". Created attachment 358242 [details]
Rename FindInPage to Find
Comment on attachment 358242 [details] Rename FindInPage to Find Clearing flags on attachment: 358242 Committed r239584: <https://trac.webkit.org/changeset/239584> |