Bug 39157 – FrameLoader: refactor FrameLoader::isDocumentSandboxed() to be a non-member, non-friend function

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

[Chris Jerdonek]

Refactor FrameLoader's isDocumentSandboxed() function to be a non-member, non-friend helper function.  This is one step towards bug 39156 (refactoring createWindow() to be non-member, non-friend).

[Chris Jerdonek]

Created attachment 56179 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 56180 [details]
Proposed patch

[Adam Barth]

Comment on attachment 56180 [details]
Proposed patch

This looks great.  One nit:

WebCore/loader/FrameLoader.cpp:178
 +  static bool isDocumentSandboxed(const Frame& frame, SandboxFlags mask)
We pass Frame by Frame* and not by const Frame&.  It's probably better to be consistent about that.

[Chris Jerdonek]

Thanks, Adam.

(In reply to comment #3)
> WebCore/loader/FrameLoader.cpp:178
>  +  static bool isDocumentSandboxed(const Frame& frame, SandboxFlags mask)
> We pass Frame by Frame* and not by const Frame&.  It's probably better to be consistent about that.

I thought it would be preferable given that the function assumes a non-null pointer, but I'm also okay with being consistent.  Do you know if there is a reason to prefer passing by Frame* (other than current convention), or was that an arbitrary historical decision?

[Adam Barth]

We use Frame* in a bunch of places where the point is non-null.

The difference is whether the type is a "value" type or a "pointer" type.  Value types are allocated on the stack and passed by const reference.  Pointer types are allocated on the heap and passed by pointer.  It's fairly common in C++ code in many projects to distinguish between these two kinds of types.  In this case, Frames are allocated on the heap (and have their lifetime managed with reference counters), so we pass it by pointer to reminder ourselves of that.

[Chris Jerdonek]

Created attachment 56225 [details]
Proposed patch 2

[Darin Adler]

Comment on attachment 56225 [details]
Proposed patch 2

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.

[Chris Jerdonek]

(In reply to comment #7)

Thanks a lot for your comments.  I have a couple responses I hope you can
look at.

> 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.

I had four reasons for doing this on its own merits -- some stronger than
others.  Moving createWindow() only answers the "why now" (because I had to
touch the declaration of isDocumentSandboxed() anyways since it is private).

The isDocumentSandboxed() function doesn't need access to private
FrameLoader data.  It's essentially a helper function.  Making it
non-member, non-friend makes this clearer.

Making it non-member doesn't increase the surface area of the API that can
access private data (i.e. it doesn't decrease encapsulation).  Since
they'll need to make it a member before doing so, future developers will
be more likely to pause and think before modifying the function to make it
depend on private data.

By making it non-member, we can avoid declaring it in the header file.
This is because it is only needed from within the .cpp file.

By removing it from the header file, we are simplifying the FrameLoader.h
API for consumers.  It's true that callers can code their own version of
isDocumentSandboxed() (since it is a combination of public method calls).
But by removing it from the header file we are indirectly telling consumers
they should not need to call this particular function.

> > +// 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".

Thanks for the info.  Do you mean "non-const" data, since a Frame contains
a const FrameLoader which itself seems to carry a lot of state. 

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

Thanks.  Beginner mistake.

[Darin Adler]

(In reply to comment #8)
> (In reply to comment #7)
> I had four reasons for doing this on its own merits -- some stronger than
> others.  Moving createWindow() only answers the "why now" (because I had to
> touch the declaration of isDocumentSandboxed() anyways since it is private).

That makes more sense.

> The isDocumentSandboxed() function doesn't need access to private
> FrameLoader data.  It's essentially a helper function.  Making it
> non-member, non-friend makes this clearer.
> 
> Making it non-member doesn't increase the surface area of the API that can
> access private data (i.e. it doesn't decrease encapsulation).  Since
> they'll need to make it a member before doing so, future developers will
> be more likely to pause and think before modifying the function to make it
> depend on private data.
> 
> By making it non-member, we can avoid declaring it in the header file.
> This is because it is only needed from within the .cpp file.
> 
> By removing it from the header file, we are simplifying the FrameLoader.h
> API for consumers.  It's true that callers can code their own version of
> isDocumentSandboxed() (since it is a combination of public method calls).
> But by removing it from the header file we are indirectly telling consumers
> they should not need to call this particular function.

Sure, those reasons seem fine.

> > Generally speaking we always use the type "Frame*" rather than "const Frame*" because Frame itself has little data and no real notion of "const".
> 
> Thanks for the info.  Do you mean "non-const" data, since a Frame contains
> a const FrameLoader which itself seems to carry a lot of state. 

The fact that Frame contains the FrameLoader is a hidden implementation detail. Conceptually it simply has a pointer to the frame loader.

Generally speaking for an object like a Frame that represents the entire frame and its contents, we are not distinguishing const member functions from non-const. If you grep for "const Frame*" in .cpp files you find about 9 places that are doing it out of about 1571 places that are using Frame*. Please just use Frame* and eschew const for now.

[Chris Jerdonek]

Created attachment 56381 [details]
Proposed patch 3

[Chris Jerdonek]

Comment on attachment 56381 [details]
Proposed patch 3

Clearing flags on attachment: 56381

Committed r59670: <http://trac.webkit.org/changeset/59670>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.