RESOLVED FIXED 30268
[Qt] Layouttest fast/css/opacity-float.html failed on Qt
https://bugs.webkit.org/show_bug.cgi?id=30268
Summary [Qt] Layouttest fast/css/opacity-float.html failed on Qt
Chang Shu
Reported 2009-10-09 18:10:15 PDT
run WebKitBuild/Release/bin/DumpRenderTree --qt LayoutTests/fast/css/opacity-float.html error message: CONSOLE MESSAGE: line 6: TypeError: Result of expression 'layoutTestController.setPOSIXLocale' [undefined] is not a function.
Attachments
implement setPOSIXLocale (1.68 KB, patch)
2009-10-12 07:16 PDT, Chang Shu
hausmann: review-
reset locale at the beginning of each test case (1.59 KB, patch)
2009-10-13 08:35 PDT, Chang Shu
hausmann: review-
add missing ChangeLog (2.32 KB, patch)
2009-10-13 09:51 PDT, Chang Shu
hausmann: review+
re-work after baseline change (2.87 KB, patch)
2009-10-13 19:45 PDT, Chang Shu
no flags
Chang Shu
Comment 1 2009-10-12 07:16:06 PDT
Created attachment 41041 [details] implement setPOSIXLocale
Simon Hausmann
Comment 2 2009-10-12 22:46:08 PDT
Comment on attachment 41041 [details] implement setPOSIXLocale I think your patch looks good, but I think qt/DumpRenderTree.cpp's resetToConsistentStateBeforeTesting() should also set the default locale back (i.e. using QLocale::setDefault). Could you add that line, just so that we are safe that after a test called setPOSIXLocale we set this back and avoid side-effects in other tests?
Chang Shu
Comment 3 2009-10-13 06:38:10 PDT
(In reply to comment #2) > (From update of attachment 41041 [details]) > I think your patch looks good, but I think qt/DumpRenderTree.cpp's > resetToConsistentStateBeforeTesting() should also set the default locale back > (i.e. using QLocale::setDefault). > > Could you add that line, just so that we are safe that after a test called > setPOSIXLocale we set this back and avoid side-effects in other tests? Thanks for the comments! I was asking the same question to Evan in bug 29653 and now I forgot to do it myself. :( Btw, I see there is already a line of "setlocale(LC_ALL,"");" in function resetToConsistentStateBeforeTesting(). However, I don't think it works for Qt. Shall I remove it and replace with the following line? QLocale qlocale; QLocale::setDefault(qlocale); Thanks!
Evan Martin
Comment 4 2009-10-13 07:53:05 PDT
I added that setlocale line in a vain attempt to not break the Qt build (I thought you were sharing LayoutTestController to). It is safe to remove if you are resetting the locale.
Chang Shu
Comment 5 2009-10-13 08:35:55 PDT
Created attachment 41103 [details] reset locale at the beginning of each test case
Chang Shu
Comment 6 2009-10-13 08:36:39 PDT
(In reply to comment #4) > I added that setlocale line in a vain attempt to not break the Qt build (I > thought you were sharing LayoutTestController to). It is safe to remove if you > are resetting the locale. Thanks, Evan. New patch attached.
Simon Hausmann
Comment 7 2009-10-13 09:32:40 PDT
Comment on attachment 41103 [details] reset locale at the beginning of each test case Thanks! Your patch looks okay, but it is missing a ChangeLog entry. Can you add that? :)
Chang Shu
Comment 8 2009-10-13 09:51:06 PDT
Created attachment 41111 [details] add missing ChangeLog forgot to include ChangeLog last time
Simon Hausmann
Comment 9 2009-10-13 15:25:38 PDT
Comment on attachment 41111 [details] add missing ChangeLog After the patch from bug 30305 this may require a rebase, but otherwise it looks good.
Chang Shu
Comment 10 2009-10-13 15:27:02 PDT
Thanks for the review!
Chang Shu
Comment 11 2009-10-13 15:30:35 PDT
Yeah, a re-work is a must. Will submit a new patch.
Chang Shu
Comment 12 2009-10-13 19:45:50 PDT
Created attachment 41148 [details] re-work after baseline change
Simon Hausmann
Comment 13 2009-10-13 22:03:55 PDT
Comment on attachment 41148 [details] re-work after baseline change Thanks!
WebKit Commit Bot
Comment 14 2009-10-14 09:02:21 PDT
Comment on attachment 41148 [details] re-work after baseline change Clearing flags on attachment: 41148 Committed r49563: <http://trac.webkit.org/changeset/49563>
WebKit Commit Bot
Comment 15 2009-10-14 09:02:25 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.