Summary: | [WK2] Add style support for WebPopUpItem | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antaryami Pandia (apandia) <antaryami.pandia> | ||||||||
Component: | WebKit2 | Assignee: | 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
Antaryami Pandia (apandia)
2012-03-22 02:03:30 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.
Hi Darin, Please provide some feedback. Created attachment 134011 [details]
Patch.
Comment on attachment 134011 [details] Patch. Attachment 134011 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12141487 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 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(....). Created attachment 134228 [details]
Modified patch.
@anders,sam request you to review the patch. The patch is pending since long. It's not a big change. Request some reviewer to review the patch. 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.
|