I was surprised to very recently find MOZ_CAIRO_GFX in some OS/2 code, and so I looked and found several more in cross-platform code: http://mxr.mozilla.org/seamonkey/search?string=MOZ_CAIRO_GFX I think those should be removed. MOZ_THEBES is also only set in configure.in but not used anywhere. I found that then one could also get rid of GFX_HAS_INVERT which is set for cairo builds in layout/base/nsStyleConsts.h. Using the has_invert path would remove some more lines of code in layout: http://mxr.mozilla.org/seamonkey/search?string=GFX_HAS_INVERT But then I see in e.g. http://mxr.mozilla.org/seamonkey/source/layout/style/nsCSSParser.cpp#2933 http://mxr.mozilla.org/seamonkey/source/layout/base/nsLayoutUtils.cpp#2352 the #ifdefs are used for extra comments. So maybe those should stay?

GFX_HAS_INVERT is currently off, but we'd like cairo to support invert, so we have invert support again in the future, so we should probably leave that.

Oops, I misread #ifndef as #ifdef. So, yes then that left in, maybe as // XXX cairo doesn't support invert yet // #define GFX_HAS_INVERT

First try at this. This opens the path to remove even more stuff: ;-) - The FONT_LEADING_APIS_V2 #define from gfx/public/nsIFontMetrics.h. That is only used in layout/generic/nsHTMLReflowState.cpp and in gfx/src/windows/nsFontMetricsWin.{h,cpp} (Why are those files still there?!) - The mHandleAlphaColors stuff that's apparently only used in nsCSSParser.cpp. But I'm not really sure what the comment in that file implies (is it just about removing mHandleAlphaColors or about more?) Don't really know who to ask for review on this one...

Just to let you BeOS guys know that this would mess with your files. As there are no pre-cairo code paths to remove this shouldn't really affect you, though.

mHandleAlphaColors always to be TRUE, I thinks it is best to removed it.

OK, so with FONT_LEADING_APIS_V2 handled in bug 415686 I removed that part from this patch and instead added removal of the handleAlpha stuff, from the CSS parser and the only use of ParseColorString that I could find (in the canvas code). I think this is ready for review.

Comment on attachment 303921 [details] [diff] [review] 2nd try >Index: configure.in >-AC_DEFINE(MOZ_THEBES) >-AC_DEFINE(MOZ_CAIRO_GFX) You probably want to land this change last, after landing the rest and searching LXR again for both macros. Or at least make sure to search LXR that you've removed all tests for them after you land. >Index: layout/style/nsCSSParser.cpp >- (mHandleAlphaColors && (tk->mIdent.LowerCaseEqualsLiteral("rgba") || >- tk->mIdent.LowerCaseEqualsLiteral("hsla")))))) >+ (tk->mIdent.LowerCaseEqualsLiteral("rgba") || >+ tk->mIdent.LowerCaseEqualsLiteral("hsla"))))) You can remove the pair of parentheses around these two lines, since they're just the last two in a sequence of ||s. >Index: layout/style/nsICSSParser.h >- * Parse aBuffer into a nscolor |aColor|. If aHandleAlphaColors is >- * set, handle rgba()/hsla(). Will return NS_ERROR_FAILURE if >- * aBuffer is not a valid CSS color specification. >+ * Parse aBuffer into a nscolor |aColor|. rgba()/hsla() is always >+ * handled. Will return NS_ERROR_FAILURE if aBuffer is not a valid >+ * CSS color specification. How about "The alpha component of the resulting aColor may vary due to rgba()/hsla()." (It's less a reminder of the way the code used to be, but it's still worth warning callers about.) r+sr=dbaron with that, although you should probably also run this by stuart or vlad.

Nits fixed, carrying over reviews.

Comment on attachment 304015 [details] [diff] [review] patch v3 Vlad, could you please double check this as suggested by David?

(In reply to comment #7) > (From update of attachment 303921 [details] [diff] [review]) > >Index: configure.in > >-AC_DEFINE(MOZ_THEBES) > >-AC_DEFINE(MOZ_CAIRO_GFX) > > You probably want to land this change last, after landing the rest and > searching LXR again for both macros. Or at least make sure to search LXR that > you've removed all tests for them after you land. Yes, will keep that in mind. MOZ_THEBES is already gone from everywhere else, will leave them in a few hours longer to be sure.

Comment on attachment 304015 [details] [diff] [review] patch v3 Looks fine; only thing is, search for "XXX once" -- that comment may no longer be relevant. Also, for the first XXX, "cairo doesn't support invert yet", I don't think we'll ever have invert.

(In reply to comment #11) > Looks fine; only thing is, search for "XXX once" -- that comment may no longer > be relevant. I didn't touch that thinking that bug 372781 would deal with it. And honestly, I don't understand the part in brackets. Perhaps change // XXX Once non-cairo is no longer supported, we should remove // the special parsing of transparent for background-color and // border-color. (It currently overrides this, since keywords // are checked earlier in ParseVariant.) to // XXX Remove special parsing of transparent for background-color // and border-color (bug 372781). > Also, for the first XXX, "cairo doesn't support invert yet", I > don't think we'll ever have invert. OK, will do that without "XXX" and "yet" then... Thanks for the hints.

For that 4-line CSS comment you quote in comment 12, I'd just change "Once" to "Now that" and leave the rest.

OK then, this is for check-in. Carrying over reviews. As this just removed obsolete #defines and related stuff this should have no risk.

Comment on attachment 304423 [details] [diff] [review] patch v4 a=beltzner for 1.9

OK, the patch is in, except the configure.in change as per comment 7 and comment 10. Will get that in later today.

OK, using MXR I verified that MOZ_THEBES and MOZ_CAIRO_GFX were really not found anywhere in the tree any more, and so checked in the change to configure.in, too. So this is fixed now.