[webkit-reviews] review granted: [Bug 126180] [iOS] Upstream WebCore/page changes : [Attachment 219931] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 126180: [iOS] Upstream WebCore/page changes
https://bugs.webkit.org/show_bug.cgi?id=126180

Attachment 219931: Patch
https://bugs.webkit.org/attachment.cgi?id=219931&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219931&action=review


OK to merge all of this. A lot of it needs refinement longer term.

> Source/WTF/wtf/FeatureDefines.h:85
> +#if !defined(ENABLE_LETTERPRESS)
> +#define ENABLE_LETTERPRESS 1
> +#endif

This doesn’t seem to be in alphabetical order. Can we keep the alphabetical
sorting?

> Source/WTF/wtf/FeatureDefines.h:145
> +#if !defined(ENABLE_IOS_TOUCH_EVENTS) && ENABLE(TOUCH_EVENTS)
> +#define ENABLE_IOS_TOUCH_EVENTS 1
> +#endif

This doesn’t seem to be in alphabetical order. Can we keep the alphabetical
sorting?

> Source/WebCore/page/Chrome.cpp:518
> +#if !PLATFORM(IOS)
>      m_client.setCursor(cursor);
> +#else
> +    UNUSED_PARAM(cursor);
> +#endif

Seems like this should be a platform-has-no-cursor-ifdef, not an iOS-ifdef.
Maybe ENABLE(CURSOR_SUPPORT) or ENABLE(CURSOR)? Seems there is quite a bit of
code in this patch that would go under that.

> Source/WebCore/page/Chrome.cpp:527
> +#if !PLATFORM(IOS)
>      m_client.setCursorHiddenUntilMouseMoves(hiddenUntilMouseMoves);
> +#else
> +    UNUSED_PARAM(hiddenUntilMouseMoves);
> +#endif

Ditto.

> Source/WebCore/page/ChromeClient.h:46
> +#include "Console.h"
> +#include "GraphicsLayer.h"
> +#include <wtf/HashMap.h>

Surprising that we need these includes in the header.