Bug 108215 – [GTK] Add WTFURL source files to the build

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK] Add WTFURL source files to the build

[Zan Dobersek]

Created attachment 185291 [details]
Patch

[Martin Robinson]

Comment on attachment 185291 [details]
Patch

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

> Source/WebCore/platform/KURLWTFURL.cpp:-288
> -String KURL::fileSystemPath() const
> -{
> -    return string();
> -}
> -

If you remove the implementation, won't this cause linking errors?

[Zan Dobersek]

Comment on attachment 185291 [details]
Patch

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

>> Source/WebCore/platform/KURLWTFURL.cpp:-288
>> -
> 
> If you remove the implementation, won't this cause linking errors?

At the moment no port actually uses WTFURL so this change wouldn't affect any system, but the KURL::fileSystemPath method is at the moment implemented in port-specific manners in Source/WebCore/platform/gtk/KURLGtk.cpp, Source/WebCore/platform/cf/KURLCFNet.cpp etc.

[Benjamin Poulain]

With the recent changes in the URL specs, I am less and less certain WTFURL is the right solution.

The current direction reverse the steam and seems to go away from the custom URL handling as done by GoogleURL/WTFURL.

Zan, you seem to have a genuine interest in URL handling. What is your opinion on the matter?

[Zan Dobersek]

AFAIK there was once intention to have an API based on WTFURL (pretty much what ParsedURL is today) that could be used outside of WebKit as well. I like that idea and think that if there's still interest in it, it should be 'branded' as a WTFURL utility.

The implementation of it should, of course, follow the standards. While they may be a moving target I think the current WTFURL code, based on GoogleURL, still has parts that could be reused in the future implementations - canonicalization, percent encoding, general parsing etc.

My idea of how to proceed here is to clean up the WTFURL code a bit, align its style with the style guidelines and add unit tests for the current behavior. These should probably consist of test cases currently used in GoogleURL. The implementation should then proceed to be altered so it would be in line with the specification.

The WTFURL backend of KURL could be expanded either now or concurrently with bringing WTFURL into conformance with the specs. Either way the WTFURL API will be expanded and polished, though there's no need for it to strictly expose only functionality that's needed by the backend implementation.

Thoughts?

[Benjamin Poulain]

(In reply to comment #5)
> AFAIK there was once intention to have an API based on WTFURL (pretty much what ParsedURL is today) that could be used outside of WebKit as well. I like that idea and think that if there's still interest in it, it should be 'branded' as a WTFURL utility.

Yep, that is a goal of WTFURL.
In order for it to be adopted by Chromium, we must be able to build WTFURL separately from WTF.

> The implementation of it should, of course, follow the standards. While they may be a moving target I think the current WTFURL code, based on GoogleURL, still has parts that could be reused in the future implementations - canonicalization, percent encoding, general parsing etc.
> 
> My idea of how to proceed here is to clean up the WTFURL code a bit, align its style with the style guidelines and add unit tests for the current behavior. These should probably consist of test cases currently used in GoogleURL. The implementation should then proceed to be altered so it would be in line with the specification.
> 
> The WTFURL backend of KURL could be expanded either now or concurrently with bringing WTFURL into conformance with the specs. Either way the WTFURL API will be expanded and polished, though there's no need for it to strictly expose only functionality that's needed by the backend implementation.
> 
> Thoughts?

Okay, I support this work. Please CC me whenever you post patches related to WTFURL.

[Benjamin Poulain]

Comment on attachment 185291 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should remove this line.

> Source/WebCore/platform/KURLWTFURL.cpp:-287
> -// FIXME: Get rid of this function from KURL.
> -String KURL::fileSystemPath() const
> -{
> -    return string();
> -}

I think that is gonna break the Mac build with WTFURL.

> Source/autotools/symbols.filter:243
> +_ZNK7WebCore4KURL6stringEv;

Shouldn't this be guarded by an #ifdef? Maybe you do not preprocess the symbols on GTK?

> ChangeLog:17
> [phone]  Zan Dobersek  <[e-mail]>
> +
> +        [GTK] Add WTFURL source files to the build
> +        https://bugs.webkit.org/show_bug.cgi?id=108215
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/autotools/symbols.filter:
> +
> [phone]  Zan Dobersek  <[e-mail]>
> +
> +        [GTK] Add WTFURL source files to the build
> +        https://bugs.webkit.org/show_bug.cgi?id=108215
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/autotools/symbols.filter: Force exporting the KURL::string() symbol

Double changelog.

[Zan Dobersek]

Comment on attachment 185291 [details]
Patch

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

>> Source/WebCore/platform/KURLWTFURL.cpp:-287
>> -}
> 
> I think that is gonna break the Mac build with WTFURL.

I can wrap it inside !PLATFORM(GTK) for now.

>> Source/autotools/symbols.filter:243
>> +_ZNK7WebCore4KURL6stringEv;
> 
> Shouldn't this be guarded by an #ifdef? Maybe you do not preprocess the symbols on GTK?

No, the file isn't preprocessed, but the symbol can still be exported, it doesn't hurt the default build. I'll upload the new patch here and let EWS process it just in case.

[Zan Dobersek]

Created attachment 185706 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 185706 [details]
Patch for landing

Clearing flags on attachment: 185706

Committed r141622: <http://trac.webkit.org/changeset/141622>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.