Bug 76821 – [Qt] Use ICU if available

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Hausmann]

Qt 5.0 is going to add ICU as a required dependency.

We should switch from QTextBreakIterator and the Qt unicode functions over to using ICU for the Qt build, for consistent behavior with the other ports (and rumors have it, better performance ;)

[Simon Hausmann]

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.

[Jesus Sanchez-Palencia]

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

[Jesus Sanchez-Palencia]

(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.

[Simon Hausmann]

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.

[Jesus Sanchez-Palencia]

Created attachment 124005 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Jesus Sanchez-Palencia]

(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.

[Simon Hausmann]

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

:)

[Jesus Sanchez-Palencia]

Created attachment 124109 [details]
Patch

[Simon Hausmann]

Comment on attachment 124109 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 124109 [details]
Patch

Clearing flags on attachment: 124109

Committed r105997: <http://trac.webkit.org/changeset/105997>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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.

[Jesus Sanchez-Palencia]

(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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[Jesus Sanchez-Palencia]

(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.

[Rafael Brandao]

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)? :-)

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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.

[Jesus Sanchez-Palencia]

(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.

[Rafael Brandao]

Created attachment 124749 [details]
Patch

[WebKit Review Bot]

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.

[Rafael Brandao]

Comment on attachment 124749 [details]
Patch

I'll add the test that jeez_ removed and made it flaky, so ignore this one.

[Rafael Brandao]

Created attachment 124754 [details]
Patch

[WebKit Review Bot]

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.

[Csaba Osztrogonác]

Comment on attachment 124754 [details]
Patch

r=me, let's go.

[WebKit Review Bot]

Comment on attachment 124754 [details]
Patch

Clearing flags on attachment: 124754

Committed r106372: <http://trac.webkit.org/changeset/106372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.