Bug 81883 - [WK2] Add style support for WebPopUpItem
Summary: [WK2] Add style support for WebPopUpItem
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 81882
  Show dependency treegraph
 
Reported: 2012-03-22 02:03 PDT by Antaryami Pandia (apandia)
Modified: 2014-02-05 10:52 PST (History)
9 users (show)

See Also:


Attachments
WIP (10.96 KB, patch)
2012-03-22 02:07 PDT, Antaryami Pandia (apandia)
no flags Details | Formatted Diff | Diff
Patch. (19.11 KB, patch)
2012-03-27 03:01 PDT, Antaryami Pandia (apandia)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Modified patch. (20.84 KB, patch)
2012-03-28 00:25 PDT, Antaryami Pandia (apandia)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.