Bug 30268 - [Qt] Layouttest fast/css/opacity-float.html failed on Qt
Summary: [Qt] Layouttest fast/css/opacity-float.html failed on Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 29653
  Show dependency treegraph
 
Reported: 2009-10-09 18:10 PDT by Chang Shu
Modified: 2009-10-14 09:02 PDT (History)
2 users (show)

See Also:


Attachments
implement setPOSIXLocale (1.68 KB, patch)
2009-10-12 07:16 PDT, Chang Shu
hausmann: review-
Details | Formatted Diff | Diff
reset locale at the beginning of each test case (1.59 KB, patch)
2009-10-13 08:35 PDT, Chang Shu
hausmann: review-
Details | Formatted Diff | Diff
add missing ChangeLog (2.32 KB, patch)
2009-10-13 09:51 PDT, Chang Shu
hausmann: review+
Details | Formatted Diff | Diff
re-work after baseline change (2.87 KB, patch)
2009-10-13 19:45 PDT, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 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.
Comment 1 Chang Shu 2009-10-12 07:16:06 PDT
Created attachment 41041 [details]
implement setPOSIXLocale
Comment 2 Simon Hausmann 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?
Comment 3 Chang Shu 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!
Comment 4 Evan Martin 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.
Comment 5 Chang Shu 2009-10-13 08:35:55 PDT
Created attachment 41103 [details]
reset locale at the beginning of each test case
Comment 6 Chang Shu 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.
Comment 7 Simon Hausmann 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? :)
Comment 8 Chang Shu 2009-10-13 09:51:06 PDT
Created attachment 41111 [details]
add missing ChangeLog

forgot to include ChangeLog last time
Comment 9 Simon Hausmann 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.
Comment 10 Chang Shu 2009-10-13 15:27:02 PDT
Thanks for the review!
Comment 11 Chang Shu 2009-10-13 15:30:35 PDT
Yeah, a re-work is a must. Will submit a new patch.
Comment 12 Chang Shu 2009-10-13 19:45:50 PDT
Created attachment 41148 [details]
re-work after baseline change
Comment 13 Simon Hausmann 2009-10-13 22:03:55 PDT
Comment on attachment 41148 [details]
re-work after baseline change

Thanks!
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2009-10-14 09:02:25 PDT
All reviewed patches have been landed.  Closing bug.