Updated autotools to match ToT, allow disabling of database and icon database in the Gtk port, and merge patches and various autotools fixes.
Created attachment 18171 [details] various autotools update and fixes patch
Comment on attachment 18171 [details] various autotools update and fixes patch Although it's not new to this patch, I think it's a seriously bad idea that GNUMakeFile.am has a copy of the contents of DerivedSources.make -- the whole idea of DerivedSources.make is to have a shared makefile for these more complicated rules. Surely we can find a way to incorporate those rules without copying and pasting the entire file. +#if ENABLE(DATABASE) +#include "Database.h" +#endif This change is wrong. The file "Database.h" already has an #if ENABLE(DATABASE) around its contents. We don't want to ifdef it in both places. +#if ENABLE(DATABASE) +#include "DatabaseTracker.h" +#endif We need to be consistent. The ENABLE #if statements are inside Database.h and should go inside DatabaseTracker.h too. Or we could go the other way and change how we handle optional things like database. ENABLE_XBL should not be in the makefile. XBL was an experiment done more than two years ago. It doesn't make sense to have it commented-out code in there with a FIXME. Everything else looks fine. review- because of the database include files issue.
Created attachment 18172 [details] Fix database inclusion and few fixes to the patch Hi Darin > +#if ENABLE(DATABASE) > +#include "Database.h" > +#endif > > This change is wrong. The file "Database.h" already has an #if > ENABLE(DATABASE) around its contents. We don't want to ifdef it in both > places. Removed the ifdef in Database.h. See explanation below. > +#if ENABLE(DATABASE) > +#include "DatabaseTracker.h" > +#endif > > We need to be consistent. The ENABLE #if statements are inside Database.h and > should go inside DatabaseTracker.h too. Or we could go the other way and > change how we handle optional things like database. I went the other way around to make Database consistent with the other features, since Database ifdefs are already there, it's just a matter of adding a guard before including Database.h, which pretty much what the other features are doing (e.g. include xpath headers only if xpath is enabled). > ENABLE_XBL should not be in the makefile. XBL was an experiment done more than > two years ago. It doesn't make sense to have it commented-out code in there > with a FIXME. Removed XBL from the makefile/configure scripts. Also, ideally we should be using DerivedSources.make but i had some issues with the existing makefile (e.g., vpath mangling, some make constructs do not work well with automake) as well as the risk of making untested changes that affects the other platforms (i dont have a mac for example). But anyway, patches are always welcome if somebody wants to work on it. Thanks for the review!
Comment on attachment 18172 [details] Fix database inclusion and few fixes to the patch r=me The database header include change justification makes sense to me. We should keep an eye on the build bots to make sure this doesn't cause any issues in other ports after landing though.
Landed in r29033.
(In reply to comment #3) > I went the other way around to make Database consistent with the other > features, since Database ifdefs are already there, it's just a matter of adding > a guard before including Database.h, which pretty much what the other features > are doing (e.g. include xpath headers only if xpath is enabled). You say this is consistent with "the other features" but: 1) The XPath header files like XPathEvaluator.h all have guards in the header files. 2) The SVG header files like SVGUseElement.h all have guards in the header files. 3) The XSLT header files like XSTLProcessor.h all have guards in the header files. While I'm sure that there are some extra #if statements that aren't needed around some includes, I thinkt he pattern is that we put the guards in the header files, not at the include sites. I'd appreciate following one consistent approach. I think the current one is the "in the header file" approach and should be changed to be consistently that. > Also, ideally we should be using DerivedSources.make but i had some issues with > the existing makefile (e.g., vpath mangling, some make constructs do not work > well with automake) We need to do that work if possible. Putting the VPATH into a different file would be fine.
Looks like this was landed with the changes to make ENABLE(DATABASE) inconsistent with the other ENABLE use in headers. I'd prefer these things be consistent. See my comment above.
(In reply to comment #6) Hi Darin. Thanks for the reply. > While I'm sure that there are some extra #if statements that aren't needed > around some includes, I thinkt he pattern is that we put the guards in the > header files, not at the include sites. This is what confused me. dom/Document.cpp is a good example of this. > I'd appreciate following one consistent approach. I think the current one is > the "in the header file" approach and should be changed to be consistently > that. May I ask, what are the reasons why guards are put in the header files instead of in the include sites? > We need to do that work if possible. Putting the VPATH into a different file > would be fine. Ok. There's already a bit of discussion on this but no definite plan yet on when it will happen. Thanks again. Reopening the bug for patch submission as per outcome of this discussion.
(In reply to comment #8) > May I ask, what are the reasons why guards are put in the header files instead > of in the include sites? The main reason is that there are more include sites than header files. We discussed some other reasons on the webkit-dev mailing list. My main concern is consistency. This patch made DATABASE inconsistent with the others, and I don't like that. > > We need to do that work if possible. Putting the VPATH into a different file > > would be fine. > > Ok. There's already a bit of discussion on this but no definite plan yet on > when it will happen. > > Thanks again. > > Reopening the bug for patch submission as per outcome of this discussion. It doesn't seem right to reopen this bug for the task of sharing DerivedSources.make. That task should be tracked with another bug.