[webkit-reviews] review denied: [Bug 28882] Merge chromium's config.h.in into WebCore's config.h : [Attachment 39025] Adds chromium specific defines to WebCore/config.h

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Yaar Schnitman
<yaar at chromium.org>'s request for review:
Bug 28882: Merge chromium's config.h.in into WebCore's config.h
https://bugs.webkit.org/show_bug.cgi?id=28882

Attachment 39025: Adds chromium specific defines to WebCore/config.h
https://bugs.webkit.org/attachment.cgi?id=39025&action=review

------- Additional Comments from David Levin <levin at chromium.org>

> diff --git a/WebCore/config.h b/WebCore/config.h
> @@ -17,7 +17,6 @@
>   * Boston, MA 02110-1301, USA.
>   *
>   */
> -
Please leave this the way it was.

Also add an appropriate copyright line at the top (presumably Google).


> +// TODO(mark): This list will hopefully shrink but may also grow.
s/TODO(mark)/FIXME/

I guess this is a perpetual FIXME.

> +// run "nm libwebcore.a | grep -E '[atsATS] ([+-]\[|\.objc_class_name)'" and


> +#define WebCoreControlTintObserver \
> +	   ChromiumWebCoreObjCWebCoreControlTintObserver

I didn't see this name when I ran "nm libwebcore.a | grep -E '[atsATS]
([+-]\[|\.objc_class_name)'"

> +#else // !PLATFORM(DARWIN)
> +
> +// Don't define SKIA on Mac. Undefine other things as well that might get
set
> +// as side-effects.

I find this comment very confusing.  It says not to define it but then defines
it.  Of course, this is the non OSX section.  How about this?

// Only use SKIA on non-Mac platforms.

> +#define WTF_PLATFORM_SKIA 1

// Undefine other items that may get set as side-effects.
As side-effects of what (using SKIA)?

> +#undef WTF_PLATFORM_CG
> +#undef WTF_PLATFORM_CF


> +#endif // if PLATFORM(CHROMIUM)
No "if" here.