[Qt][Symbian] Fix Api test failure -- tst_QWebView::setPalette()

• Component: WebKit API
• Status: RESOLVED FIXED

[Yi Shen]

The tst_QWebView::setPalette() is used to test whether we can use QWebView.setPalette() to specify the highlight color for the web page. This test fails on symbian because RenderThemeQt doesn't use the customized palette if USE_QT_MOBILE_THEME is defined. (See the code below)

void RenderThemeQt::setPaletteFromPageClientIfExists(QPalette& palette) const
{
#if USE(QT_MOBILE_THEME)
    static QPalette lightGrayPalette(Qt::lightGray);
    palette = lightGrayPalette;
    return;
#endif
    // If the webview has a custom palette, use it
    if (!m_page)
        return;
    Chrome* chrome = m_page->chrome();
    if (!chrome)
        return;
    ChromeClient* chromeClient = chrome->client();
    if (!chromeClient)
        return;
    QWebPageClient* pageClient = chromeClient->platformPageClient();
    if (!pageClient)
        return;
    palette = pageClient->palette();
}

[Yi Shen]

Hi Eric, could you please put some comments for this test failure because I saw you are the person who added this logic :) I am not clear why we can't use the customized palette for the mobile platform. To fix this test failure, either we change the RenderThemeQt to allow it use customized palette, or exclude the this test for mobile platform. Thanks

[Eric Seidel (no email)]

I suspect you'll find if you look at the ChangeLog that I did not add the logic, rather I just committed someone else's patch.

[Yi Shen]

Sorry Eric, I think Luiz Agostini might be the right person to ask :) 

Hi Luiz, could you please put some comments for this issue, thanks.

[Luiz Agostini]

(In reply to comment #3)
> Sorry Eric, I think Luiz Agostini might be the right person to ask :) 

Sorry yi shen, but I am not who you are looking for. :)

> 
> Hi Luiz, could you please put some comments for this issue, thanks.

Those lines have been introduced by r54180 (http://trac.webkit.org/changeset/54180). What I did was to replace the #ifdef Q_WS_MAEMO_5 by the #if USE(QT_MOBILE_THEME) in r58788 (http://trac.webkit.org/changeset/58788) because it was decided at that time that what we had done for MAEMO 5 would became our mobile theme.

[Yi Shen]

(In reply to comment #4)
> (In reply to comment #3)
> > Sorry Eric, I think Luiz Agostini might be the right person to ask :) 
> 
> Sorry yi shen, but I am not who you are looking for. :)
> 
> > 
> > Hi Luiz, could you please put some comments for this issue, thanks.
> 
> Those lines have been introduced by r54180 (http://trac.webkit.org/changeset/54180). What I did was to replace the #ifdef Q_WS_MAEMO_5 by the #if USE(QT_MOBILE_THEME) in r58788 (http://trac.webkit.org/changeset/58788) because it was decided at that time that what we had done for MAEMO 5 would became our mobile theme.

Thanks Luiz :)

Kling, could you take a look at this issue? I think it is yours change :)

[Yi Shen]

Created attachment 88653 [details]
skips this test for symbian

I will check the meego also once I got the meego environment.

[Kenneth Rohde Christiansen]

Comment on attachment 88653 [details]
skips this test for symbian

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

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:44
> +#if !defined(Q_OS_SYMBIAN)

Wouldnt it be better to check the USE(Qt_MOBILE_THEME) here?

[Yi Shen]

(In reply to comment #7)
> (From update of attachment 88653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88653&action=review
> 
> > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:44
> > +#if !defined(Q_OS_SYMBIAN)
> 
> Wouldnt it be better to check the USE(Qt_MOBILE_THEME) here?
Thanks for reviewing.

The WTF_USE_Qt_MOBILE_THEME is not defined for the api test, and we can't use the Macro 'USE' in the test, so ...

[Alexis Menard (darktears)]

Comment on attachment 88653 [details]
skips this test for symbian

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

>>> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:44
>>> +#if !defined(Q_OS_SYMBIAN)
>> 
>> Wouldnt it be better to check the USE(Qt_MOBILE_THEME) here?
> 
> Thanks for reviewing.
> 
> The WTF_USE_Qt_MOBILE_THEME is not defined for the api test, and we can't use the Macro 'USE' in the test, so ...

qwebpage auto test have #if defined(ENABLE_WEBGL) && ENABLE_WEBGL

you should definitively checks further.

[Yi Shen]

Created attachment 90228 [details]
second try

Thanks Alexis, you are right. There is a way to use WTF_USE_QT_MOBILE_THEME in the api test :)

[Alexis Menard (darktears)]

Comment on attachment 90228 [details]
second try

Better :D

[Laszlo Gombos]

Does this means now that WTF_USE_QT_MOBILE_THEME define is part of the QtWebKit API - as it seems that that is the only way to detect API functionality ?

[Yi Shen]

(In reply to comment #12)
> Does this means now that WTF_USE_QT_MOBILE_THEME define is part of the QtWebKit API - as it seems that that is the only way to detect API functionality ?
No, that define is only for the test since I added following line in the test.pri

+use_qt_mobile_theme: DEFINES += WTF_USE_QT_MOBILE_THEME=1

[WebKit Commit Bot]

Comment on attachment 90228 [details]
second try

Rejecting attachment 90228 [details] from commit-queue.

Failed to run "[u'zip', u'-r', u'../commit-queue-logs/57254-layout-test-results-3.zip', u'/tmp/layout-test-resul..." exit_code: 12

	zip warning: name not matched: /tmp/layout-test-results

zip error: Nothing to do! (try: zip -r ../commit-queue-logs/57254-layout-test-results-3.zip . -i /tmp/layout-test-results)

Full output: http://queues.webkit.org/results/8475460

[WebKit Commit Bot]

Comment on attachment 90228 [details]
second try

Rejecting attachment 90228 [details] from commit-queue.

Failed to run "[u'zip', u'-r', u'../commit-queue-logs/57254-layout-test-results-2.zip', u'/tmp/layout-test-resul..." exit_code: 12

	zip warning: name not matched: /tmp/layout-test-results

zip error: Nothing to do! (try: zip -r ../commit-queue-logs/57254-layout-test-results-2.zip . -i /tmp/layout-test-results)

Full output: http://queues.webkit.org/results/8470509

[Eric Seidel (no email)]

Comment on attachment 90228 [details]
second try

My apologies, the cq got confused.  The python tests have been fixed on trunk and the cq will be made robust against such future failures with bug 58955.

[WebKit Commit Bot]

Comment on attachment 90228 [details]
second try

Clearing flags on attachment: 90228

Committed r84334: <http://trac.webkit.org/changeset/84334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.