[webkit-reviews] review granted: [Bug 102151] Consolidate FrameLoader::load() into one function taking a FrameLoadRequest : [Attachment 174552] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted James Simonsen
<simonjam at chromium.org>'s request for review:
Bug 102151: Consolidate FrameLoader::load() into one function taking a
FrameLoadRequest
https://bugs.webkit.org/show_bug.cgi?id=102151

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174552&action=review


> Source/WebCore/loader/FrameLoadRequest.h:30
> +#include "Document.h"
> +#include "Frame.h"

These are some large headers.  Do we really need to include them, or can we
forward declare the classes?

> Source/WebCore/loader/FrameLoadRequest.h:69
> +    FrameLoadRequest(Frame* frame, const ResourceRequest& resourceRequest)
> +	   : m_requester(frame->document()->securityOrigin())
> +	   , m_resourceRequest(resourceRequest)
> +	   , m_lockHistory(false)
> +	   , m_shouldCheckNewWindowPolicy(false)
>      {
>      }

Maybe we should move this constructor out of line?

> Source/WebKit/blackberry/Api/WebPage.cpp:753
> +    frameRequest.setSubstituteData(substituteData);

I wonder if it would be worth adding an optional SubstituteData parameter to
the above given that you've got this pattern in a bunch of places.

> Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:137
> +    request.setSubstituteData(SubstituteData(buf, String("text/html"),
String("UTF-8"), KURL()));

There's no need to call the String constructor explicitly.  The compiler should
let you call it implicitly.

Also, I would probably rename buf to buffer and len to length.	This is really
a lot of work to load the empty string!

> Source/WebKit/mac/Plugins/WebPluginController.mm:407
> +	   frameRequest.setShouldCheckNewWindowPolicy(true);

I like that we don't have these mysterious "false" parameters anymore.