Bug 235691 – Introduce WebFoundTextRange and WebFoundTextRangeController to support restorable find results

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aditya Keerthi]

...

[Aditya Keerthi]

rdar://88117167

[Aditya Keerthi]

Created attachment 450162 [details]
Patch

[Wenson Hsieh]

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).

[Aditya Keerthi]

Created attachment 450177 [details]
Patch

[Aditya Keerthi]

(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.

[Wenson Hsieh]

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.

[Aditya Keerthi]

(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.

[Aditya Keerthi]

Created attachment 450191 [details]
Patch for landing

[Aditya Keerthi]

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.

[EWS]

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].