[webkit-reviews] review denied: [Bug 175759] Introduce Storage Access API (document parts) as an experimental feature : [Attachment 318645] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 175759: Introduce Storage Access API (document parts) as an experimental
feature
https://bugs.webkit.org/show_bug.cgi?id=175759

Attachment 318645: Patch

https://bugs.webkit.org/attachment.cgi?id=318645&action=review




--- Comment #11 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 318645
  --> https://bugs.webkit.org/attachment.cgi?id=318645
Patch

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

This is a great start, but let's handle the experimental feature the right way
from the start. r- to correct that. :-)

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:157
> +ENABLE_STORAGE_ACCESS_API = ENABLE_STORAGE_ACCESS_API;

I don't think we want to add more "compile-time" guards like this for
experimental features. We want to expose them as runtime-configurable switches
we can control through the User Agent application (e.g., Safari's Developer
menu). This doesn't seem like a feature that requires special back-end support
or other code that needs to be compiled out for WebKit ports that are missing
some critical platform feature. I suggest getting rid of this feature
definition entirely.

> Source/WebCore/dom/Document.cpp:5014
> +    String returnValue;

This doesn't seem to be used.

> Source/WebCore/dom/Document.idl:125
> +    [Conditional=STORAGE_ACCESS_API] boolean requestStorageAccess();

As Andy pointed out in his review, this should be
"EnabledBySetting=StorageAccessAPI". Then update
"WebKit/Shared/WebPreferencesDefinition.h" and add "StorageAccessAPI" to the
FOR_EACH_WEBKIT_EXPERIMENTAL_FEATURE_PREFERENCE macro. Take a look at
SubresourceIntegrity/SubresourceIntegrityEnabled to see the right way to do
this.

> Source/WebCore/dom/SecurityContext.cpp:89
> +	   "allow-forms", "allow-same-origin", "allow-scripts",
"allow-top-navigation", "allow-pointer-lock", "allow-popups",
"allow-popups-to-escape-sandbox", "allow-top-navigation-by-user-activation",
"allow-storage-access-by-user-activation"

You could just add the conditional inside the array declaration here. But as I
argue earlier, I don't think it should be conditional at all.

We should check if the feature is turned on, and just ignore the policy if we
don't have it active.