[webkit-reviews] review granted: [Bug 62510] Remove trival "forward-to-client" member functions from FrameLoader : [Attachment 96874] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Adam Barth
<abarth at webkit.org>'s request for review:
Bug 62510: Remove trival "forward-to-client" member functions from FrameLoader
https://bugs.webkit.org/show_bug.cgi?id=62510

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96874&action=review

I have mixed feelings about this patch. On one hand, in most cases we are
logically talking to the loader, and the client doesn't need to know that said
loader even has a "client". Exposing additional entities to more code is not
good.

On the other hand, I think that knowing that there is not other logic besides
calling the client improves readability in practice.

> Source/WebCore/loader/MainResourceLoader.cpp:121
> -ResourceError MainResourceLoader::interruptionForPolicyChangeError() const
> +ResourceError MainResourceLoader::interruptForPolicyChangeError() const

Wouldn't it be better to use the "interruption" name everywhere instead? Also,
do we need this function, given that we always just make a client call in
place?

> Source/WebCore/loader/PingLoader.cpp:118
> +    // FIXME: Why activeDocumentLoader? I would have expected
documentLoader()...

It would be helpful if you could add a few words about why you expected that.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:49
> -    return frame()->loader()->blockedError(request);
> +    return frame()->loader()->client()->blockedError(request);

I wonder why we are even creating the error via WebCore.