Bug 67773 – [Qt] Remove the QStyle dependency in Qt's mobile theme

• Component: New Bugs
• Status: RESOLVED FIXED

[Pierre Rossi]

[Qt] Remove the QStyle dependency in Qt's mobile theme

[Pierre Rossi]

Created attachment 106720 [details]
Patch

[Andreas Kling]

Comment on attachment 106720 [details]
Patch

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

r=me with nits.

> Source/WebCore/ChangeLog:6
> +        GraphicsContext3DQt doesn't actually use QStyleOption

use -> need

> Source/WebCore/ChangeLog:10
> +        No new tests as there should be no functionnal change.

functionnal -> functional

[Pierre Rossi]

Created attachment 106723 [details]
Patch

[WebKit Review Bot]

Attachment 106723 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/qt/RenderThemeQStyle.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pierre Rossi]

Created attachment 106725 [details]
More readable version (still pretty huge)

This is just a more review-friendly version of the patch above since it takes the "git mv" into account.

[Early Warning System Bot]

Comment on attachment 106723 [details]
Patch

Attachment 106723 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9615754

[Pierre Rossi]

Created attachment 106751 [details]
Patch

[WebKit Review Bot]

Attachment 106751 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/qt/RenderThemeQStyle.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pierre Rossi]

Created attachment 106752 [details]
Review-friendly version.

[Laszlo Gombos]

I think we should consider making the mobile theme (perhaps we should find a better name) the default and having an option to turn on QStyle. Mobile theme should be available anywhere where Qt runs, while QStyle might end up being an optional Qt class in Qt5.

This would involve change the name of the build option and the guard names and adjusting the LayotuTests results to default to the mobile theme. I'm not sure if this change needs to be part of this patch/bug, but I find this discussion relevant here.

[Simon Hausmann]

Comment on attachment 106751 [details]
Patch

Clearing review and taking this out of the review queue as the patch is going to change soon :)

[Pierre Rossi]

Created attachment 114665 [details]
Patch

So here's a new approach that doesn't require a compile time decision. It has the benefit of allowing us to still use QStyle for the WebKit1 API and potentially for the desktop webview.

[Simon Hausmann]

Comment on attachment 114665 [details]
Patch

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

> Source/WebCore/html/HTMLSelectElement.h:203
> +    if (!qgetenv("QT_WEBKIT_USE_MOBILE_THEME").isNull())

Initial feedback: It would be nice to centralize the qgetenv() calls into for example a class method in RenderThemeQt.h.

[Pierre Rossi]

Created attachment 114674 [details]
Patch

[Pierre Rossi]

Created attachment 114691 [details]
Patch

[Simon Hausmann]

Comment on attachment 114691 [details]
Patch

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

r=me with the proposed changes.

> Source/WebCore/Target.pri:-25
> -contains(DEFINES, WTF_USE_QT_MOBILE_THEME=1) {
> -    DEFINES += ENABLE_NO_LISTBOX_RENDERING=1
> -}

IT looks like that we may be able to replace the NO_LISTBOX_RENDERING feature define with PLATFORM(QT) if we want to.

> Source/WebCore/html/HTMLSelectElement.h:208
> +#if PLATFORM(QT)
> +    if (RenderThemeQt::useMobileTheme())
> +#endif
>      return true;

I think this is easier to read if the PLATFORM(QT) branch had its own return statement.

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:400
> +#else
> +        qputenv("QT_WEBKIT_USE_MOBILE_THEME", QByteArray("1"));
> +#endif

This should be removed, useMobileTheme() should return true if we don't have QStyle.

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:570
> +void RenderThemeQtMobile::adjustMenuListStyle(CSSStyleSelector*, RenderStyle* style, Element*) const
> +{
> +    style->resetBorder();
> +
> +    // Height is locked to auto.
> +    style->setHeight(Length(Auto));
> +
> +    // White-space is locked to pre
> +    style->setWhiteSpace(PRE);
> +
> +    computeSizeBasedOnStyle(style);
> +
> +    // Add in the padding that we'd like to use.
> +    setPopupPadding(style);
> +    style->setPaddingLeft(Length(menuListPadding, Fixed));
> +}

It would be nicer if the QtMobileImplementation would call the base class implementation and then do the padding adjustment. This can be fixed in a separate patch.

> Source/WebKit/qt/Api/qwebpage.cpp:3276
> +    if (qgetenv("QT_WEBKIT_USE_MOBILE_THEME").isNull()) {

I think this should be replaced with a call to RenderThemeQt::useMobileTheme().

[Kent Tamura]

Comment on attachment 114691 [details]
Patch

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

> Source/WebCore/html/HTMLSelectElement.h:210
>  #if ENABLE(NO_LISTBOX_RENDERING)
> +#if PLATFORM(QT)
> +    if (RenderThemeQt::useMobileTheme())
> +#endif
>      return true;
> -#else
> -    return !m_multiple && m_size <= 1;
>  #endif
> +    return !m_multiple && m_size <= 1;

We should introduce a function like usesMenuListFor(const HTMLSelectElement*) to RenderTheme, and remove NO_LISTBOX_RENDERING.
RenderThemeQtMobile should return true from this function.

[Simon Hausmann]

(In reply to comment #17)
> (From update of attachment 114691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114691&action=review
> 
> > Source/WebCore/html/HTMLSelectElement.h:210
> >  #if ENABLE(NO_LISTBOX_RENDERING)
> > +#if PLATFORM(QT)
> > +    if (RenderThemeQt::useMobileTheme())
> > +#endif
> >      return true;
> > -#else
> > -    return !m_multiple && m_size <= 1;
> >  #endif
> > +    return !m_multiple && m_size <= 1;
> 
> We should introduce a function like usesMenuListFor(const HTMLSelectElement*) to RenderTheme, and remove NO_LISTBOX_RENDERING.
> RenderThemeQtMobile should return true from this function.

Yes, good idea! We'll do that separately when removing the NO_LISTBOX_RENDERING define altogether.

[Simon Hausmann]

Landed with qgetenv usage fixed and minor instantiation fix in themeForPage

Committed r100123: <http://trac.webkit.org/changeset/100123>