Bug 30155 – Factor PolicyChecker out of FrameLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Adam Barth]

The job of this class is to manage the policy callbacks.

[Adam Barth]

Created attachment 40770 [details]
LoaderPolicy

[Adam Barth]

Created attachment 40774 [details]
LoaderPolicy

[Darin Adler]

Comment on attachment 40774 [details]
LoaderPolicy

It's great to do this.

I'm don't think the object should be named "loader policy", because the object is not a policy. I think it's a "policy xxx" where "xxx" needs to be some other noun. PolicyChecker perhaps? I know it's elegant to have this have a single word name, "policy", but I don't think it makes enough logical sense.

> +    // Is the activeDocumentLoader always us?
> +    ASSERT(frameLoader()->activeDocumentLoader());

What is "us" here? This is the main resource loader, not a document loader. I suppose you mean the document loader that's asking for this main resource load, so maybe I'm just being too picky.

> +        // FIXME: Seriously?  This is why we can't have nice things.

This code was definitely controversial when first created. I don't want to rain on the fun, but I don't think the comment adds much. A more sober comment might carry some information that could help future programmers. It's obvious to you what's wrong, but might not be to them. Your point is that loading a document should be possible without having to create a Frame and a FrameView, I suppose. Or perhaps without so much fakery and so many function calls? As I said, the point should be obvious, but it's not, even to me!

I'm going to say review+ but I have serious reservations about the name of this new object, and I'm not entirely sure I understand the boundaries of what does and does not go in the object.

[Adam Barth]

> I'm don't think the object should be named "loader policy", because the object
> is not a policy. I think it's a "policy xxx" where "xxx" needs to be some other
> noun. PolicyChecker perhaps? I know it's elegant to have this have a single
> word name, "policy", but I don't think it makes enough logical sense.

I was tempted to use PolicyChecker too, but there's already an object named PolicyCheck.  Maybe I should rename PolicyCheck to PolicyCallback?

> > +    // Is the activeDocumentLoader always us?
> > +    ASSERT(frameLoader()->activeDocumentLoader());
> 
> What is "us" here? This is the main resource loader, not a document loader. I
> suppose you mean the document loader that's asking for this main resource load,
> so maybe I'm just being too picky.

I'll remove this comment.  I was wondering if there was a more direct way to get to this object than via the frame loader.

> > +        // FIXME: Seriously?  This is why we can't have nice things.
> 
> This code was definitely controversial when first created. I don't want to rain
> on the fun, but I don't think the comment adds much.

Yeah, I'll remove this comment.   We might have to do something abou that code eventually, but the comment isn't helping the situation.

> A more sober comment might
> carry some information that could help future programmers. It's obvious to you
> what's wrong, but might not be to them. Your point is that loading a document
> should be possible without having to create a Frame and a FrameView, I suppose.
> Or perhaps without so much fakery and so many function calls? As I said, the
> point should be obvious, but it's not, even to me!

The problem with that code is a lot of downstream code in the loader has to be aware that it might be living in a fake page / frame.   For example, when we detect mixed content, we send a notification to the FrameLoaderClient that we loaded some insecure content.  However, if we do that in the fake page, no one is listening...

> I'm going to say review+ but I have serious reservations about the name of this
> new object, and I'm not entirely sure I understand the boundaries of what does
> and does not go in the object.

This object is in charge of managing the state associated with the FrameLoaderClient::*decide* methods.  So, it knows which decisions are in flight and what kind of decisions they are.  It can also mutate this state, for example by stopping or canceling them.  I'd like to make the API slightly tighter.  You'll notice that most of the call sites make a series of calls into m_policy.  Ideally, they'd make one call that encapsulates what policy they'd like to check.

[Adam Barth]

Created attachment 40814 [details]
Updated patch

[Adam Barth]

My plan is to land the above patch, which incorporates Darin's comments.  Let me know if you'd like another chance to comment.

[Adam Barth]

Committed r49284: <http://trac.webkit.org/changeset/49284>