Bug 58960 – [Chromium]Mac UI polish to add left/right padding space for autofill popup window.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

[Chromium]Mac UI polish to add left/right padding space for autofill popup window.

[Naoki Takano]

Created attachment 90299 [details]
Patch

[Nico Weber]

Can you please upload before and after screenshots to (e.g.) dropmocks.com and paste the link here?

[Naoki Takano]

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?

[Naoki Takano]

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?

[Nico Weber]

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.

[Eric Seidel (no email)]

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).

[David Holloway]

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).

[Ilya Sherman]

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"

[Naoki Takano]

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.

[Naoki Takano]

Created attachment 90470 [details]
Patch

[Naoki Takano]

Comment on attachment 90470 [details]
Patch

Could you please review again?

[Nico Weber]

Nit: const implicitly has internal linkage, so you can drop the static in front of it.

[Naoki Takano]

Created attachment 90648 [details]
Patch

[Naoki Takano]

Comment on attachment 90648 [details]
Patch

Erased static.

Please review again.

[Nico Weber]

Comment on attachment 90648 [details]
Patch

This can land with Eric's r+.

[WebKit Commit Bot]

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

[Naoki Takano]

Comment on attachment 90648 [details]
Patch

Eric, could you r+?

[WebKit Commit Bot]

Comment on attachment 90648 [details]
Patch

Clearing flags on attachment: 90648

Committed r84640: <http://trac.webkit.org/changeset/84640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.