Basically move the ResourceHandle implementation to the network process, but modernized, cleaned up, and adapted to the NetworkSession model.
Created attachment 291949 [details] Patch This is a first patch, but it should be feature complete and hopefully mature enough (tests pass fine here). There's still some duplicated code that could be moved to common code, but I don't want to deal with Xcode in this patch, so it could be done in a follow up. The patch won't apply anyway, because it depends on bug #163588.
!
If you don't want to deal with Xcode, you could upload a patch that doesn't build on Mac or a patch with empty files and I can come clean it up and make it work on Mac. I'm really excited about this and will do what I can to help.
Comment on attachment 291949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291949&action=review > Source/WebKit2/NetworkProcess/NetworkDataTask.h:147 > + bool isDownload() const { return !!m_pendingDownloadID.downloadID(); } Could we call this isPendingDownload?
(In reply to comment #3) > If you don't want to deal with Xcode, you could upload a patch that doesn't > build on Mac or a patch with empty files and I can come clean it up and make > it work on Mac. I'm really excited about this and will do what I can to > help. Well, I said I don't want, but the fact is that I can't, because I don't have a Mac, and I really don't know how to manually edit XCode files. This patch is big enough, and it's a first implementation, my plan was to move the common code in a follow up patch. However, after talking to Keith Rollin by email, I plan to switch to a subclass model instead, so the common code would be in the base abstract class. I prefer to do that refactoring over an already working code base, so it's not only a matter of dealing with XCode.
(In reply to comment #4) > Comment on attachment 291949 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291949&action=review > > > Source/WebKit2/NetworkProcess/NetworkDataTask.h:147 > > + bool isDownload() const { return !!m_pendingDownloadID.downloadID(); } > > Could we call this isPendingDownload? That's only true for Cocoa, in soup we don't have a download task, it's the same data task that becomes a download, so after the task is converted into a download it's no longer pending.
Created attachment 292064 [details] Rebased patch
Attachment 292064 [details] did not pass style-queue: ERROR: Source/WebKit2/config.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
hmm, I'm using g_output_stream_write_all_async() for downloads which is too new, I'll have to use my own for older versions of glib.
Created attachment 292071 [details] Try to fix the builds
Attachment 292071 [details] did not pass style-queue: ERROR: Source/WebKit2/config.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 292073 [details] Try to fix the build
Attachment 292073 [details] did not pass style-queue: ERROR: Source/WebKit2/config.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 292081 [details] Try to fix the builds
Attachment 292081 [details] did not pass style-queue: ERROR: Source/WebKit2/config.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 292081 [details] Try to fix the builds View in context: https://bugs.webkit.org/attachment.cgi?id=292081&action=review r=me > Source/WebKit2/NetworkProcess/Downloads/Download.h:50 > +#if USE(NETWORK_SESSION) Let's just put this into the #if USE(NETWORK_SESSION) right above it. > Source/WebKit2/NetworkProcess/Downloads/Download.h:55 > +class NetworkDataTask; Let's put this below. Forward declaring an unused class doesn't cause problems. > Source/WebKit2/NetworkProcess/Downloads/Download.h:59 > +#else > +typedef void* PlatformDownloadTaskRef; Let's not start defining things for a hypothetical third implementation. If someone wants to make another implementation, they can define what they want. > Source/WebKit2/NetworkProcess/Downloads/Download.h:148 > #else > + PlatformDownloadTaskRef m_download; Let's remove this. It will need to be some kind of smart pointer anyways. > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:274 > + if (m_task && m_task->isDownload()) isPendingDownload > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:52 > + , m_session(&session) It looks like we could make m_session a Ref. > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:144 > + url.setQuery(String()); > + url.removeFragmentIdentifier(); I think these are unnecessary > Source/WebKit2/NetworkProcess/soup/NetworkSessionSoup.cpp:37 > +Ref<NetworkSession> NetworkSession::create(Type type, SessionID sessionID, CustomProtocolManager*) Maybe in a followup patch, and definitely an existing problem from my implementation, but it seems like type and sessionID contain redundant information. Type could be removed. > Source/WebKit2/config.h:78 > -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || USE(SOUP) I haven't tested this on linux. If you want, you could land this as a separate change so if it needs to be reverted then it's easier to revert a one line change than all this code.
(In reply to comment #6) > (In reply to comment #4) > > Comment on attachment 291949 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=291949&action=review > > > > > Source/WebKit2/NetworkProcess/NetworkDataTask.h:147 > > > + bool isDownload() const { return !!m_pendingDownloadID.downloadID(); } > > > > Could we call this isPendingDownload? > > That's only true for Cocoa, in soup we don't have a download task, it's the > same data task that becomes a download, so after the task is converted into > a download it's no longer pending. Oh, I missed this. Fine with me.
(In reply to comment #16) > Comment on attachment 292081 [details] > Try to fix the builds > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292081&action=review > > r=me Thanks! > > Source/WebKit2/NetworkProcess/Downloads/Download.h:50 > > +#if USE(NETWORK_SESSION) > > Let's just put this into the #if USE(NETWORK_SESSION) right above it. Ok, reordered to follow the same logic than the #ifdefs block in the members declarations. > > Source/WebKit2/NetworkProcess/Downloads/Download.h:55 > > +class NetworkDataTask; > > Let's put this below. Forward declaring an unused class doesn't cause > problems. It's not here to be inside a platform ifdef, but because I need it for the PlatformDownloadTaskRef definition. > > Source/WebKit2/NetworkProcess/Downloads/Download.h:59 > > +#else > > +typedef void* PlatformDownloadTaskRef; > > Let's not start defining things for a hypothetical third implementation. If > someone wants to make another implementation, they can define what they want. Agree, better fail the build than crash. > > Source/WebKit2/NetworkProcess/Downloads/Download.h:148 > > #else > > + PlatformDownloadTaskRef m_download; > > Let's remove this. It will need to be some kind of smart pointer anyways. Yes. > > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:274 > > + if (m_task && m_task->isDownload()) > > isPendingDownload > > > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:52 > > + , m_session(&session) > > It looks like we could make m_session a Ref. Indeed. Same for the Cocoa implementation. Actually this is part of the code that could be made common, so I'll change this as part of the follow up refactoring to also update NetworkDataTaskCocoa. > > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:144 > > + url.setQuery(String()); > > + url.removeFragmentIdentifier(); > > I think these are unnecessary You are right, the path doesn't include the fragment identifier nor the query, so lastPathComponent() will never include them. > > Source/WebKit2/NetworkProcess/soup/NetworkSessionSoup.cpp:37 > > +Ref<NetworkSession> NetworkSession::create(Type type, SessionID sessionID, CustomProtocolManager*) > > Maybe in a followup patch, and definitely an existing problem from my > implementation, but it seems like type and sessionID contain redundant > information. Type could be removed. I'll take it into account when refactoring it. > > Source/WebKit2/config.h:78 > > -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || USE(SOUP) > > I haven't tested this on linux. If you want, you could land this as a > separate change so if it needs to be reverted then it's easier to revert a > one line change than all this code. I removed the non network session code and assumed it to be enabled unconditionally when USE(SOUP) to simplify the code. I'll check the bots after landing this to roll it out asap if needed.
Created attachment 292152 [details] Patch for landing
Committed r207586: <http://trac.webkit.org/changeset/207586>