GoogleURL version of KURL does not implement conversion to CFURLRef

• Component: Platform
• Status: RESOLVED WONTFIX

[Mark Mentovai]

In WebCore/platform/graphics/cg/GraphicsContextCG.cpp, GraphicsContext::setURLForRect takes a KURL argument and converts it to a CFURLRef, to pass to CGPDFContextSetURLForRect.  This can only work when the KURL-to-CFURLRef conversion is available in KURL.

In Chromium, we're using a different implementation of KURL, based on GoogleURL, and ours doesn't do CFURLRef conversions.  We run into a problem, then, when GraphicsContext::setURLForRect tries to do this conversion.  All implementations of GraphicsContext other than the CG one just do notImplemented() in setURLForRect.

The attached patch makes GraphicsContextCG sensitive to whether the GoogleURL library is in use based on USE(GOOGLEURL), which was proposed in bug 21807.  Because KURL support for CFURLRef conversions is also dependent on PLATFORM(CF), I've included that for completeness.  If PLATFORM(CF) is not defined or USE(GOOGLEURL) is, GraphicsContextCG::setURLForRect is a simple notImplemented(), as it currently is on other platforms.

[Mark Mentovai]

Created attachment 24623 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 24623 [details]
Patch

Having a dependency on USE(GOOGLEURL) in GraphicsContext feels deeply wrong.

[Mark Mentovai]

Having a dependency on URLs in general in a GraphicsContext feels pretty wrong...

[Darin Adler]

Comment on attachment 24623 [details]
Patch

A better way to handle this is would be to put the notImplemented() call inside the GOOGLEURL version of CFURLRef. There's no good reason that KURL when PLATFORM(CF) is true shouldn't implement conversion to a CFURL, regardless of whether it's based on the Google URL library; putting the workaround for that in the GraphicsContext file seems wrong.

> +#if PLATFORM(CF) && !USE(GOOGLEURL)

The !USE(GOOGLEURL) part is fine, but the PLATFORM(CF) check here is unnecessary and should be removed. CoreGraphics is built on top of CoreFoundation. You can't compile GraphicsContextCG.cpp if PLATFORM(CF) is false.

I'm going to say review- because I think this check is in the wrong file, and even if you disagree and think this is the right file. it's not done exactly right.

[Mark Mentovai]

Created attachment 24625 [details]
Alternate patch

Takes out PLATFORM(CF) check as suggested.

[Darin Adler]

Comment on attachment 24625 [details]
Alternate patch

Please fix this in KURL.h as I requested.

[Darin Adler]

(In reply to comment #4)
> (From update of attachment 24623 [details] [edit])
> A better way to handle this is would be to put the notImplemented() call inside
> the GOOGLEURL version of CFURLRef.

I meant the GOOGLEURL version of KURL::createCFURL().

[Darin Adler]

(In reply to comment #0)
> In Chromium, we're using a different implementation of KURL, based on
> GoogleURL, and ours doesn't do CFURLRef conversions.

That seems wrong.

There's no reason to not implement conversions from a GoogleURL object to the underlying platform's URL object, CFURL.

It's analogous to saying "we have our own string class and have chosen to not implement conversion to CFString".

[Darin Adler]

Retitling based on my understanding of what the bug is.

[Mark Mentovai]

I'll just fix this directly in GURL then.