Bug 82700 – [Qt][WK2] REGRESSION, fix a failing API test in qmltests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

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)]

[Marcelo Lira]

Created attachment 143075 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 143075 [details]
Patch

Question : why the behavior changed in Qt?

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Marcelo Lira]

Created attachment 143079 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Looks good to me.

[Rafael Brandao]

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?

[Alexis Menard (darktears)]

(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

[Marcelo Lira]

(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

[WebKit Review Bot]

Comment on attachment 143079 [details]
Patch

Clearing flags on attachment: 143079

Committed r117812: <http://trac.webkit.org/changeset/117812>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.