Bug 62510 – Remove trival "forward-to-client" member functions from FrameLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove trival "forward-to-client" member functions from FrameLoader

[Adam Barth]

Created attachment 96874 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 96874 [details]
Patch

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.

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 96874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96874&action=review
> 
> > 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?

Actually, I wonder if interruptedForPolicyChangeError to match the tense of the other functions.  I'll post a followup patch that does this renaming since this patch is big enough as is.

It's likely we should inline this function as well, but I didn't study it.

> > 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.

Will do.

> > 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.

Yes, it does seems strange to round-trip through WebCore.  I didn't investigate the cause.

[Adam Barth]

+ap.

Replies to your comments above.  Thanks for the review.

[Adam Barth]

Committed r88618: <http://trac.webkit.org/changeset/88618>