| Summary: | Remove trival "forward-to-client" member functions from FrameLoader | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ap, cmarcelo, darin, eric, japhet | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 29947 | ||||||
| Attachments: |
|
||||||
|
Description
Adam Barth
2011-06-12 03:42:26 PDT
Created attachment 96874 [details]
Patch
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. (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. +ap. Replies to your comments above. Thanks for the review. Committed r88618: <http://trac.webkit.org/changeset/88618> |