Summary: | [iOS] Get default value in the UI process for whether synchronous XMLHttpRequest are allowed during unload | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, dean_johnson, simon.fraser, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2021-02-24 12:46:14 PST
Created attachment 421443 [details]
Patch
Created attachment 421444 [details]
Patch
Created attachment 421446 [details]
Patch
Created attachment 421450 [details]
Patch
Created attachment 421452 [details]
Patch
Comment on attachment 421452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421452&action=review r=me > Source/WebKit/UIProcess/WebPageProxy.cpp:7988 > +#if PLATFORM(IOS) Should this check #if !PLATFORM(MACCATALYST) && !PLATFORM(WATCHOS) as well? (In reply to Brent Fulgham from comment #7) > Comment on attachment 421452 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421452&action=review > > r=me > > > Source/WebKit/UIProcess/WebPageProxy.cpp:7988 > > +#if PLATFORM(IOS) > > Should this check #if !PLATFORM(MACCATALYST) && !PLATFORM(WATCHOS) as well? That is a good point. I think we even could create a HAVE(MANAGED_CONFIGURATION) define, to avoid these platform defines. I can create a follow-up patch for that. What do you think? Thanks for reviewing! Committed r273452: <https://commits.webkit.org/r273452> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421452 [details]. Comment on attachment 421452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421452&action=review > Source/WebKit/Shared/ios/WebPreferencesDefaultValuesIOS.mm:49 > + static NeverDestroyed<Optional<bool>> allowsRequest; Why isn't this just static bool value = [[PAL::getMCProfileConnectionClass() sharedConnection] effectiveBoolValueForSetting:@"allowDeprecatedWebKitSynchronousXHRLoads"] == MCRestrictedBoolExplicitYes; in the function below? > Source/WebKit/Shared/ios/WebPreferencesDefaultValuesIOS.mm:62 > + cachedAllowsRequest() = allowsRequest; This reads oddly. >>> Source/WebKit/UIProcess/WebPageProxy.cpp:7988 >>> +#if PLATFORM(IOS) >> >> Should this check #if !PLATFORM(MACCATALYST) && !PLATFORM(WATCHOS) as well? > > That is a good point. I think we even could create a HAVE(MANAGED_CONFIGURATION) define, to avoid these platform defines. I can create a follow-up patch for that. What do you think? > > Thanks for reviewing! Why not just not #ifdef this. It's fine if it's false by default. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:642 > +#if PLATFORM(IOS) > + setAllowsDeprecatedSynchronousXMLHttpRequestDuringUnload(parameters.allowsDeprecatedSynchronousXMLHttpRequestDuringUnload); > +#endif This pollutes the WebPage ctor with random stuff. Why is this here and not a bunch of other config stuff? (In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 421452 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421452&action=review > > > Source/WebKit/Shared/ios/WebPreferencesDefaultValuesIOS.mm:49 > > + static NeverDestroyed<Optional<bool>> allowsRequest; > > Why isn't this just static bool value = [[PAL::getMCProfileConnectionClass() > sharedConnection] > effectiveBoolValueForSetting:@"allowDeprecatedWebKitSynchronousXHRLoads"] == > MCRestrictedBoolExplicitYes; in the function below? > We would like to avoid all Managed Configuration calls in WP, since loading the library can then be avoided. > > Source/WebKit/Shared/ios/WebPreferencesDefaultValuesIOS.mm:62 > > + cachedAllowsRequest() = allowsRequest; > > This reads oddly. > Yes, I can look into a better way of doing this. > >>> Source/WebKit/UIProcess/WebPageProxy.cpp:7988 > >>> +#if PLATFORM(IOS) > >> > >> Should this check #if !PLATFORM(MACCATALYST) && !PLATFORM(WATCHOS) as well? > > > > That is a good point. I think we even could create a HAVE(MANAGED_CONFIGURATION) define, to avoid these platform defines. I can create a follow-up patch for that. What do you think? > > > > Thanks for reviewing! > > Why not just not #ifdef this. It's fine if it's false by default. > Yes, I think we could do that. Alternatively, we could add a HAVE(MANAGED_CONFIGURATION) define, since this only applies for platforms using this feature. This would also remove some of the other platform defines related to this. > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:642 > > +#if PLATFORM(IOS) > > + setAllowsDeprecatedSynchronousXMLHttpRequestDuringUnload(parameters.allowsDeprecatedSynchronousXMLHttpRequestDuringUnload); > > +#endif > > This pollutes the WebPage ctor with random stuff. Why is this here and not a > bunch of other config stuff? That's a good point. It would be better to pass this as part of the Web process creation parameters, I think. I will create a follow-up patch addressing these issues. Thanks for reviewing! |