Summary: | [Qt][WK2] REGRESSION, fix a failing API test in qmltests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||
Component: | Tools / Tests | Assignee: | Marcelo Lira <marcelo.lira> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Critical | CC: | cmarcelo, hausmann, menard, ossy, rafael.lobo, webkit.review.bot, zoltan | ||||||
Priority: | P1 | Keywords: | Qt, QtTriaged | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 70236 | ||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2012-03-30 00:14:51 PDT
Created attachment 143075 [details]
Patch
Comment on attachment 143075 [details]
Patch
Question : why the behavior changed in Qt?
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 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. Created attachment 143079 [details]
Patch
Looks good to me. 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? (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 (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 on attachment 143079 [details] Patch Clearing flags on attachment: 143079 Committed r117812: <http://trac.webkit.org/changeset/117812> All reviewed patches have been landed. Closing bug. |