[Webkit-unassigned] [Bug 58505] [Chromium]UI polishes and tweaks to Autofill dropdown menu.

[bugzilla-daemon at webkit.org]

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





--- Comment #28 from Kent Tamura <tkent at chromium.org>  2011-04-18 00:25:17 PST ---
(From update of attachment 89952)
View in context: https://bugs.webkit.org/attachment.cgi?id=89952&action=review

>>> Source/WebCore/platform/PopupMenuStyle.h:38
>>> +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, bool isAutofillStyle)
>> 
>> Please avoid new bool parameter.  We prefer introducing enum. Also, we can avoid updating RenderMenuList and RenderTextConstrolSingleLine by a default parameter.
>> 
>> enum PopupMenuType { SelectPopup, AutofillPopup };
>> PopupMenuStyle(const Color&...., PopupMenuType menuType = SelectPopup)
> 
> I assumed any default parameter is prohibited.
> I believe it it true in Chromium project, but is WetKit different?

Yes. WebKit is not Chromium. You can see a lot of default parameter usages in WebKit.

>>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:78
>>> +static const int kLinePaddingHeight = 3; // Padding height put at the top and bottom of each line.
>> 
>> Do not use "k" prefix for constant values.
> 
> I referred to
>  static const int kLabelToIconPadding = 5;
>  static const int kMinEndOfLinePadding = 2;
> 
> They start with "k", but do I have change "kLinePaddingHeight" to "linePaddingHeight"?
> 
> Thanks,

It's ok to keep "k" if you think consistency in this file is more important than consistency in WebKit.
Anyway, I expect someone makes a patch to remove all of "k" prefixes in PopupMenuChromium.cpp and ScrollbarThemeChromiumWin.cpp later.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.