Bug 163597 – [SOUP] Add NetworkSession implementation and switch to use it

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Basically move the ResourceHandle implementation to the network process, but modernized, cleaned up, and adapted to the NetworkSession model.

[Carlos Garcia Campos]

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.

[Alex Christensen]

!

[Alex Christensen]

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.

[Alex Christensen]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 292064 [details]
Rebased patch

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 292071 [details]
Try to fix the builds

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Created attachment 292073 [details]
Try to fix the build

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Created attachment 292081 [details]
Try to fix the builds

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[Alex Christensen]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 292152 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r207586: <http://trac.webkit.org/changeset/207586>