Bug 127104 – [SOUP] Remove soupURIToKURL

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

In favor of a URL constructor receiving a SoupURI. We can also add a method to URL to create a soupURI.

[Carlos Garcia Campos]

Created attachment 221371 [details]
Patch

[EFL EWS Bot]

Comment on attachment 221371 [details]
Patch

Attachment 221371 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6191449162907648

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 221371 [details])
> Attachment 221371 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/6191449162907648

Last 500 characters of output:
ir/__/__/DerivedSources/WebCore/JSSVGAnimatedEnumeration.cpp.o
c++: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.7/README.Bugs> for instructions.
make[2]: *** [Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/JSDOMWindow.cpp.o] Error 4
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Source/WebCore/CMakeFiles/WebCore.dir/all] Error 2
make: *** [all] Error 2

That doesn't seem to be my fault.

[Anders Carlsson]

Comment on attachment 221371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221371&action=review

> Source/WebCore/platform/URL.h:169
> +    SoupURI* createSoupURI() const;

Why can't you make createSoupURI() return a GOwnPtr?

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 221371 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221371&action=review
> 
> > Source/WebCore/platform/URL.h:169
> > +    SoupURI* createSoupURI() const;
> 
> Why can't you make createSoupURI() return a GOwnPtr?

Because our GOwnPtr is non copyable. We don't have PassGOwnPtr.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 221371 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=221371&action=review
> > 
> > > Source/WebCore/platform/URL.h:169
> > > +    SoupURI* createSoupURI() const;
> > 
> > Why can't you make createSoupURI() return a GOwnPtr?
> 
> Because our GOwnPtr is non copyable. We don't have PassGOwnPtr.

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

[Carlos Garcia Campos]

Created attachment 221996 [details]
New patch

Rebased and changed to return GUniquePtr when a new allocated SoupURI is returned.

[Carlos Garcia Campos]

Created attachment 222271 [details]
Try to fix EFL build

Not sure I did it right, I haven't found the way yo add include paths only for the webcore test, so I've added to the global includes (which seems to mix includes from webcore, JavaScriptCore, WebKit2, etc.)

[WebKit Commit Bot]

Attachment 222271 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/CMakeLists.txt:18:  Alphabetical sorting problem. "JavaScriptCore" should be before "WTF".  [list/order] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 222272 [details]
Try to fix EFL build

This time it was WTR

[WebKit Commit Bot]

Attachment 222272 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/CMakeLists.txt:18:  Alphabetical sorting problem. "JavaScriptCore" should be before "WTF".  [list/order] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 222272 [details]
Try to fix EFL build

View in context: https://bugs.webkit.org/attachment.cgi?id=222272&action=review

> Source/WebCore/platform/URL.h:168
> +    URL(SoupURI*);

Perhaps this should be marked "explicit."

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:129
> +GUniquePtr<SoupURI> ResourceRequest::createSoupURI() const
>  {
> -    // WebKit does not support fragment identifiers in data URLs, but soup does.
> -    // Before passing the URL to soup, we should make sure to urlencode any '#'
> -    // characters, so that soup does not interpret them as fragment identifiers.
> -    // See http://wkbug.com/68089
> -    if (m_url.protocolIsData()) {
> -        String urlString = m_url.string();
> -        urlString.replace("#", "%23");
> -        return soup_uri_new(urlString.utf8().data());
> -    }
> -
>      URL url = m_url;
>      url.removeFragmentIdentifier();
> -    SoupURI* uri = soup_uri_new(url.string().utf8().data());
> -
> -    // Versions of libsoup prior to 2.42 have a soup_uri_new that will convert empty passwords that are not
> -    // prefixed by a colon into null. Some parts of soup like the SoupAuthenticationManager will only be active
> -    // when both the username and password are non-null. When we have credentials, empty usernames and passwords
> -    // should be empty strings instead of null.
> -    if (!url.user().isEmpty() || !url.pass().isEmpty()) {
> -        soup_uri_set_user(uri, url.user().utf8().data());
> -        soup_uri_set_password(uri, url.pass().utf8().data());
> -    }
> -    return uri;
> +    return url.createSoupURI();
>  }

