Bug 30155 - Factor PolicyChecker out of FrameLoader
: Factor PolicyChecker out of FrameLoader
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 29947
  Show dependency treegraph
 
Reported: 2009-10-07 01:08 PST by
Modified: 2009-10-07 20:17 PST (History)


Attachments
LoaderPolicy (49.35 KB, patch)
2009-10-07 01:15 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
LoaderPolicy (49.55 KB, patch)
2009-10-07 01:55 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (49.70 KB, patch)
2009-10-07 12:50 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-07 01:08:38 PST
The job of this class is to manage the policy callbacks.
------- Comment #1 From 2009-10-07 01:15:33 PST -------
Created an attachment (id=40770) [details]
LoaderPolicy
------- Comment #2 From 2009-10-07 01:55:18 PST -------
Created an attachment (id=40774) [details]
LoaderPolicy
------- Comment #3 From 2009-10-07 09:54:47 PST -------
(From update of attachment 40774 [details])
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.
------- Comment #4 From 2009-10-07 10:07:42 PST -------
> 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.
------- Comment #5 From 2009-10-07 12:50:25 PST -------
Created an attachment (id=40814) [details]
Updated patch
------- Comment #6 From 2009-10-07 13:04:27 PST -------
My plan is to land the above patch, which incorporates Darin's comments.  Let me know if you'd like another chance to comment.
------- Comment #7 From 2009-10-07 20:17:23 PST -------
Committed r49284: <http://trac.webkit.org/changeset/49284>