[webkit-reviews] review granted: [Bug 74727] [Qt] Mobile theme refinements : [Attachment 119629] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Pierre Rossi
<pierre.rossi at gmail.com>'s request for review:
Bug 74727: [Qt] Mobile theme refinements
https://bugs.webkit.org/show_bug.cgi?id=74727

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119629&action=review


> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:99
> +    // Translating the painter has a tendency to mess with gradients.

The comment is not clear. So you do thing differently here because of that, or
something that should be fixed later? I think the comment could make that more
clear

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:129
> +	   return 0;

Can't a 0 mess things up? or is that considered invalid?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:271
> +    QPolygon upArrow, downArrow;
> +    upArrow << QPoint(0, arrowHeight) << QPoint(arrowHeight, 0) <<
QPoint(right, arrowHeight);
> +    downArrow << QPoint(0, bottomBaseline) << QPoint(arrowHeight,
bottomBaseline + arrowHeight)
> +		 << QPoint(right, bottomBaseline);

Is this more effective? I think you can mention such changes in the changelog

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:680
> +	   }
> +
> +    } else

unneeded newline

> Source/WebCore/platform/qt/RenderThemeQtMobile.h:113
> +    QPixmap findLineEdit(const QSize&, bool focused) const;

Maybe we should keep the names closer to the html form counterparts?