Bug 90806 - [Qt] Improve the mobile theme slightly
Summary: [Qt] Improve the mobile theme slightly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pierre Rossi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-09 11:10 PDT by Pierre Rossi
Modified: 2012-07-13 17:16 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.64 KB, patch)
2012-07-09 12:29 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (8.64 KB, patch)
2012-07-11 05:26 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Rossi 2012-07-09 11:10:40 PDT
[Qt] Improve the mobile theme slightly
Comment 1 Pierre Rossi 2012-07-09 12:29:04 PDT
Created attachment 151296 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-07-10 22:19:57 PDT
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
Comment 3 Pierre Rossi 2012-07-11 05:26:28 PDT
Created attachment 151690 [details]
Patch

Fixed style issues pointed out by Kenneth ;)
Comment 4 Pierre Rossi 2012-07-11 05:32:15 PDT
(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.
Comment 5 WebKit Review Bot 2012-07-13 17:15:57 PDT
Comment on attachment 151690 [details]
Patch

Clearing flags on attachment: 151690

Committed r122647: <http://trac.webkit.org/changeset/122647>
Comment 6 WebKit Review Bot 2012-07-13 17:16:01 PDT
All reviewed patches have been landed.  Closing bug.