Bug 76821

Adding dependency. The use_system_icu option may need some updating, we need a config test and also switch the default along with updated layout tests.

Simon, does this make sense: https://gist.github.com/1673136 ? If yes, I will prepare a proper patch. :) I want to figure it out first the tests we can skip after this. So far it seems that the ones from the bugs that now block this one plus: Expected to fail, but passed: (19) fast/encoding/GBK/EUC-CN.html fast/encoding/GBK/chinese.html fast/encoding/GBK/cn-gb.html fast/encoding/GBK/csgb2312.html fast/encoding/GBK/csgb231280.html fast/encoding/GBK/gb2312.html fast/encoding/GBK/gb_2312-80.html fast/encoding/GBK/gbk.html fast/encoding/GBK/iso-ir-58.html fast/encoding/GBK/x-euc-cn.html fast/encoding/GBK/x-gbk.html fast/encoding/char-decoding-mac.html fast/encoding/char-decoding.html fast/encoding/char-encoding-mac.html fast/encoding/char-encoding.html fast/encoding/hebrew/8859-8-e.html fast/encoding/hebrew/8859-8-i.html fast/encoding/hebrew/csISO88598I.html fast/encoding/hebrew/logical.html Regressions: Unexpected text diff mismatch : (2) fast/encoding/utf-16-big-endian.html = TEXT fast/encoding/utf-16-little-endian.html = TEXT Regressions: Unexpected no expected results found : (2) fast/encoding/invalid-UTF-8.html = MISSING fast/encoding/xmacroman-encoding-test.html = MISSING and Expected to fail, but passed: (20) sputnik/Unicode/Unicode_218/S7.6_A5.3_T1.html sputnik/Unicode/Unicode_218/S7.6_A5.3_T2.html sputnik/Unicode/Unicode_500/S7.6_A3.1.html sputnik/Unicode/Unicode_500/S7.6_A3.2.html sputnik/Unicode/Unicode_500/S7.6_A5.3_T1.html sputnik/Unicode/Unicode_500/S7.6_A5.3_T2.html sputnik/Unicode/Unicode_510/S15.5.4.16_A1.html sputnik/Unicode/Unicode_510/S15.5.4.18_A1.html sputnik/Unicode/Unicode_510/S7.6_A1.1_T1.html sputnik/Unicode/Unicode_510/S7.6_A1.1_T2.html sputnik/Unicode/Unicode_510/S7.6_A1.1_T4.html sputnik/Unicode/Unicode_510/S7.6_A2.2_T1.html sputnik/Unicode/Unicode_510/S7.6_A2.2_T2.html sputnik/Unicode/Unicode_510/S7.6_A2.3.html sputnik/Unicode/Unicode_510/S7.6_A5.2_T1.html sputnik/Unicode/Unicode_510/S7.6_A5.2_T2.html sputnik/Unicode/Unicode_510/S7.6_A5.2_T4.html sputnik/Unicode/Unicode_510/S7.6_A5.2_T7.html sputnik/Unicode/Unicode_510/S7.6_A5.2_T8.html sputnik/Unicode/Unicode_510/S7.6_A5.2_T9.html

