[webkit-reviews] review denied: [Bug 28921] Tablet Input Panel Icon does not show up by text fields : [Attachment 38943] Fix

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 28921: Tablet Input Panel Icon does not show up by text fields
https://bugs.webkit.org/show_bug.cgi?id=28921

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +#if PLATFORM(WIN)
> +void Frame::updateWindowsSystemCaret()

This function should go in FrameWin.cpp.

> +{
> +    ::DestroyCaret();
> +
> +    Page* page = document()->page();
> +    if (!page || !page->chrome()->platformWindow())
> +	   return;
> +
> +    AccessibilityObject* focusedObject =
document()->axObjectCache()->focusedUIElementForPage(page);
> +    if (!focusedObject || !focusedObject->isTextControl())
> +	   return;

It seems strange to use the accessibility code for this. Can we figure this out
some other way?

> +    PlatformWidget widget = page->chrome()->platformWindow();
> +    if (!widget)
> +	   return;
> +
> +    HBITMAP caretBitmap = ::CreateBitmap(1, 1, 1, 1, NULL);

Can we use a 0x0 bitmap instead of a 1x1 bitmap? It seems like a 1x1 bitmap
will make 1 pixel on the screen blink. MSDN seems to indicate that a 0x0 bitmap
should work.

> +    ::CreateCaret(widget, caretBitmap, 1, 1);
> +    ::ShowCaret(widget);
> +
> +    IntRect rect = selection()->absoluteCaretBounds();
> +
> +    ::SetCaretPos(rect.topLeft().x() - view()->scrollX(),
> +	   rect.topLeft().y() - view()->scrollY());

There's no need for the .topLeft() calls here. Can we use something like
contentsToWindow here?

MSDN says multiple times that "A window should create a caret only when it has
the keyboard focus or is active. The window should destroy the caret before
losing the keyboard focus or becoming inactive." Does your code guarantee this?