Can we just get rid of this entirely in favor of url().createSoupURI()?

[Carlos Garcia Campos]

(In reply to comment #12)
> (From update of attachment 222272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222272&action=review
> 
> > Source/WebCore/platform/URL.h:168
> > +    URL(SoupURI*);
> 
> Perhaps this should be marked "explicit."

I thought about it, but other platform constructor don't do that so I followed it :-P

> > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:129
> > +GUniquePtr<SoupURI> ResourceRequest::createSoupURI() const
> >  {
> > -    // WebKit does not support fragment identifiers in data URLs, but soup does.
> > -    // Before passing the URL to soup, we should make sure to urlencode any '#'
> > -    // characters, so that soup does not interpret them as fragment identifiers.
> > -    // See http://wkbug.com/68089
> > -    if (m_url.protocolIsData()) {
> > -        String urlString = m_url.string();
> > -        urlString.replace("#", "%23");
> > -        return soup_uri_new(urlString.utf8().data());
> > -    }
> > -
> >      URL url = m_url;
> >      url.removeFragmentIdentifier();
> > -    SoupURI* uri = soup_uri_new(url.string().utf8().data());
> > -
> > -    // Versions of libsoup prior to 2.42 have a soup_uri_new that will convert empty passwords that are not
> > -    // prefixed by a colon into null. Some parts of soup like the SoupAuthenticationManager will only be active
> > -    // when both the username and password are non-null. When we have credentials, empty usernames and passwords
> > -    // should be empty strings instead of null.
> > -    if (!url.user().isEmpty() || !url.pass().isEmpty()) {
> > -        soup_uri_set_user(uri, url.user().utf8().data());
> > -        soup_uri_set_password(uri, url.pass().utf8().data());
> > -    }
> > -    return uri;
> > +    return url.createSoupURI();
> >  }
> 
> Can we just get rid of this entirely in favor of url().createSoupURI()?

I left this just because of the url.removeFragmentIdentifier(); I assumed that's desired only in the cases where this method is called.

[Martin Robinson]

Comment on attachment 222272 [details]
Try to fix EFL build

Okay. That all sounds reasonable.

[Carlos Garcia Campos]

Committed r162922: <http://trac.webkit.org/changeset/162922>

[Michal Pakula vel Rutka]

It seems that r162922 broke 9 tests in EFL: 
fast/css/import-style-update.html fast/events/attribute-listener-cloned-from-frameless-doc-context-2.html fast/events/attribute-listener-cloned-from-frameless-doc-context.html fast/events/attribute-listener-extracted-from-frameless-doc-context-2.html fast/events/attribute-listener-extracted-from-frameless-doc-context.html fast/html/link-rel-stylesheet.html fast/loader/data-url-encoding-html.html fast/loader/data-url-encoding-svg.html fast/spatial-navigation/snav-iframe-nested.html

According to flakiness dashboard there are failures also in GTK i.e. http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss%2Fimport-style-update.html

[Carlos Garcia Campos]

(In reply to comment #16)
> It seems that r162922 broke 9 tests in EFL: 
> fast/css/import-style-update.html fast/events/attribute-listener-cloned-from-frameless-doc-context-2.html fast/events/attribute-listener-cloned-from-frameless-doc-context.html fast/events/attribute-listener-extracted-from-frameless-doc-context-2.html fast/events/attribute-listener-extracted-from-frameless-doc-context.html fast/html/link-rel-stylesheet.html fast/loader/data-url-encoding-html.html fast/loader/data-url-encoding-svg.html fast/spatial-navigation/snav-iframe-nested.html
> 
> According to flakiness dashboard there are failures also in GTK i.e. http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss%2Fimport-style-update.html

Ok, I'm looking at it.

[Carlos Garcia Campos]

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