Bug 81883 – [WK2] Add style support for WebPopUpItem

• Status: UNCONFIRMED
• Component: WebKit2

[Antaryami Pandia (apandia)]

While working on issue "https://bugs.webkit.org/show_bug.cgi?id=81882" found that the style is not yet implemented for WebPopUpItem.

[Antaryami Pandia (apandia)]

Created attachment 133208 [details]
WIP

Attaching the WIP patch to get some feedback.The approach I am taking is:-
Adding a new struct "WebPopupItemStyle" to hold the style information and then passing it to WebPopupItem.

Wanted the below feedback before proceeding further:-
Is the WIP approach is Ok or should I add all style info to the WebPopupItem itself.

[Antaryami Pandia (apandia)]

Hi Darin,
Please provide some feedback.

[Antaryami Pandia (apandia)]

Created attachment 134011 [details]
Patch.

[Build Bot]

Comment on attachment 134011 [details]
Patch.

Attachment 134011 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12141487

[Antaryami Pandia (apandia)]

Comment on attachment 134011 [details]
Patch.

Mac throws below compilation error:-

WebPopupMenuProxyMac.mm:73: error: 'const struct WebKit::WebPopupItem' has no member named 'm_textDirection'
WebPopupMenuProxyMac.mm:80: error: 'const struct WebKit::WebPopupItem' has no member named 'm_hasTextDirectionOverride'

I will resolve these errors in a new patch along with review comments provided by you.

Marking the "r?", to get review comments.

[Alexis Menard (darktears)]

Comment on attachment 134011 [details]
Patch.

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

r- to me.

> Source/WebKit2/GNUmakefile.am:441
> +        Source/WebKit2/Shared/WebPopupItemStyle.cpp \

Indentation issue.

> Source/WebKit2/Shared/WebPopupItem.cpp:-33
> -using namespace WebCore;

You need to keep that.

> Source/WebKit2/Shared/WebPopupItemStyle.h:45
> +    WebPopupItemStyle(bool visible, bool isDisplayNone, WebCore::TextDirection, bool hasTextDirectionOverride);

The names of the parameters are not adding anything in the .h file, remove them

> Source/WebKit2/WebProcess/WebCoreSupport/WebPopupMenu.cpp:92
> +            WebPopupItemStyle style = WebPopupItemStyle(itemStyle.isVisible(), itemStyle.isDisplayNone(), itemStyle.textDirection(), itemStyle.hasTextDirectionOverride());

Lot of copies no? The affection and then in WebPopupItem constructor it will copy. Could you save one at least? WebPopupItemStyle style(....).

[Antaryami Pandia (apandia)]

Created attachment 134228 [details]
Modified patch.

[Antaryami Pandia (apandia)]

@anders,sam
request you to review the patch.

[Antaryami Pandia (apandia)]

The patch is pending since long. It's not a big change.
Request some reviewer to review the patch.

[Anders Carlsson]

Comment on attachment 134228 [details]
Modified patch.

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.