Bug 235691 - Introduce WebFoundTextRange and WebFoundTextRangeController to support restorable find results
Summary: Introduce WebFoundTextRange and WebFoundTextRangeController to support restor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks: 235692 235693
  Show dependency treegraph
 
Reported: 2022-01-26 23:38 PST by Aditya Keerthi
Modified: 2022-01-27 23:32 PST (History)
7 users (show)

See Also:


Attachments
Patch (43.74 KB, patch)
2022-01-27 11:55 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (43.80 KB, patch)
2022-01-27 14:17 PST, Aditya Keerthi
wenson_hsieh: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (44.68 KB, patch)
2022-01-27 15:37 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2022-01-26 23:38:17 PST
...
Comment 1 Aditya Keerthi 2022-01-26 23:38:36 PST
rdar://88117167
Comment 2 Aditya Keerthi 2022-01-27 11:55:58 PST
Created attachment 450162 [details]
Patch
Comment 3 Wenson Hsieh 2022-01-27 12:15:16 PST
Comment on attachment 450162 [details]
Patch

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

> Source/WebKit/Shared/WebFindOptions.cpp:31
> +WebCore::FindOptions core(OptionSet<FindOptions> options)

Nit - should probably either add a few more WebCore:: in this function, or add a `using namespace WebCore;` instead. I think right now it might be getting the missing WebCore:: from earlier unified sources.

> Source/WebKit/Shared/WebFindOptions.h:53
> +WebCore::FindOptions core(OptionSet<FindOptions>);

(Here too)

> Source/WebKit/Shared/WebFoundTextRange.cpp:49
> +bool WebFoundTextRange::decode(IPC::Decoder& decoder, WebFoundTextRange& result)

Nit - I think we generally prefer to use the Optional<>-type decode method in new code (this older style is considered "legacy").

> Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.h:51
> +    explicit WebFoundTextRangeController(WebPage*);

Do we expect `WebFoundTextRangeController` to be initialized with a WebPage* that is `nullptr`? (If not, let's make this take a WebPage& instead).
Comment 4 Aditya Keerthi 2022-01-27 14:17:55 PST
Created attachment 450177 [details]
Patch
Comment 5 Aditya Keerthi 2022-01-27 14:19:31 PST
(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 450162 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450162&action=review
> 
> > Source/WebKit/Shared/WebFindOptions.cpp:31
> > +WebCore::FindOptions core(OptionSet<FindOptions> options)
> 
> Nit - should probably either add a few more WebCore:: in this function, or
> add a `using namespace WebCore;` instead. I think right now it might be
> getting the missing WebCore:: from earlier unified sources.

I see any missing WebCore:: here, the parameter is in the WebKit namespace.
 
> > Source/WebKit/Shared/WebFindOptions.h:53
> > +WebCore::FindOptions core(OptionSet<FindOptions>);
> 
> (Here too)

See above.
 
> > Source/WebKit/Shared/WebFoundTextRange.cpp:49
> > +bool WebFoundTextRange::decode(IPC::Decoder& decoder, WebFoundTextRange& result)
> 
> Nit - I think we generally prefer to use the Optional<>-type decode method
> in new code (this older style is considered "legacy").

Done.
 
> > Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.h:51
> > +    explicit WebFoundTextRangeController(WebPage*);
> 
> Do we expect `WebFoundTextRangeController` to be initialized with a WebPage*
> that is `nullptr`? (If not, let's make this take a WebPage& instead).

Done.
Comment 6 Wenson Hsieh 2022-01-27 14:25:49 PST
Comment on attachment 450177 [details]
Patch

r=mews

(In reply to Aditya Keerthi from comment #5)
> (In reply to Wenson Hsieh from comment #3)
> > Comment on attachment 450162 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=450162&action=review
> > 
> > > Source/WebKit/Shared/WebFindOptions.cpp:31
> > > +WebCore::FindOptions core(OptionSet<FindOptions> options)
> > 
> > Nit - should probably either add a few more WebCore:: in this function, or
> > add a `using namespace WebCore;` instead. I think right now it might be
> > getting the missing WebCore:: from earlier unified sources.
> 
> I see any missing WebCore:: here, the parameter is in the WebKit namespace.

LGTM — I missed that these were different types altogether.
Comment 7 Aditya Keerthi 2022-01-27 14:29:22 PST
(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 450177 [details]
> Patch
> 
> r=mews
> 
> (In reply to Aditya Keerthi from comment #5)
> > (In reply to Wenson Hsieh from comment #3)
> > > Comment on attachment 450162 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=450162&action=review
> > > 
> > > > Source/WebKit/Shared/WebFindOptions.cpp:31
> > > > +WebCore::FindOptions core(OptionSet<FindOptions> options)
> > > 
> > > Nit - should probably either add a few more WebCore:: in this function, or
> > > add a `using namespace WebCore;` instead. I think right now it might be
> > > getting the missing WebCore:: from earlier unified sources.
> > 
> > I see any missing WebCore:: here, the parameter is in the WebKit namespace.
> 
> LGTM — I missed that these were different types altogether.

It's certainly confusing because the enum has the same name in both namespaces.

Filed https://bugs.webkit.org/show_bug.cgi?id=235735 to fix this in a follow-up.
Comment 8 Aditya Keerthi 2022-01-27 15:37:03 PST
Created attachment 450191 [details]
Patch for landing
Comment 9 Aditya Keerthi 2022-01-27 15:38:25 PST
Comment on attachment 450191 [details]
Patch for landing

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

> Source/WebKit/Shared/WebPageCreationParameters.cpp:340
> +    std::optional<std::optional<WebCore::DestinationColorSpace>> colorSpace;

Needed to fix unified-source builds.
Comment 10 EWS 2022-01-27 23:32:53 PST
Committed r288732 (246525@main): <https://commits.webkit.org/246525@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450191 [details].