Bug 222377 – [iOS] Get default value in the UI process for whether synchronous XMLHttpRequest are allowed during unload

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Getting this default value in the UI process instead of in every WebContent process should be a small speedup, since getting this default value is a bit costly on iOS.

[Per Arne Vollan]

Created attachment 421443 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/74709239>

[Per Arne Vollan]

Created attachment 421444 [details]
Patch

[Per Arne Vollan]

Created attachment 421446 [details]
Patch

[Per Arne Vollan]

Created attachment 421450 [details]
Patch

[Per Arne Vollan]

Created attachment 421452 [details]
Patch

[Brent Fulgham]

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?

[Per Arne Vollan]

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

[EWS]

Committed r273452: <https://commits.webkit.org/r273452>

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

[Simon Fraser (smfr)]

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?

[Per Arne Vollan]

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