|Summary:||Making usage of the icon database optional|
|Product:||WebKit||Reporter:||Adam Treat <manyoso>|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Version:||523.x (Safari 3)|
Description Adam Treat 2007-05-24 14:43:16 PDT
Currently we use sqlite for storing icons. I'd like to introduce a define to make usage of the icon database optional for target platforms that do not support sqlite. I'll produce a patch and post for review.
Comment 1 Adam Treat 2007-05-24 19:04:11 PDT
Created attachment 14710 [details] Makes use of icon database optional Add defines around all usage of the icon database and set default to use it.
Comment 2 David Kilzer (:ddkilzer) 2007-05-24 22:28:14 PDT
Comment 3 Brady Eidson 2007-05-25 09:58:31 PDT
I really don't like this approach. Normally we #ifdef out pieces of larger overall features for varying platforms, but this is nuking *an entire* feature by scattering ifdefs over all of its code. Something that would make me a lot happier would be to have separate skeleton files with the blank implementations and have it defined at the project file level. That might not be ideal, either, but conditionally ifdefing out entire source files like this is less ideal imho. For example, though, taking the separate file approach you shouldn't even need to touch SQLDatabase* stuff at all - #ifdefing out SQLite utilities on a platform without SQLite is silly - that'd be like #ifdefing out CG utilities on non-CG platforms. You just don't include those files!
Comment 4 George Staikos 2007-05-25 10:19:13 PDT
I think the difference is that this is not platform dependent, but feature dependent. We can't have sqlite in a build we're doing and we need a way to remove that functionality. One issue is that if we move to separate files then we just push the problem into the makefiles instead of in config.h. It's not pleasant to deal with there either.
Comment 5 George Staikos 2007-05-25 10:20:26 PDT
note: some builds are now sharing makefiles which I think compounds the problem.
Comment 6 Adam Treat 2007-05-25 10:25:37 PDT
BTW, we already do #ifdef out CG for non-CG platforms.
Comment 7 Adam Treat 2007-05-25 10:30:08 PDT
Comment 8 Brady Eidson 2007-05-25 10:48:15 PDT
I suppose I had two points - re: SQL vs CG, my point was there are *CG only* files that are simply not included in non-CG builds - the same pattern holds for a lot of other platform or library specific files. My main point was re: the uglification of the IconDatabase code. We try to keep our cross-platform .cpp files as clean as possible and I like that! Some of the really complicated, large classes can't help but have some #ifdef blocks creep in there, but we largely avoid it by subclassing or splitting implementations between platform specific and non-specific files. I'm not saying that we don't care about platforms without sqlite and that we shouldn't make a fix here to support them - I'm just saying this solution is suboptimal. Perhaps reorganizing the header, putting a few intelligent #ifdefs there, then having two implementation files would be better. IconDatabaseSqlite and IconDatabaseNone (the names need a little work..) :) You could then conditionally comment out one or the other implementation file based on the new flag. Still suboptimal, but a lot prettier...
Comment 9 George Staikos 2007-05-25 11:26:44 PDT
(In reply to comment #8) > Perhaps reorganizing the header, putting a few intelligent #ifdefs there, then > having two implementation files would be better. IconDatabaseSqlite and > IconDatabaseNone (the names need a little work..) :) > > You could then conditionally comment out one or the other implementation file > based on the new flag. I like it.
Comment 10 Brady Eidson 2007-05-25 11:55:55 PDT
Comment 11 Adam Treat 2007-06-18 15:20:26 PDT
Created attachment 15109 [details] Makes the icon database optional Revised patch as per comments and suggestions.
Comment 12 Brady Eidson 2007-06-25 13:31:48 PDT
Comment on attachment 15109 [details] Makes the icon database optional The license at the top of the newly added file can just be "2007 Apple Inc." Otherwise, looks good. I assume you'll be adding the new one to your relevant project files in a seperate patch?
Comment 13 Adam Treat 2007-06-25 13:48:59 PDT
Thanks! Yes, actually making usage of it is a separate patch :)
Comment 14 George Staikos 2007-06-25 21:59:02 PDT