Bug 82700 - [Qt][WK2] REGRESSION, fix a failing API test in qmltests
: [Qt][WK2] REGRESSION, fix a failing API test in qmltests
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: All All
: P1 Critical
Assigned To: Marcelo Lira
: Qt, QtTriaged
Depends on:
Blocks: 70236
  Show dependency treegraph
 
Reported: 2012-03-30 00:14 PDT by Csaba Osztrogonác
Modified: 2012-05-21 13:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.20 KB, patch)
2012-05-21 12:17 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (2.19 KB, patch)
2012-05-21 12:54 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-03-30 00:14:51 PDT
All API tests pass with the latest pinned Qt5 hash (7d0327830bb4768753cb0c14a23d98fed08be7d2),
but the following test fails with a newer Qt5: 15f3399576bd433898f8e89bbcb3c1196e0da5b9

FAIL!  : qmltests::WebViewPreferences::test_standardFontFamilyChanged() Compared values are not the same
   Actual   (): 'Bitstream Vera Serif'
   Expected (): Bitstream Vera Serif
   Loc: [/home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_preferences.qml(204)]
Comment 1 Marcelo Lira 2012-05-21 12:17:37 PDT
Created attachment 143075 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-05-21 12:29:52 PDT
Comment on attachment 143075 [details]
Patch

Question : why the behavior changed in Qt?
Comment 3 Caio Marcelo de Oliveira Filho 2012-05-21 12:33:49 PDT
Comment on attachment 143075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143075&action=review

Thanks for finally fixing this one, Marcelo :-)  I have some comments following.

> Source/WebKit2/ChangeLog:10
> +        Font family names returned by WebKit experimental always return values

I would rephrase this, at least to make clear is our: __WebView__ experimental preferences API. And we return the same values as existing WKPreferences C API for WK2.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_preferences.qml:199
> +                return text.replace(/^('*)([^']*)('*)$/, '$2')

Can we make it simpler? Like /^'?(.*)'?$/, '$1'... or even no Regexp at all.

This still doesn't support fonts that had ' originally in the name, but since our set of fonts is controlled for testing, that should be OK.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_preferences.qml:210
> +                compare(unquote(webView.title), unquote(defaultStandardFontFamily))

Just need to unquote the webView.title here since we expect defaultStandarFontFamily to be unquoted.
Comment 4 Caio Marcelo de Oliveira Filho 2012-05-21 12:37:29 PDT
Comment on attachment 143075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143075&action=review

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_preferences.qml:199
>> +                return text.replace(/^('*)([^']*)('*)$/, '$2')
> 
> Can we make it simpler? Like /^'?(.*)'?$/, '$1'... or even no Regexp at all.
> 
> This still doesn't support fonts that had ' originally in the name, but since our set of fonts is controlled for testing, that should be OK.

Bah. My suggestion is wrong because (.*) will eat everything. But I still think we can simplify this.
Comment 5 Marcelo Lira 2012-05-21 12:54:54 PDT
Created attachment 143079 [details]
Patch
Comment 6 Caio Marcelo de Oliveira Filho 2012-05-21 12:56:26 PDT
Looks good to me.
Comment 7 Rafael Brandao 2012-05-21 12:57:49 PDT
Comment on attachment 143079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143079&action=review

> Source/WebKit2/ChangeLog:9
> +        quotes when white space is present in its name, and no quotes otherwise.

Is it on the spec?
Comment 8 Alexis Menard (darktears) 2012-05-21 13:01:53 PDT
(In reply to comment #7)
> (From update of attachment 143079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143079&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        quotes when white space is present in its name, and no quotes otherwise.
> 
> Is it on the spec?

Yes
Comment 9 Marcelo Lira 2012-05-21 13:03:58 PDT
(In reply to comment #7)
> (From update of attachment 143079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143079&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        quotes when white space is present in its name, and no quotes otherwise.
> 
> Is it on the spec?

"To avoid mistakes in escaping, it is recommended to quote font family names that contain white space, digits, or punctuation characters other than hyphens (...)"

http://www.w3.org/TR/CSS21/fonts.html#font-family-prop
Comment 10 WebKit Review Bot 2012-05-21 13:47:35 PDT
Comment on attachment 143079 [details]
Patch

Clearing flags on attachment: 143079

Committed r117812: <http://trac.webkit.org/changeset/117812>
Comment 11 WebKit Review Bot 2012-05-21 13:47:40 PDT
All reviewed patches have been landed.  Closing bug.