(In reply to comment #2) > If yes, I will prepare a proper patch. :) > I want to figure it out first the tests we can skip after this. So far it seems that the ones from the bugs that now block this one plus: I mean the tests we can UNSKIP.

Yes, that sounds about right. Techically we need a configure test and uses icu-config on unix (!mac) and use the libs directly on windows and mac (a system library on the latter, so always available). I think we should do this only for the Qt 5 build at the moment and not affect Qt 4. Then we can land it in trunk and adjust the qt5 layout test results accordingly.

Created attachment 124005 [details] Patch

Comment on attachment 124005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124005&action=review > Source/WTF/WTF.pri:23 > +haveQt(5):contains(QT_CONFIG,icu) { > + unix:!mac: LIBS += $$system(icu-config --ldflags-searchpath --ldflags-libsonly) > + win32:LIBS += -licuin > } else { What do we do on mac, mac has libicu.

(In reply to comment #6) > What do we do on mac, mac has libicu. Yes, you are right, but I'm not sure what we need to do on mac since it has icu available on the system.

Comment on attachment 124005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124005&action=review >> Source/WTF/WTF.pri:23 >> +haveQt(5):contains(QT_CONFIG,icu) { >> + unix:!mac: LIBS += $$system(icu-config --ldflags-searchpath --ldflags-libsonly) >> + win32:LIBS += -licuin >> } else { > > What do we do on mac, mac has libicu. I guess it should read: unix:!mac: LIBS += icu-config-stuff else: LIBS += -licuin :)

Created attachment 124109 [details] Patch

Comment on attachment 124109 [details] Patch r=me

Comment on attachment 124109 [details] Patch Clearing flags on attachment: 124109 Committed r105997: <http://trac.webkit.org/changeset/105997>

All reviewed patches have been landed. Closing bug.

And you landed a skip/unskip follow-up patch: http://trac.webkit.org/changeset/106027 And I skipped many tests, because they still fail on Qt5 - http://trac.webkit.org/changeset/106106/trunk/LayoutTests/platform/qt-5.0/Skipped These tests need investigation if they are regression or progression, etc. It would be great if you can do it and make the bot green again.

(In reply to comment #13) > And you landed a skip/unskip follow-up patch: http://trac.webkit.org/changeset/106027 Ossy, I rolled this out in http://trac.webkit.org/changeset/106040 exactly because I realized the tests needed more investigation. So I don't really get why you needed to skip more tests event after I rolled it out.

I don't know why, I didn't have time to check it ... did you roll out 106027 too? Otherwise please comment the bug if you land a followup fix, skip/unskip patch, rollout, etc. related to a bug. Because nobody noticed that you rolled out this patch, etc. I reopened this bug, because it isn't solved now.

I checked the history: - ICU enable patch landed in r105997 - skip/unskip patch landed in r106027 - skip/unskip patch rolled out in r106040 - my skip patch landed in r106106 The problem wasn't caused by r106027, but the original r105997. That's why I had to skip many tests by r106106.

(In reply to comment #16) > The problem wasn't caused by r106027, but the original > r105997. That's why I had to skip many tests by r106106. It's hard to sort this out if some of these tests are passing here locally. When I landed the ICU patch I asked for help from the bot administrators and only kbalazs was available to help. We checked it together and we skipped the tests that I had unskipped and that were causing troubles and the bot was green again. I remember he also said that a few tests just needed to be rebaselined. I will check with a clean build now, but to be honest it would be great if one of the gardeners (with access to the bot) could help with this.

Created attachment 124635 [details] Patch I've investigated the tests skipped here and some of them had false expected results (then I've moved to qt-4.8 or removed from Skipped), others had to be rebaselined, and a few others still need more investigation (only one of them seems to relate with this bug). Ossy, do you mind if we let the bots test these changes (or double check before landing)? :-)

(In reply to comment #18) > Created an attachment (id=124635) [details] > Patch > > I've investigated the tests skipped here and some of them had false expected results (then I've moved to qt-4.8 or removed from Skipped), others had to be rebaselined, and a few others still need more investigation (only one of them seems to relate with this bug). Ossy, do you mind if we let the bots test these changes (or double check before landing)? :-) I'm double checking it right now. And will r+ after I finished.

I finished with double checking, short results: - editing/selection/regional-indicators.html needs qt-4.8 specific expected result (which you removed from qt platform) - css1/text_properties/text_transform.html fails with qt5 (wk1 and wk2 too) - editing/selection/drag-select-1.html became flakey (TIMEOUT PASS) on qt5-wk2 I'll check them later.

(In reply to comment #20) > I finished with double checking, short results: Thanks for the help, Ossy. > - css1/text_properties/text_transform.html fails with qt5 (wk1 and wk2 too) We manage to get the correct new result of this test but I believe Rafael forgot to add them on the patch. > - editing/selection/drag-select-1.html became flakey (TIMEOUT PASS) on qt5-wk2 How do we solve issues with flakey tests? This is a legitimate question due to our lack of experience with tests and the build bots.

Created attachment 124749 [details] Patch

Attachment 124749 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124749 [details] Patch I'll add the test that jeez_ removed and made it flaky, so ignore this one.

Created attachment 124754 [details] Patch

Attachment 124754 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124754 [details] Patch r=me, let's go.

Comment on attachment 124754 [details] Patch Clearing flags on attachment: 124754 Committed r106372: <http://trac.webkit.org/changeset/106372>

All reviewed patches have been landed. Closing bug.