[webkit-reviews] review denied: [Bug 32372] Unify origin sandbox flag with m_noAccess in SecurityOrigin : [Attachment 46201] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 32372: Unify origin sandbox flag with m_noAccess in SecurityOrigin
https://bugs.webkit.org/show_bug.cgi?id=32372

Attachment 46201: Patch
https://bugs.webkit.org/attachment.cgi?id=46201&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // This method exists because we treat data URLs as havine a unique
origin,

Typo: "havine".

> +    // contrary to the current (9/19/2009) draft of the HTML5 specification.

> +    // We still want to let folks paint data URLs onto untainted canvases,
so
> +    // we special case data URLs below.  If we change to match HTML5 w.r.t.
> +    // data URL security, then we can remove this method in favor of
> +    // !canRequest.

This is a member function, and C++ does not have methods, so I suggest we call
it a "function" in this comment.

>      if (url.protocolIs("data"))
>	   return false;
>  
> +void SecurityOrigin::setSandboxFlags(SandboxFlags flags)
> +{
> +    m_sandboxFlags = flags;
> +    if (isSandboxed(SandboxOrigin))
> +	   m_isUnique = true;
> +}

This can correctly handle tightening of sandbox flags, but not loosening of
them. At the very least we should be asserting that the origin restriction is
never loosened here, because we then would have to set m_isUnique to
shouldTreatURLSchemeAsNoAccess(m_protocol).

> -    // Sandboxing status as determined by the frame.

Why did you remove this comment. I assume you had a good reason, but I don't
know what it is.

> -    void setSandboxFlags(SandboxFlags flags) { m_sandboxFlags = flags; }
> +    void setSandboxFlags(SandboxFlags flags);

I suggest removing the argument name here.

> +    // The origin is a globally unique identifier assigned when the Document
is
> +    // created. 
http://www.whatwg.org/specs/web-apps/current-work/#sandboxOrigin
> +    //
> +    // There's a subtle difference between a unique origin and an origin
that
> +    // has the SandboxOrigin flag set.  The later implies the former, and,
in
> +    // addition, the SandboxOrigin flag is inherited by iframes.
> +    bool isUnique() const { return m_isUnique; }
> +

Periods, not two.

I think you mean "the latter" rather than "the later".

> +    // When using the string value, it's important to remember that it might
be
> +    // "null".  This happens when this SecurityOrigin is unique For example,


Missing period after "unique".

> -    bool m_noAccess;
> -    bool m_universalAccess;
> -    bool m_domainWasSetInDOM;
> -    bool m_canLoadLocalResources;
> +    bool m_isUnique : 1;
> +    bool m_universalAccess : 1;
> +    bool m_domainWasSetInDOM : 1;
> +    bool m_canLoadLocalResources : 1;

Why the bit fields here? Are there enough SecurityOrigin objects that this has
a significant memory cost we want to reduce? Does this make code smaller or
faster? I think we don't have good a reason to use them here.

review- because I'm sure you'll want to fix some of these