[Webkit-unassigned] [Bug 30155] Factor LoaderPolicy out of FrameLoader

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30155





--- Comment #4 from Adam Barth <abarth at webkit.org>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.