[webkit-reviews] review granted: [Bug 30155] Factor LoaderPolicy out of FrameLoader : [Attachment 40774] LoaderPolicy

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 30155: Factor LoaderPolicy out of FrameLoader
https://bugs.webkit.org/show_bug.cgi?id=30155

Attachment 40774: LoaderPolicy
https://bugs.webkit.org/attachment.cgi?id=40774&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.