Summary: | [Qt] Layouttest fast/css/opacity-float.html failed on Qt | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chang Shu <cshu> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, evan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 29653 | ||||||||||||
Attachments: |
|
Description
Chang Shu
2009-10-09 18:10:15 PDT
Created attachment 41041 [details]
implement setPOSIXLocale
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?
(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! 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. Created attachment 41103 [details]
reset locale at the beginning of each test case
(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 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? :)
Created attachment 41111 [details]
add missing ChangeLog
forgot to include ChangeLog last time
Comment on attachment 41111 [details] add missing ChangeLog After the patch from bug 30305 this may require a rebase, but otherwise it looks good. Thanks for the review! Yeah, a re-work is a must. Will submit a new patch. Created attachment 41148 [details]
re-work after baseline change
Comment on attachment 41148 [details]
re-work after baseline change
Thanks!
Comment on attachment 41148 [details] re-work after baseline change Clearing flags on attachment: 41148 Committed r49563: <http://trac.webkit.org/changeset/49563> All reviewed patches have been landed. Closing bug. |