Bug 62510

Summary: Remove trival "forward-to-client" member functions from FrameLoader
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: 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 Flags
Patch ap: review+, abarth: commit-queue?

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>