[webkit-reviews] review granted: [Bug 89788] [chromium] Introduce way to reload a page using the original request URL : [Attachment 149827] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted dfalcantara at chromium.org's request
for review:
Bug 89788: [chromium] Introduce way to reload a page using the original request
URL
https://bugs.webkit.org/show_bug.cgi?id=89788

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

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


If I were writing this patch, I would include a simple API test that showed
that reloading a WebFrame with an override URL preserved some state about the
page that you cared about (like the scroll position) but ended up at a
different URL.	Note: It's not important for the test to involve a
redirect---it just needs two URLs.

> Source/WebCore/loader/FrameLoader.cpp:1446
> +    // If the URL is empty, don't bother with reloading.

I would remove this comment because it just states what the code does.

> Source/WebCore/loader/FrameLoader.cpp:1452
> +    ResourceRequest initialRequest = m_documentLoader->request();
> +    initialRequest.setURL(overrideUrl);
> +    reloadInitialRequest(initialRequest, endToEndReload);

nit: I would just call initialRequest "request".  The word "initial" doesn't
really mean much here.

> Source/WebCore/loader/FrameLoader.cpp:1478
> +    if (!m_documentLoader)
> +	   return;
> +

Given that this is a private method, we don't need to re-check m_documentLoader
for being 0.  If you like, you can ASSERT that it's non-0 instead.

> Source/WebCore/loader/FrameLoader.h:118
> +    void reloadWithOverrideURL(const KURL& overrideUrl, bool
endToEndReload);

Should endToEndReload default to false, like for reload() ?

> Source/WebCore/loader/FrameLoader.h:348
> +    void reloadInitialRequest(const ResourceRequest&, bool endToEndReload);

Maybe reloadInitialRequest -> reloadWithRequest ?  Again, there's nothing
particularly "initial" about the request at this point.

> Source/WebKit/chromium/public/WebFrame.h:323
> +    // Reload the current document, but change the URL to the given one
first.
> +    // This is used for situations where we need to avoid a redirect.

I might say something like:

// This is used for situations where we want to reload a different URL because
of a redirect.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:981
> +void WebFrameImpl::reloadWithOverrideURL(const WebURL& overrideUrl, bool
endToEndReload)

It's slightly odd that the second parameter changed names.