[Webkit-unassigned] [Bug 163597] [SOUP] Add NetworkSession implementation and switch to use it

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=163597

--- Comment #18 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161020/d5c590cf/attachment.html>