Bug 30155 - Factor PolicyChecker out of FrameLoader
Summary: Factor PolicyChecker out of FrameLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 29947
  Show dependency treegraph
 
Reported: 2009-10-07 01:08 PDT by Adam Barth
Modified: 2009-10-07 20:17 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-10-07 01:08:38 PDT
The job of this class is to manage the policy callbacks.
Comment 1 Adam Barth 2009-10-07 01:15:33 PDT
Created attachment 40770 [details]
LoaderPolicy
Comment 2 Adam Barth 2009-10-07 01:55:18 PDT
Created attachment 40774 [details]
LoaderPolicy
Comment 3 Darin Adler 2009-10-07 09:54:47 PDT
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.
Comment 4 Adam Barth 2009-10-07 10:07:42 PDT
> 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 Adam Barth 2009-10-07 12:50:25 PDT
Created attachment 40814 [details]
Updated patch
Comment 6 Adam Barth 2009-10-07 13:04:27 PDT
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 Adam Barth 2009-10-07 20:17:23 PDT
Committed r49284: <http://trac.webkit.org/changeset/49284>