Bug 57254

Summary:

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

Product:

WebKit

Reporter:

Yi Shen <max.hong.shen>

Component:

WebKit API

Assignee:

Yi Shen <max.hong.shen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ademar, commit-queue, eric, kling, laszlo.gombos, luiz, menard

Priority:

Version:

528+ (Nightly build)

Hardware:

S60 Hardware

OS:

All

Bug Depends on:

Bug Blocks:

38654, 50925

Attachments:

Description	Flags
skips this test for symbian	none
second try	none

Yi Shen

Reported 2011-03-28 11:37:49 PDT

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(); }

Attachments
skips this test for symbian (2.58 KB, patch) 2011-04-07 10:06 PDT, Yi Shen	no flags	Details Formatted Diff Diff
second try (3.31 KB, patch) 2011-04-19 11:39 PDT, Yi Shen	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Yi Shen

Comment 1 2011-03-28 11:41:17 PDT

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)

Comment 2 2011-03-28 12:22:01 PDT

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

Comment 3 2011-03-28 12:26:13 PDT

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

Comment 4 2011-03-30 19:19:07 PDT

(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

Comment 5 2011-03-31 08:15:46 PDT

(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

Comment 6 2011-04-07 10:06:06 PDT

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 7 2011-04-07 13:55:34 PDT

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

Comment 8 2011-04-07 14:01:36 PDT

(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 9 2011-04-19 05:17:25 PDT

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

Comment 10 2011-04-19 11:39:57 PDT

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 11 2011-04-19 11:42:23 PDT

Comment on attachment 90228 [details] second try Better :D

Laszlo Gombos

Comment 12 2011-04-19 11:54:51 PDT

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

Comment 13 2011-04-19 12:03:21 PDT

(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 14 2011-04-19 17:13:20 PDT

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 15 2011-04-19 18:14:33 PDT

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 16 2011-04-19 18:20:33 PDT

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 17 2011-04-19 20:21:24 PDT

Comment on attachment 90228 [details] second try Clearing flags on attachment: 90228 Committed r84334: <http://trac.webkit.org/changeset/84334>

WebKit Commit Bot

Comment 18 2011-04-19 20:21:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.