Bug 90806 – [Qt] Improve the mobile theme slightly

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

[Pierre Rossi]

[Qt] Improve the mobile theme slightly

[Pierre Rossi]

Created attachment 151296 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 151296 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151296&action=review

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

weird placement

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:118
> + *      4      |<--->| 3                    0  |  xÎ + x  |    y

unicode problem?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:129
> +static void addPointToOctants(QVector<QPainterPath> &octants, const QPointF &center, qreal x, qreal y , int xDelta = 0)

no space before , pls

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:131
> +    ASSERT(octants.count() == 8);

newline after assert

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:135
> +        // The Gray code corresponding to the octant's index helps doing the math in a more generic way

why Gray with uppercase? why no punctuation mark at end?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:165
> +    qreal x = 0, y;

not webkit style

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:167
> +    // stay within reasonable distance from edge values, which can cause artifacts at certain zoom levels

missing dot, start with capital

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:169
> +    for (y = radius - epsilon ; y - epsilon > x; y -= 0.5) {

no space before ;

> Source/WebCore/platform/qt/RenderThemeQtMobile.h:59
> +    // We don't want the focus ring to be drawn by the graphics context so we
> +    // always claim to support it in the theme.
> +    // FIXME: This could be a usability problem in the case of contenteditable divs.
> +    virtual bool supportsFocusRing(const RenderStyle*) const { return true; }

what does other ports do? mobile ports

[Pierre Rossi]

Created attachment 151690 [details]
Patch

Fixed style issues pointed out by Kenneth ;)

[Pierre Rossi]

(In reply to comment #2)

> > Source/WebCore/platform/qt/RenderThemeQtMobile.h:59
> > +    // We don't want the focus ring to be drawn by the graphics context so we
> > +    // always claim to support it in the theme.
> > +    // FIXME: This could be a usability problem in the case of contenteditable divs.
> > +    virtual bool supportsFocusRing(const RenderStyle*) const { return true; }
> 
> what does other ports do? mobile ports

Now on that one it's particularly tricky for us because we have this whole WK1 vs. WK2, QStyle vs. hardcoded mobile theme dilemma. When the focus ring for contentEditable divs is drawn by the graphics context, this whole logic fails.
I think the best way to solve this in a clean fashion would be to adopt's chromium approach with PlatformSupport, even for styling, but it's a bit bigger than the scope of this current patch.

[WebKit Review Bot]

Comment on attachment 151690 [details]
Patch

Clearing flags on attachment: 151690

Committed r122647: <http://trac.webkit.org/changeset/122647>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.