[webkit-reviews] review denied: [Bug 114862] [EFL][WK2]<select> element's text is clipped when a height is specified along with CSS line-height. : [Attachment 201546] Patch

[bugzilla-daemon at webkit.org]

Gyuyoung Kim <gyuyoung.kim at samsung.com> has denied Rashmi Kulakarni
<rashmi.vijay at samsung.com>'s request for review:
Bug 114862: [EFL][WK2]<select> element's text is clipped when a height is
specified along with CSS line-height.
https://bugs.webkit.org/show_bug.cgi?id=114862

Attachment 201546: Patch
https://bugs.webkit.org/attachment.cgi?id=201546&action=review

------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201546&action=review


It looks this patch looks like work-around for specific cases. I think you need
to find more general solution.

> Source/WebCore/ChangeLog:3
> +	   [EFL][WK2]<select> element's text is clipped when a height is
specified along with CSS line-height.

I think this patch is for both EFL WK1 and WK2. We haven't used [WK2] prefix
when patch only touch WebCore/platform/efl.

> Source/WebCore/ChangeLog:7
> +

Missing patch description.

> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

You should mention test cases which covered by this patch. If there is no new
test, you can said there is no new test or remove this line.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:856
> +    // Case 1:We need to apply theme specific values for padding only when
the top padding padding goes beyond theme padding value defined

AFAIK, Case 1, 2 isn't WebKit style. No need.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:858
> +    if ((style->paddingTop().intValue() > desc->padding.top().intValue() && 
style->appearance() == MenulistButtonPart) || (style->paddingTop().intValue() >
desc->padding.top().intValue() &&  style->appearance() == MenulistPart ))

Too long line. line breaking is needed.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:860
> +    // Case 2:We need to handle scenarios when a particular webkit style is
sepcified.

ditto.