[Webkit-unassigned] [Bug 91952] [EFL]Not to adjust height of buttons

[bugzilla-daemon at webkit.org]

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


Raphael Kubo da Costa (rakuco) <rakuco at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #153715|                            |commit-queue-
               Flag|                            |




--- Comment #6 from Raphael Kubo da Costa (rakuco) <rakuco at webkit.org>  2012-07-22 20:15:47 PST ---
(From update of attachment 153715)
View in context: https://bugs.webkit.org/attachment.cgi?id=153715&action=review

Thanks for the patch. I recommend doing some research before working on a patch like this -- checking other ports' implementations of RenderTheme::adjustButtonStyle(), I came across GTK+'s, added in r69436. That revision, on its turn, points to an existing layout test that has a similar purpose to the one you have added, and which we are already skipping because it currently fails. And checking LayoutTests/platform/efl/TestExpectations, it is possible to see that the issue is being tracked in bug 85494.

I thus suggest closing this bug as a duplicate of bug 85494, checking if the existing test is enough to cover this change and sending a patch there.

> Source/WebCore/ChangeLog:3
> +        [EFL]Not to adjust height of buttons

Nit: we normally add a space between '[Category]' and the rest of the description. Plus "Not to adjust" doesn't look right, "Do not adjust" sounds better.

> Source/WebCore/ChangeLog:8
> +        The style for a width and a height should be applyed also for a button element.

s/applyed/applied/.

> Source/WebCore/ChangeLog:10
> +        The code should be removed because on other ports, the force height setting code for Buttons not exists.

Nitpicking a little on this reasoning, just because other ports don't do that isn't a good explanation; you should rather explain why it does not make sense to use Auto here.

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