Bug 13865 – Making usage of the icon database optional

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Treat]

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.

[Adam Treat]

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.

[David Kilzer (:ddkilzer)]

Comment on attachment 14710 [details]
Makes use of icon database optional

Needs a ChangeLog entry for JavaScriptCore (yes, even for such a small change :).

Note that I did not review the rest of the patch.

Please set "review?" flag in the future for patches that you'd like to have reviewed.

[Brady Eidson]

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!

[George Staikos]

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.

[George Staikos]

note: some builds are now sharing makefiles which I think compounds the problem.

[Adam Treat]

BTW, we already do #ifdef out CG for non-CG platforms.

[Adam Treat]

Created attachment 14725 [details]
Update to patch with JavaScriptCore/Changelog included

Update the JavaScriptCore Changelog too.

[Brady Eidson]

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...

[George Staikos]

(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.

[Brady Eidson]

Comment on attachment 14725 [details]
Update to patch with JavaScriptCore/Changelog included

I bounced it off Darin too, and he likes it, so I think we have agreement to go with 2 files globally ifdef'd instead of spewing ifdefs all over

[Adam Treat]

Created attachment 15109 [details]
Makes the icon database optional

Revised patch as per comments and suggestions.

[Brady Eidson]

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?

[Adam Treat]

Thanks!  Yes, actually making usage of it is a separate patch :)

[George Staikos]

patch applied