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(); }
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
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.
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.
(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.
(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 :)
Created attachment 88653 [details] skips this test for symbian I will check the meego also once I got the meego environment.
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?
(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 ...
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.
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 :)
Comment on attachment 90228 [details] second try Better :D
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 ?
(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
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
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
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.
Comment on attachment 90228 [details] second try Clearing flags on attachment: 90228 Committed r84334: <http://trac.webkit.org/changeset/84334>
All reviewed patches have been landed. Closing bug.