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

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> 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 319029: Patch

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




--- Comment #18 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 319029
  --> https://bugs.webkit.org/attachment.cgi?id=319029
Patch

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

> Source/WebCore/dom/Document.cpp:4996
> +    if (m_frame->isMainFrame()) {

Nothing guarantees the document has a frame.

> Source/WebCore/dom/Document.cpp:5004
> +    SecurityOrigin& thisSecurityOrigin = securityOrigin();

I think we should just call this securityOrigin.

> Source/WebCore/dom/Document.cpp:5005
> +    SecurityOrigin& topSecurityOrigin =
m_frame->tree().top().document()->securityOrigin();

topDocument().securityOrigin();

> Source/WebCore/dom/Document.cpp:5021
> +    if ((page && page->chrome().runJavaScriptConfirm(*m_frame,
builder.toString())) || m_grantStorageAccessOverride) {

What happens if requestStorageAccess() is called by the JS in a loop?
Do we usually ask for permission from the user in this fashion? What about
localization?

> Source/WebCore/dom/Document.cpp:7134
> +bool Document::isSandboxedIframe() const

I don't think this should be on document.

> Source/WebCore/dom/Document.idl:124
> +    [EnabledBySetting=StorageAccessAPI] readonly attribute boolean
hasStorageAccess;

FYI, you are going to have to use EnabledAtRuntime instead of EnabledBySetting
if you plan to expose things to workers.

Also, is this API specified somewhere? "hasStorageAccess" is super generic and
does not really tell me what this is. Same comment for the method below.

> Source/WebCore/html/HTMLFrameOwnerElement.h:57
> +    bool isSandboxedIframe() const { return m_isSandboxedIframe; }

I am not convinced we need those on the frame owner element. Why isn't
sandboxFlags() != SandboxNone sufficient? Also, it is pretty uncommon to not
check for a specific sandbox flag.

> Source/WebCore/html/HTMLIFrameElement.cpp:96
> +	   setIsSandboxedIframe(true);

This looks fishy.