Bug 21807 – Add the ability for embedders to use Google-URL without changing shared headers

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brett Wilson (Google)]

Embedders should have the ability to optionally use Google-URL without changing shared headers. Chromium currently has KURL.h forked.

Some background is discussed in these two threads:

https://lists.webkit.org/pipermail/webkit-dev/2008-October/005200.html
https://lists.webkit.org/pipermail/webkit-dev/2008-October/005221.html

[Brett Wilson (Google)]

Created attachment 24564 [details]
Patch

Changes to KURL.h to allow it to bring in Google-URL. No actual code using Google-URL is present in this patch, it merely changes the shared files.

The goal of this patch is not to be a long-term solution, but something that lets Chromium unfork in the short term, and allows the WebKit community to evaluate the library. This will allow the community to make a better determination of the future direction.

Technically, I tried to impack KURL.h as little as possible. I conditionally include a header, and use the object defined in that header to replace most of the private stuff in KURL. This allows us not to pollute the file with a lot of experimental code.

I moved a handful of functions to the .ccp file since their implementation would be different and this avoids having to add more ifdefs to the header file. I kept a few functions in the header that should make a difference being inlined like the string getter, and used ifdefs for them. I'm happy to use more or less inlining if you prefer (with corresponding change in ugliness in the header).

[Brett Wilson (Google)]

Created attachment 24565 [details]
Example implementation (not to be checked in)

This is an example implementation of the GoogleURLPrivate.h file conditionally included in KURL.h in the previous patch, as well as a different implementation of the .cp file using this.

This patch is not for checkin, it is provided for reference so the reviewer for the KURL.h patch can see how things are meant to to work. It works well in teh Chromium build on Windows. I am having some problems getting it to work properly in WebKit on Mac under Safari, so I am currently still working on this.

[Sam Weinig]

Comment on attachment 24564 [details]
Patch

I am unflagging this for review because I don't think we have come to a consensus on whether to put a second URL implementation into WebCore.  The email thread on webkit-dev seemed inconclusive, and as such, I don't think it is appropriate to proceed.  Brett, it may be helpful for you respond to the thread with your understanding of the conclusion (if any).

[Brett Wilson (Google)]

Comment on attachment 24564 [details]
Patch

I had a lot of discussions with Maciej about this and we agreed that this would be an acceptable interim solution.

The second thread I mentioned in my previous comment was the result of these discussions.
https://lists.webkit.org/pipermail/webkit-dev/2008-October/005221.html

I'm unclear on what you think was not resolved.

[Maciej Stachowiak]

Brett,

You never replied to this:

https://lists.webkit.org/pipermail/webkit-dev/2008-October/005237.html

So I'm not sure if you agree with the comments of your Google co-workers:

https://lists.webkit.org/pipermail/webkit-dev/2008-October/005242.html
https://lists.webkit.org/pipermail/webkit-dev/2008-October/005255.html

Thus we're not entirely sure what we'd signing up for with this patch.


It would also be good to discuss how we'll evaluate URL code to arrive at a single solution; that doesn't have to block landing any kind of patch, but it might be good to start that discussion in parallel, otherwise we might end up putting it off forever.

[Brett Wilson (Google)]

I thought Peter answered that question, which is why I never replied. I agree with his response.

[Darin Adler]

Comment on attachment 24564 [details]
Patch

This patch unnecessarily reduces the amount of inlining done in KURL for simple operations. The non-Google-URL versions of these functions should still be inlined.

This can be done either by adding a new header for the inline functions that's included by KURL.h or by adding these function definitions to the end of KURL.h rather than KURL.cpp.

Or you could prove that inlining of these isn't helpful for performance or code size and then leave them in the .cpp file.

Otherwise the patch seems OK if still a bit inelegant.

 214     void parse(const char* url, const String* originalString);  // KURLMac calls this.
 215     void copyToBuffer(Vector<char, 512>& buffer) const;  // KURLCFNet uses this.

These should be outside the #if USE(GOOGLEURL) rather than repeated on both sides of the if.

review- because of the unnecessary inlining change

[Brett Wilson (Google)]

Created attachment 25985 [details]
Patch v2

Thanks for the comments. This addressed Darin's comments by putting the inline functions at the bottom of the header file like he suggested.

[Darin Adler]

Comment on attachment 25985 [details]
Patch v2

r=me

> +#if !USE(GOOGLEURL)
> +// Inline versions of some non-GoogleURL functions so we can get inlining
> +// without having to have a lot of ugly ifdefs in the class definition.

Incredibly minor comment someone could possibly tweak while landing: I'd like to see a blank line here to better match the blank line before #endif.

[Brett Wilson (Google)]

Fixed in 39253 with Darin's extra newline.