Bug 62510 - Remove trival "forward-to-client" member functions from FrameLoader
Summary: Remove trival "forward-to-client" member functions from FrameLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 29947
  Show dependency treegraph
 
Reported: 2011-06-12 03:42 PDT by Adam Barth
Modified: 2011-06-12 12:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (28.81 KB, patch)
2011-06-12 03:46 PDT, Adam Barth
ap: review+
abarth: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-06-12 03:42:26 PDT
Remove trival "forward-to-client" member functions from FrameLoader
Comment 1 Adam Barth 2011-06-12 03:46:58 PDT
Created attachment 96874 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-06-12 11:26:19 PDT
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.
Comment 3 Adam Barth 2011-06-12 12:17:48 PDT
(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.
Comment 4 Adam Barth 2011-06-12 12:18:13 PDT
+ap.

Replies to your comments above.  Thanks for the review.
Comment 5 Adam Barth 2011-06-12 12:21:55 PDT
Committed r88618: <http://trac.webkit.org/changeset/88618>