| Summary: | Remove NetworkResourceLoaderClient and subclasses. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
| Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, beidson, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Antti Koivisto
2014-08-29 07:28:03 PDT
Created attachment 237349 [details]
patch
Created attachment 237350 [details]
fix cmake build
Comment on attachment 237350 [details] fix cmake build View in context: https://bugs.webkit.org/attachment.cgi?id=237350&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:388 > +#endif // __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 I don’t think this comment is helpful. It’s gotten out of sync with the #if, which is one of the main reasons I don’t like comments like this, and it’s only a few lines away from the #if. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:174 > + struct SynchronousLoadData { > + SynchronousLoadData(WebCore::ResourceRequest& request, PassRefPtr<Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply> reply) > + : m_originalRequest(request) > + , m_delayedReply(reply) > + { > + ASSERT(m_delayedReply); > + } > + WebCore::ResourceRequest m_originalRequest; > + WebCore::ResourceRequest m_currentRequest; > + RefPtr<Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply> m_delayedReply; > + WebCore::ResourceResponse m_response; > + WebCore::ResourceError m_error; > + }; I don’t think we need to put the definition of this struct in the header. We can compile references to this struct and unique_ptr to this struct without having the entire class in the header. And it would be nice to not have to include ResourceError.h and ResourceResponse.h here. Can we forward-declare the struct here and put the definition in the .cpp file? I believe this can be done even for a nested struct. Comment on attachment 237350 [details] fix cmake build View in context: https://bugs.webkit.org/attachment.cgi?id=237350&action=review Makes sense to me as well, the abstraction did cause some confusion in the past. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:264 > + if (isSynchronous()) { It's slightly annoying that the unusual code path goes first here and elsewhere. Comment on attachment 237350 [details] fix cmake build View in context: https://bugs.webkit.org/attachment.cgi?id=237350&action=review >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:264 >> + if (isSynchronous()) { > > It's slightly annoying that the unusual code path goes first here and elsewhere. I agree with if...else cases but here specifically we are doing an early return. I think the favored style is that main body of the function is the common case while early returns are exceptions. |