Bug 21841 - GoogleURL version of KURL does not implement conversion to CFURLRef
Summary: GoogleURL version of KURL does not implement conversion to CFURLRef
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Mark Mentovai
URL:
Keywords:
Depends on: 21807
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-23 15:28 PDT by Mark Mentovai
Modified: 2008-10-23 17:28 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2008-10-23 15:29 PDT, Mark Mentovai
darin: review-
Details | Formatted Diff | Diff
Alternate patch (1.60 KB, patch)
2008-10-23 16:13 PDT, Mark Mentovai
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Mentovai 2008-10-23 15:28:39 PDT
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.
Comment 1 Mark Mentovai 2008-10-23 15:29:20 PDT
Created attachment 24623 [details]
Patch
Comment 2 Mark Rowe (bdash) 2008-10-23 15:35:41 PDT
Comment on attachment 24623 [details]
Patch

Having a dependency on USE(GOOGLEURL) in GraphicsContext feels deeply wrong.
Comment 3 Mark Mentovai 2008-10-23 15:37:19 PDT
Having a dependency on URLs in general in a GraphicsContext feels pretty wrong...
Comment 4 Darin Adler 2008-10-23 16:09:30 PDT
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.
Comment 5 Mark Mentovai 2008-10-23 16:13:36 PDT
Created attachment 24625 [details]
Alternate patch

Takes out PLATFORM(CF) check as suggested.
Comment 6 Darin Adler 2008-10-23 16:14:01 PDT
Comment on attachment 24625 [details]
Alternate patch

Please fix this in KURL.h as I requested.
Comment 7 Darin Adler 2008-10-23 16:14:28 PDT
(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().
Comment 8 Darin Adler 2008-10-23 16:15:53 PDT
(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".
Comment 9 Darin Adler 2008-10-23 16:17:43 PDT
Retitling based on my understanding of what the bug is.
Comment 10 Mark Mentovai 2008-10-23 17:28:09 PDT
I'll just fix this directly in GURL then.