[Webkit-unassigned] [Bug 141704] [Win] Popup menu is not accessible.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=141704

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #251459|review?                     |review-
              Flags|                            |

--- Comment #7 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 251459
  --> https://bugs.webkit.org/attachment.cgi?id=251459
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251459&action=review

Fantastic! I'm so excited you have this working (as I'm sure James Craig will be, too). I don't think it's quite ready to land, but it's almost there.

Are there any tests we can unskip with this fix in place?

> Source/WebCore/platform/win/PopupMenuWin.cpp:756
> +    if (lParam != OBJID_CLIENT)

This should be written as "if (static_cast<LONG>(lParam) != OBJID_CLIENT)"

See <https://bugs.webkit.org/show_bug.cgi?id=141391>

> Source/WebCore/platform/win/PopupMenuWin.cpp:1155
> +{

I like to check for null, and return E_POINTER if it's not valid.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1161
> +{

Ditto E_POINTER.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1176
> +{

E_POINTER if value is NULL.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1186
> +    *value = SysAllocString(itemText.characters16());

You might see if the BString class provides this functionality in a single function call.

BString itemText(m_popupMenu.client()->itemText(index));
*value = itemText.release();

> Source/WebCore/platform/win/PopupMenuWin.cpp:1197
> +{

E_POINTER

> Source/WebCore/platform/win/PopupMenuWin.cpp:1214
> +{

E_POINTER

> Source/WebCore/platform/win/PopupMenuWin.cpp:1255
> +{

E_POINTER for null 'left', 'top', etc.

> Source/WebCore/platform/win/PopupMenuWin.cpp:1324
> +        return E_POINTER;

Yay! :-)

> Source/WebCore/platform/win/ScrollbarThemeWin.h:38
> +    virtual int scrollbarThickness(ScrollbarControlSize = RegularScrollbar) override;

We have been removing the 'virtual' when we use 'override'.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:40
> +    virtual void themeChanged() override;

Ditto.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:46
> +    virtual IntRect trackRect(ScrollbarThemeClient*, bool painting = false) override;

Ditto.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:53
> +    virtual bool shouldSnapBackToDragOrigin(ScrollbarThemeClient*, const PlatformMouseEvent&) override;

Ditto.

> Source/WebCore/platform/win/ScrollbarThemeWin.h:58
> +    virtual void paintThumb(GraphicsContext*, ScrollbarThemeClient*, const IntRect&) override;

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150423/b30db950/attachment.html>