Summary: | [Chromium]UI polishes and tweaks to Autofill dropdown menu. | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, dhollowa, fishd, honten, isherman, rniwa, tkent, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Naoki Takano
2011-04-13 19:03:29 PDT
Created attachment 89517 [details]
Patch
Comment on attachment 89517 [details]
Patch
Could you review my patch?
The original bug report is here. http://code.google.com/p/chromium/issues/detail?id=51077 Comment on attachment 89517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89517&action=review I know that a lot of the code in this file is shared by the <select> popup (except on Mac) and by the Autofill popup. I believe we only want these changes to affect the Autofill popup. Have you verified that this is indeed the case? > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:79 > +static const int kLineHeightMargin = 3; // Line height margin put at the top and bottom of each line. nit: This should probably be called "kLineHeightPadding" -- margins would be allowed to overlap, whereas this is additive, like padding. > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:208 > + // Paint an top and bottom padding. nit: "an" -> "the" > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:858 > +void PopupListBox::paintTopBottomPadding(GraphicsContext* gc, const IntRect& rect) nit: Perhaps "paintVerticalPadding"? > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:864 > + Color backColor = m_popupClient->itemStyle(0).backgroundColor(); nit: Prefer to use unabbreviated names -- in this case, "backgroundColor" > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:868 > + topRect.move(0, - kPaddingHeight); nit: Omit the space in "- kPaddingHeight" since this is a unary minus > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:957 > + gc->fillRect(separatorRect, Color(0xcd, 0xcd, 0xcd), ColorSpaceDeviceRGB); nit: I wonder if there's an existing named color for this. It seems a little weird to declare a color constant as a function parameter. I'm not completely up to speed on WebKit style guidelines, though. > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1146 > + return separatorHeight; nit: As mentioned in the Chromium bug, we might want a few pixels of padding around the separator. I'd need to see more screenshots or play with the code to have a beter idea of what exactly we want here, though. (In reply to comment #4) > (From update of attachment 89517 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89517&action=review > > I know that a lot of the code in this file is shared by the <select> popup (except on Mac) and by the Autofill popup. I believe we only want these changes to affect the Autofill popup. Have you verified that this is indeed the case? I verified them, but I assumed the other popups also need to obey this rule. I'll leave the other popups as it is. My plan is 1, Add whole padding and margin information into PopupMenuStyle. 2, Add popup window type into PopupMenuStyle and according to the type, change the margin. I prefer to 1. > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:957 > > + gc->fillRect(separatorRect, Color(0xcd, 0xcd, 0xcd), ColorSpaceDeviceRGB); > > nit: I wonder if there's an existing named color for this. It seems a little weird to declare a color constant as a function parameter. I'm not completely up to speed on WebKit style guidelines, though. There is not #cdcdcd color in Webkit as defined. But as I suggested in bug track comment, I will use #c0c0c0 which is Color::lightGray. That would be better. > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1146 > > + return separatorHeight; > > nit: As mentioned in the Chromium bug, we might want a few pixels of padding around the separator. I'd need to see more screenshots or play with the code to have a beter idea of what exactly we want here, though. As Roma isn't worried about it, so I will leave here. Thanks, Created attachment 89718 [details]
Revised patch.
Please review again.
Once I got ToT, my webkit-patch upload doesn't work anymore because of exception. Now I'm investigating...
So I upload it manually.
Please review again. Created attachment 89722 [details]
Patch
Attachment 89722 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:326: at least two spaces before inline comment [pep8/E261] [5]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 89723 [details]
Remove separator padding at the edges.
Remove separator padding.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 89517 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89517&action=review > > > > I know that a lot of the code in this file is shared by the <select> popup (except on Mac) and by the Autofill popup. I believe we only want these changes to affect the Autofill popup. Have you verified that this is indeed the case? > I verified them, but I assumed the other popups also need to obey this rule. I'll leave the other popups as it is. > > My plan is > 1, Add whole padding and margin information into PopupMenuStyle. > 2, Add popup window type into PopupMenuStyle and according to the type, change the margin. > > I prefer to 1. Long-term, we want to more cleanly separate the Autofill popup code from the <select> popup code -- the current dual-purpose code can be a bit clumsy to update. I agree that (1) is probably a better short term solution that (2) though -- perhaps some WebKit folks more familiar with this code could comment? > As Roma isn't worried about it, so I will leave here. Ok :) Comment on attachment 89723 [details] Remove separator padding at the edges. View in context: https://bugs.webkit.org/attachment.cgi?id=89723&action=review Nearly all of these are fairly opinionated nits -- feel free to ignore ones that seem totally bogus to you. One the whole, this looks pretty good to me, but I am not a WebKit reviewer. > Source/WebCore/ChangeLog:12 > + Put PopupMenuStyle::paddingLineHeightk() at the top and bottom of each line. nit: Stray 'k's in the function names > 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, int paddingHeight, int paddingLineHeight) nit: "listBoxPaddingHeight" and "linePaddingHeight" might be slightly clearer names for these variables. > Source/WebCore/platform/PopupMenuStyle.h:72 > + int m_paddingHeight; // Padding height put at the top and bottom of window. nit: "window" -> "popup" > Source/WebCore/platform/PopupMenuStyle.h:73 > + int m_paddingLineHeight; // Padding line height put at the top and bottom of each line. nit: "Padding line height" -> "Padding height" (you already mention that this is for each line later in the comment) > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:954 > + gc->fillRect(separatorRect, Color::lightGray, ColorSpaceDeviceRGB); Is it possible for a <select> popup to have a separator? If so, again, I think we only want this change for the Autofill popup. (I'm not 100% sure about that though.) > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1352 > + int windowHeight = menuStyle.paddingHeight(); // Top padding heights is added first. nit: remove "heights" > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1366 > + windowHeight += menuStyle.paddingHeight(); // Bottom padding heights is added last. nit: remove "heights" Ilya, Thank you for your great comments every time. (In reply to comment #12) > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:954 > > + gc->fillRect(separatorRect, Color::lightGray, ColorSpaceDeviceRGB); > > Is it possible for a <select> popup to have a separator? If so, again, I think we only want this change for the Autofill popup. (I'm not 100% sure about that though.) Hmm... I'm also not sure we will have separator for other popup windows. At least, we don't have any separator tag for select html tag though. We also can drag the padding value with PopupMenuStyle. But the class is getting bigger... So is there anybody who can review my patch? Especially it is very helpful to let me know if the separator padding width should be configurable or not with PopupMenuStyle. Comment on attachment 89723 [details]
Remove separator padding at the edges.
Can you address the nits?
Sure!! (In reply to comment #15) > (From update of attachment 89723 [details]) > Can you address the nits? To see the screen shot, please refer to http://code.google.com/p/chromium/issues/detail?id=51077 After looking around the source code, I notice itemIsSeparator() might return true even if not AutoFillPopupMenuClient. So it implies other popup window separator might be drawn. Ok, I add one more parameter to PopupMenuStyle and drag it. (In reply to comment #13) > Ilya, > > Thank you for your great comments every time. > > (In reply to comment #12) > > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:954 > > > + gc->fillRect(separatorRect, Color::lightGray, ColorSpaceDeviceRGB); > > > > Is it possible for a <select> popup to have a separator? If so, again, I think we only want this change for the Autofill popup. (I'm not 100% sure about that though.) > Hmm... I'm also not sure we will have separator for other popup windows. At least, we don't have any separator tag for select html tag though. We also can drag the padding value with PopupMenuStyle. But the class is getting bigger... I have some suggestions about layout and separator color. Please see my comments in http://code.google.com/p/chromium/issues/detail?id=51077. Thanks, David. (In reply to comment #19) > I have some suggestions about layout and separator color. Please see my comments in http://code.google.com/p/chromium/issues/detail?id=51077. Created attachment 89915 [details]
Patch
Per David's request, I tweak my patch again. Please review again, David. Thanks, Created attachment 89952 [details]
Patch
Comment on attachment 89952 [details]
Patch
Per Roma's request, I added font size change for label if autofill.
Please review.
(In reply to comment #13) > > Is it possible for a <select> popup to have a separator? If so, again, I think we only want this change for the Autofill popup. (I'm not 100% sure about that though.) > Hmm... I'm also not sure we will have separator for other popup windows. At least, we don't have any separator tag for select html tag though. We also can drag the padding value with PopupMenuStyle. But the class is getting bigger... <select> can have an <hr> child, and the <hr> is rendered as a separator though the HTML parser doesn't support this structure. (HTML5 parser regression??) Comment on attachment 89952 [details] Patch 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) > 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. > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1000 > + d.setComputedSize(d.computedSize()*0.8); Add spaces around "*" Kent-san, Thank you for your review. (In reply to comment #26) > (From update of attachment 89952 [details]) > 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. I assumed any default parameter is prohibited. I believe it it true in Chromium project, but is WetKit different? > enum PopupMenuType { SelectPopup, AutofillPopup }; > PopupMenuStyle(const Color&...., PopupMenuType menuType = SelectPopup) > > > 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, Comment on attachment 89952 [details] Patch 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. Created attachment 90130 [details]
Patch
Comment on attachment 90130 [details]
Patch
Please review again.
About 'k' prefix problem, I'll be in charge of it next time...
Comment on attachment 90130 [details]
Patch
Looks ok.
Roma requests very tiny parameter tuning, maybe 0.8 to 0.9. Once the parameter is fixed, I let you know. Thanks, Created attachment 90301 [details]
Patch
Comment on attachment 90301 [details]
Patch
Kent-san,
Sorry for bothering you again.
But I just changed 0.8 to 0.9 ratio for label font.
Please commit if it's ok.
Thanks,
Comment on attachment 90301 [details]
Patch
ok
Comment on attachment 90301 [details] Patch Clearing flags on attachment: 90301 Committed r84342: <http://trac.webkit.org/changeset/84342> All reviewed patches have been landed. Closing bug. |