[webkit-reviews] review denied: [Bug 74293] [Qt] Mobile theme could use a little refresh : [Attachment 118786] Patch

[bugzilla-daemon at webkit.org]

Pierre Rossi <pierre.rossi at gmail.com> has denied Pierre Rossi
<pierre.rossi at gmail.com>'s request for review:
Bug 74293: [Qt] Mobile theme could use a little refresh
https://bugs.webkit.org/show_bug.cgi?id=74293

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

------- Additional Comments from Pierre Rossi <pierre.rossi at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118786&action=review


>> Source/WebCore/css/mobileThemeQt.css:30
>> +}
> 
> What about using Nokia font if available?

@Kenneth: that's a actually not a bad idea at all... ;)

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:100
>> +static inline QRect squareFromRect(const QRect& rect)
> 
> expandRectToSquare ?

Actually it shrinks, so I'd probably go with shrinkRectToSquare. Your comment
makes it clear that the name is not clear enough :)

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:116
>> +	    painter->setRenderHint(QPainter::SmoothPixmapTransform);
> 
> is old stored somewhere? or could this be two lines? what about adding a
comment and also I dislike old* :-) previous sounds more clear

I basically named it that to be consistent with what was already there in
stylePainter (oldBrush and oldAntiAliasing).

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:171
>> +	if (true || !QPixmapCache::find(key, result)) {
> 
> true || ?

duh, I should really read things again sometimes. Remainders from testing
something...

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:190
>> +	const int border = size.width()/ 4;
> 
> Coding style: " / " :)

Interesting that this wasn't caught by check-webkit-style...

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:213
>> +//	  painter->fillRect(QRect(QPoint(0,0), size), Qt::red);
> 
> Uh?

yup, I should definitely have proof-read that...

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:226
>> +	const int width = size.width();
> 
> why do you do with this width and not height which you use multiple places

true. Should have done that when I changed the spacing to not be constant.

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:267
>> +	    + QLatin1String("-") + (enabled ? QLatin1String("on") :
QLatin1String("off"));
> 
> You seem to do this multiple places? maybe we need a dedicated method for
doing this

While I agree there seems to be a pattern, it's done in 4 places with a
different number and type of arguments. I'm just not sure it's worth
over-engineering...