...
rdar://88117167
Created attachment 450162 [details] Patch
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).
Created attachment 450177 [details] Patch
(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 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.
(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.
Created attachment 450191 [details] Patch for landing
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.
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].