[webkit-reviews] review denied: [Bug 39157] FrameLoader: refactor FrameLoader::isDocumentSandboxed() to be a non-member, non-friend function : [Attachment 56225] Proposed patch 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 39157: FrameLoader: refactor FrameLoader::isDocumentSandboxed() to be a
non-member, non-friend function
https://bugs.webkit.org/show_bug.cgi?id=39157

Attachment 56225: Proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=56225&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I don't understand why having this be a non-member is important to having
createWindow be a non-member. A public member function can be called by any
function.

The decision of member function vs. non-member function should be decided based
on clarity at the call site among other things. A function does not have to be
a non-member to be public. Moving createWindow out of the FrameLoader class
should not be tied to moving this, and moving this should be decided on its own
merits.

> +// FIXME: isDocumentSandboxed should eventually replace isSandboxed.
> +static bool isDocumentSandboxed(const Frame* const frame, SandboxFlags mask)


It's not helpful to give this the type "const Frame* const".

Generally speaking we always use the type "Frame*" rather than "const Frame*"
because Frame itself has little data and no real notion of "const". There's no
point in trying to start adding that now unless there's some major benefit.

And putting the const in after the "*" is marking the argument "const" which
has no effect and should be avoided. It's like this:

    double squareRoot(double);
    const double squareRoot(const double);

Those functions are nearly identical and we would always use the first.