<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [SOUP] Add NetworkSession implementation and switch to use it"
href="https://bugs.webkit.org/show_bug.cgi?id=163597#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [SOUP] Add NetworkSession implementation and switch to use it"
href="https://bugs.webkit.org/show_bug.cgi?id=163597">bug 163597</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=163597#c16">comment #16</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=292081&action=diff" name="attach_292081" title="Try to fix the builds">attachment 292081</a> <a href="attachment.cgi?id=292081&action=edit" title="Try to fix the builds">[details]</a></span>
> Try to fix the builds
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=292081&action=review">https://bugs.webkit.org/attachment.cgi?id=292081&action=review</a>
>
> r=me</span >
Thanks!
<span class="quote">> > 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.</span >
Ok, reordered to follow the same logic than the #ifdefs block in the members declarations.
<span class="quote">> > Source/WebKit2/NetworkProcess/Downloads/Download.h:55
> > +class NetworkDataTask;
>
> Let's put this below. Forward declaring an unused class doesn't cause
> problems.</span >
It's not here to be inside a platform ifdef, but because I need it for the PlatformDownloadTaskRef definition.
<span class="quote">> > 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.</span >
Agree, better fail the build than crash.
<span class="quote">> > 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.</span >
Yes.
<span class="quote">> > 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.</span >
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.
<span class="quote">> > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:144
> > + url.setQuery(String());
> > + url.removeFragmentIdentifier();
>
> I think these are unnecessary</span >
You are right, the path doesn't include the fragment identifier nor the query, so lastPathComponent() will never include them.
<span class="quote">> > 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.</span >
I'll take it into account when refactoring it.
<span class="quote">> > 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.</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>