Bug 108215 - [GTK] Add WTFURL source files to the build
Summary: [GTK] Add WTFURL source files to the build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-29 12:41 PST by Zan Dobersek
Modified: 2013-02-01 12:08 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.35 KB, patch)
2013-01-29 12:52 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (7.59 KB, patch)
2013-01-31 00:32 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-01-29 12:41:13 PST
[GTK] Add WTFURL source files to the build
Comment 1 Zan Dobersek 2013-01-29 12:52:03 PST
Created attachment 185291 [details]
Patch
Comment 2 Martin Robinson 2013-01-29 13:38:04 PST
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?
Comment 3 Zan Dobersek 2013-01-29 13:54:32 PST
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.
Comment 4 Benjamin Poulain 2013-01-29 14:15:22 PST
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?
Comment 5 Zan Dobersek 2013-01-30 13:23:41 PST
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?
Comment 6 Benjamin Poulain 2013-01-30 15:02:03 PST
(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.
Comment 7 Benjamin Poulain 2013-01-30 15:05:50 PST
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
> +2013-01-29  Zan Dobersek  <zdobersek@igalia.com>
> +
> +        [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:
> +
> +2013-01-29  Zan Dobersek  <zdobersek@igalia.com>
> +
> +        [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.
Comment 8 Zan Dobersek 2013-01-31 00:17:51 PST
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.
Comment 9 Zan Dobersek 2013-01-31 00:32:57 PST
Created attachment 185706 [details]
Patch for landing
Comment 10 Zan Dobersek 2013-02-01 12:08:20 PST
Comment on attachment 185706 [details]
Patch for landing

Clearing flags on attachment: 185706

Committed r141622: <http://trac.webkit.org/changeset/141622>
Comment 11 Zan Dobersek 2013-02-01 12:08:25 PST
All reviewed patches have been landed.  Closing bug.