Summary: | [Chromium]Mac UI polish to add left/right padding space for autofill popup window. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, dhollowa, eric, honten, isherman, mark, thakis | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Naoki Takano
2011-04-19 20:58:10 PDT
Created attachment 90299 [details]
Patch
Can you please upload before and after screenshots to (e.g.) dropmocks.com and paste the link here? Sure, Just a moment... (In reply to comment #2) > Can you please upload before and after screenshots to (e.g.) dropmocks.com and paste the link here? Here it is, http://dropmocks.com/mUKYr Now I specify 4px like Linux and Win platforms. (In reply to comment #3) > Sure, > > Just a moment... > > (In reply to comment #2) > > Can you please upload before and after screenshots to (e.g.) dropmocks.com and paste the link here? LGTM as far as I can tell (I think we use text popups only for autofill, not for dropdown menus, on mac). I'm not very familiar with the theme stuff – Eric is, and he's a reviewer too, so I'm ccing him. Eric, can you take a look? It's a short patch. Comment on attachment 90299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90299&action=review Seems OK to me besides the nits below. > Source/WebCore/ChangeLog:9 > + No new test because Chromium Autofill popup window doesn't have any test framework. Thank you for explaining. > Source/WebCore/ChangeLog:12 > + The code should be removed after Autofill popup window logic is separated from WebKit to Chromium. Can you provide more context? like a bug link? > Source/WebCore/rendering/RenderThemeChromiumMac.h:59 > + int popupInternalPaddingLeft(RenderStyle*) const; > + int popupInternalPaddingRight(RenderStyle*) const; Aren't these virtual calls? (we always mark virtual calls as virtual even though it's not strictly required by the spec). The corresponding Chromium bug is: http://crbug.com/51077 This is a follow-up to WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=58505 (In reply to comment #6) > (From update of attachment 90299 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90299&action=review > > Seems OK to me besides the nits below. > > > Source/WebCore/ChangeLog:9 > > + No new test because Chromium Autofill popup window doesn't have any test framework. > > Thank you for explaining. > > > Source/WebCore/ChangeLog:12 > > + The code should be removed after Autofill popup window logic is separated from WebKit to Chromium. > > Can you provide more context? like a bug link? > > > Source/WebCore/rendering/RenderThemeChromiumMac.h:59 > > + int popupInternalPaddingLeft(RenderStyle*) const; > > + int popupInternalPaddingRight(RenderStyle*) const; > > Aren't these virtual calls? (we always mark virtual calls as virtual even though it's not strictly required by the spec). Comment on attachment 90299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90299&action=review > Source/WebCore/ChangeLog:10 > + This assumes only AutofillPopupMenuClient gives TexfieldPart appearance. nit: "Texfield" -> "TextField" > Source/WebCore/rendering/RenderThemeChromiumMac.mm:83 > + nit: "autofillPopupHorizontalPadding" might be a better name for this variable -- your call > Source/WebCore/rendering/RenderThemeChromiumMac.mm:85 > +// We assume only AutofillPopupMenuClient gives TexfieldPart appearance here. nit: "Texfield" -> "TextField" Thank you for review, guys!! (In reply to comment #6) > (From update of attachment 90299 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90299&action=review > > Source/WebCore/ChangeLog:12 > > + The code should be removed after Autofill popup window logic is separated from WebKit to Chromium. > > Can you provide more context? like a bug link? I already included the link in ChangeLog as following, + [Chromium]Mac UI polish to add left/right padding space for autofill popup window. + https://bugs.webkit.org/show_bug.cgi?id=58960 + http://code.google.com/p/chromium/issues/detail?id=51077 Do I have to add more extra info? > > > Source/WebCore/rendering/RenderThemeChromiumMac.h:59 > > + int popupInternalPaddingLeft(RenderStyle*) const; > > + int popupInternalPaddingRight(RenderStyle*) const; > > Aren't these virtual calls? (we always mark virtual calls as virtual even though it's not strictly required by the spec). Yes, you are right. I'll add them. Created attachment 90470 [details]
Patch
Comment on attachment 90470 [details]
Patch
Could you please review again?
Nit: const implicitly has internal linkage, so you can drop the static in front of it. Created attachment 90648 [details]
Patch
Comment on attachment 90648 [details]
Patch
Erased static.
Please review again.
Comment on attachment 90648 [details]
Patch
This can land with Eric's r+.
Comment on attachment 90648 [details] Patch Rejecting attachment 90648 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 1 Last 500 characters of output: pe=xml Processing 1 patch from 1 bug. Cleaning working directory Updating working directory Processing patch 90648 from bug 58960. NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Updating OpenSource Current branch master is up to date. Full output: http://queues.webkit.org/results/8495324 Comment on attachment 90648 [details]
Patch
Eric, could you r+?
Comment on attachment 90648 [details] Patch Clearing flags on attachment: 90648 Committed r84640: <http://trac.webkit.org/changeset/84640> All reviewed patches have been landed. Closing bug. |