Bug 38470 - Add first WTFURL file
Summary: Add first WTFURL file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-03 09:22 PDT by Adam Barth
Modified: 2010-05-04 16:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (297 bytes, patch)
2010-05-03 09:22 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (7.37 KB, patch)
2010-05-04 00:31 PDT, Adam Barth
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-05-03 09:22:24 PDT
Add empty folder to house URL parser
Comment 1 Adam Barth 2010-05-03 09:22:56 PDT
Created attachment 54932 [details]
Patch
Comment 2 Adam Barth 2010-05-03 09:23:32 PDT
I wasn't sure whether to put this at the root level or in the JavaScriptCore/wtf folder.
Comment 3 Darin Adler 2010-05-03 09:45:36 PDT
Comment on attachment 54932 [details]
Patch

Change log looks fine. There's no directory in the patch, but rs=me on adding the directory.
Comment 4 Adam Barth 2010-05-03 10:27:02 PDT
Comment on attachment 54932 [details]
Patch

I landed this patch and then reverted it in r58683 because Darin Adler would prefer that we not create a new top-level directory for this code.
Comment 5 Darin Adler 2010-05-03 10:35:21 PDT
It’s a lot more convenient for Apple if the new code goes in some existing directory and shares in the existing build system in that directory. While I wouldn’t rule out making a new top-level project entirely, I’d prefer that this be somewhere in either JavaScriptCore or WebCore.
Comment 6 Adam Barth 2010-05-03 10:46:27 PDT
Here are some locations that have been floated:

JavaScriptCore/wtf/url
JavaScriptCore/wtf/URLCore
JavaScriptCore/URLCore
WebCore/URLCore

It seems a bit silly to put the code in JavaScriptCore because it has very little to do with JavaScript.  It has more to do with WebCore, so maybe that makes the most sense.  My guess is that fishd's main concern is that the code not accrete dependencies on WebCore.

Perhaps WebCore/URLCore with its own ChangeLog file makes the most sense?  We already have folders in WebCore with special dependency rules (e.g., platform).
Comment 7 Alexey Proskuryakov 2010-05-03 13:25:30 PDT
If it's going to be in WebCore, why not put it in WebCore/platform?
Comment 8 Adam Barth 2010-05-03 14:47:49 PDT
(In reply to comment #7)
> If it's going to be in WebCore, why not put it in WebCore/platform?

You're suggesting WebCore/platform/url ?

Here's some more info from email:

On Mon, May 3, 2010 at 11:41 AM, Darin Adler <darin@apple.com> wrote:
>
> On May 3, 2010, at 10:17 AM, Adam Barth wrote:
>
> > How about JavaScriptCore/wtf/url ?
> >
> > I originally used the name "URLCore" to fit in with the rest of the top-level "Core" directories.  It seems a bit odd to have the code inside the JavaScriptCore folder, but that might make sense if/when wtf is moved into its own top-level directory.
>
> Seems OK.
>
> I think the real choice isn’t WebCore vs. JavaScriptCore, but rather WebCore’s platform vs. WTF. Both of those are conceptually separate libraries although they live inside the WebCore and JavaScriptCore directory. At some point I’d like to make those link and compile separately to enforce correct layering, but there are costs associated so I am not in a rush to do that right away.
>
> For Chromium’s sake, I think WTF is right since they are basically treating URL as a low level thing, and probably don’t want to have the networking library depend on the WebCore platform layer.
Comment 9 Adam Barth 2010-05-04 00:31:29 PDT
Created attachment 55000 [details]
Patch
Comment 10 Maciej Stachowiak 2010-05-04 00:54:25 PDT
Comment on attachment 55000 [details]
Patch

JavaScriptCore/wtf/url/src/URLComponent.h:66
 +      int m_length; // Will be -1 if the component is unspecified.
I'm not a fan of in-band signalling like this. It tends to be error-prone. Is the size of this class critical? In any case, I guess it's fine to initially land like this.

r=me
Comment 11 Adam Barth 2010-05-04 00:57:45 PDT
Committed r58740: <http://trac.webkit.org/changeset/58740>
Comment 12 Darin Fisher (:fishd, Google) 2010-05-04 14:24:57 PDT
(In reply to comment #5)
> It’s a lot more convenient for Apple if the new code goes in some existing
> directory and shares in the existing build system in that directory. While I
> wouldn’t rule out making a new top-level project entirely, I’d prefer that this
> be somewhere in either JavaScriptCore or WebCore.

A goal for Chromium at least is to be able to make use of this code from our network stack without having the network stack depend on any other parts of WebKit.  This is why I would prefer a new top-level directory, so the separation is clearer to developers.
Comment 13 Adam Barth 2010-05-04 14:34:41 PDT
The two issues seem somewhat separable:

1) Where the code lives.
2) What the dependency rules are.

My goal is to make the code have as few dependencies as possible.  Actually, I'm aiming for zero (or maybe just ICU), like the current code has.
Comment 14 Darin Fisher (:fishd, Google) 2010-05-04 14:36:33 PDT
(In reply to comment #13)
> The two issues seem somewhat separable:
> 
> 1) Where the code lives.
> 2) What the dependency rules are.
> 
> My goal is to make the code have as few dependencies as possible.  Actually,
> I'm aiming for zero (or maybe just ICU), like the current code has.

OK.  There is a third concern:  how do we keep it that way?
Comment 15 Adam Barth 2010-05-04 14:40:11 PDT
> OK.  There is a third concern:  how do we keep it that way?

The same way we keep most of our invariants: buildbots.
Comment 16 Adam Barth 2010-05-04 14:43:25 PDT
To be more specific, I imagine that the Chromium port will re-map JavaScriptCore/wtf/url to WebKit/chromium/wtfurl in the same way it current maps googleurl into that location.  The main Chromium build will probably re-map it to src/wtfurl (again, where googleurl is currently mapped).  By building the library using wtfurl.gyp, the Chromium buildbots will ensure that its dependencies remain in check.
Comment 17 Brett Wilson (Google) 2010-05-04 15:53:15 PDT
Currently the ICU dependency is in one source file only. It does require the headers to compile. We did this so other projects could use this coded without taking a 10MB hit, by supplying a simple converter file (assuming they don't want IDN). It is used by other projects at Google in this way.

I think we should try to maintain it being separate from ICU in this way when it lives in WebCore as well.
Comment 18 Darin Fisher (:fishd, Google) 2010-05-04 16:25:11 PDT
(In reply to comment #16)
> To be more specific, I imagine that the Chromium port will re-map
> JavaScriptCore/wtf/url to WebKit/chromium/wtfurl in the same way it current
> maps googleurl into that location.  The main Chromium build will probably
> re-map it to src/wtfurl (again, where googleurl is currently mapped).  By
> building the library using wtfurl.gyp, the Chromium buildbots will ensure that
> its dependencies remain in check.

It would be nice to avoid the extra duplication of svn checkouts.  What does re-mapping buy us?  (Maybe we should discuss this in another forum.)