Bug 81883

Summary: [WK2] Add style support for WebPopUpItem
Product: WebKit Reporter: Antaryami Pandia (apandia) <antaryami.pandia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: aestes, andersca, darin, menard, mitz, rakuco, sam, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 81882    
Attachments:
Description Flags
WIP
none
Patch.
buildbot: commit-queue-
Modified patch. none

Description Antaryami Pandia (apandia) 2012-03-22 02:03:30 PDT
While working on issue "https://bugs.webkit.org/show_bug.cgi?id=81882" found that the style is not yet implemented for WebPopUpItem.
Comment 1 Antaryami Pandia (apandia) 2012-03-22 02:07:52 PDT
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.
Comment 2 Antaryami Pandia (apandia) 2012-03-26 04:22:21 PDT
Hi Darin,
Please provide some feedback.
Comment 3 Antaryami Pandia (apandia) 2012-03-27 03:01:52 PDT
Created attachment 134011 [details]
Patch.
Comment 4 Build Bot 2012-03-27 04:34:35 PDT
Comment on attachment 134011 [details]
Patch.

Attachment 134011 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12141487
Comment 5 Antaryami Pandia (apandia) 2012-03-27 04:53:42 PDT
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.
Comment 6 Alexis Menard (darktears) 2012-03-27 13:15:22 PDT
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(....).
Comment 7 Antaryami Pandia (apandia) 2012-03-28 00:25:13 PDT
Created attachment 134228 [details]
Modified patch.
Comment 8 Antaryami Pandia (apandia) 2012-04-03 02:56:08 PDT
@anders,sam
request you to review the patch.
Comment 9 Antaryami Pandia (apandia) 2012-07-18 23:16:49 PDT
The patch is pending since long. It's not a big change.
Request some reviewer to review the patch.
Comment 10 Anders Carlsson 2014-02-05 10:52:31 PST
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.