WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
21841
GoogleURL version of KURL does not implement conversion to CFURLRef
https://bugs.webkit.org/show_bug.cgi?id=21841
Summary
GoogleURL version of KURL does not implement conversion to CFURLRef
Mark Mentovai
Reported
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.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Mark Mentovai
Comment 1
2008-10-23 15:29:20 PDT
Created
attachment 24623
[details]
Patch
Mark Rowe (bdash)
Comment 2
2008-10-23 15:35:41 PDT
Comment on
attachment 24623
[details]
Patch Having a dependency on USE(GOOGLEURL) in GraphicsContext feels deeply wrong.
Mark Mentovai
Comment 3
2008-10-23 15:37:19 PDT
Having a dependency on URLs in general in a GraphicsContext feels pretty wrong...
Darin Adler
Comment 4
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.
Mark Mentovai
Comment 5
2008-10-23 16:13:36 PDT
Created
attachment 24625
[details]
Alternate patch Takes out PLATFORM(CF) check as suggested.
Darin Adler
Comment 6
2008-10-23 16:14:01 PDT
Comment on
attachment 24625
[details]
Alternate patch Please fix this in KURL.h as I requested.
Darin Adler
Comment 7
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().
Darin Adler
Comment 8
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".
Darin Adler
Comment 9
2008-10-23 16:17:43 PDT
Retitling based on my understanding of what the bug is.
Mark Mentovai
Comment 10
2008-10-23 17:28:09 PDT
I'll just fix this directly in GURL then.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug