[webkit-reviews] review granted: [Bug 42232] Make changing Cursors work in WebKit2. : [Attachment 61533] Patch 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 42232: Make changing Cursors work in WebKit2.
https://bugs.webkit.org/show_bug.cgi?id=42232

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    switch (type) {
> +	   case Cursor::Pointer:
> +	       return pointerCursor();

Wrong indentation style.

> +const char* nameForCursorType(Cursor::Type type)
> +{
> +    switch (type) {
> +	   case Cursor::Pointer:
> +	       return "Pointer";

Same.

> +const Cursor& pointerCursor()
> +{
> +    DEFINE_STATIC_LOCAL(Cursor, c, (Cursor::Pointer));
> +    return c;
> +}

How about using the word "cursor" or "staticLocalCursor" instead of "c"?

> +    switch (m_type) {
> +	   case Cursor::Pointer:
> +	       m_platformCursor = HardRetain([NSCursor arrowCursor]);
> +	       break;

Switch style again.

> +	       m_platformCursor = HardRetain(leakNamedCursor("crossHairCursor",
11, 11));

Given the word leak in the name of this function, it seems wrong or unnecessary
to do another HardRetain.

> +    switch (m_type) {
> +	   case Cursor::Pointer:

Another switch statement.

> +    LRESULT onSetCursor(HWND hWnd, UINT message, WPARAM, LPARAM, bool&
handled);

Why does the HWND argument need names in all of these?

Otherwise looks good.