Bug 65197 – Add a generic pictograph font family

• Component: CSS
• Status: RESOLVED FIXED

Add a generic pictograph font family

[Radar WebKit Bug Importer]

<rdar://problem/9842889>

Created attachment 102036 [details]
Add -webkit-pictograph generic family, WebKit2 API and WebKit1 Mac and Windows API for setting pictograph font family

[WebKit Review Bot]

Attachment 102036 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/win/WebPreferences.cpp:656:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.cpp:663:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.h:94:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.h:97:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.h:98:  The parameter name "family" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 102036 [details]
Add -webkit-pictograph generic family, WebKit2 API and WebKit1 Mac and Windows API for setting pictograph font family

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

Will the test work on earlier versions of Mac OS X?

> Source/WebKit/mac/WebView/WebView.mm:1512
> +//    settings->setPictographFontFamily([preferences pictographFontFamily]);

This should either be uncommented or removed.

(In reply to comment #4)
> (From update of attachment 102036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102036&action=review
> 
> Will the test work on earlier versions of Mac OS X?

No.

> 
> > Source/WebKit/mac/WebView/WebView.mm:1512
> > +//    settings->setPictographFontFamily([preferences pictographFontFamily]);
> 
> This should either be uncommented or removed.

It should be uncommented. I mistakenly commented it after running the tests and before uploading the patch.

Created attachment 102041 [details]
Add -webkit-pictograph generic family, WebKit2 API and WebKit1 Mac and Windows API for setting pictograph font family

[WebKit Review Bot]

Attachment 102041 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/win/WebPreferences.cpp:656:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.cpp:663:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.h:94:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.h:97:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebPreferences.h:98:  The parameter name "family" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r91777. <http://trac.webkit.org/r91777>

[Adrienne Walker]

Committed r91798: <http://trac.webkit.org/changeset/91798>

[Adrienne Walker]

(In reply to comment #9)
> Committed r91798: <http://trac.webkit.org/changeset/91798>

mitz: Could you take a look at what Chromium output and see if that looks reasonable?

(In reply to comment #10)
> (In reply to comment #9)
> > Committed r91798: <http://trac.webkit.org/changeset/91798>
> 
> mitz: Could you take a look at what Chromium output and see if that looks reasonable?

Looks OK.

[Adrienne Walker]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Committed r91798: <http://trac.webkit.org/changeset/91798>
> > 
> > mitz: Could you take a look at what Chromium output and see if that looks reasonable?
> 
> Looks OK.

Thanks! :)

[Frédéric Wang (:fredw)]

Comment on attachment 102041 [details]
Add -webkit-pictograph generic family, WebKit2 API and WebKit1 Mac and Windows API for setting pictograph font family

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

@mitz: I found this old bug while trying to check how to implement font-family: math in Chromium (incidentally for WebKit it is bug 209595) and I think there was a mistake in this patch. See comments below.

> Source/WebCore/css/CSSFontSelector.cpp:406
> +        genericFamily = settings->pictographFontFamily();

This only uses raw string comparison of the font-family name. This does not even require "-webkit-pictograph" to be parsed as a CSSValueWebkitPictograph, so your test can still work despite the mistake. Actually I'm removing dead code in https://chromium-review.googlesource.com/c/chromium/src/+/2119545 and it still passes.

> Source/WebCore/css/CSSValueKeywords.in:107
> +-webkit-pictograph

I believe CSSValueWebkitPictograph is actually never parsed. This is because consumeGenericFamily always only accepts values between "serif" and "-webkit-body" and you added "-webkit-pictograph" *after* them. I guess the only possible place where CSSValueWebkitPictograph is generated is WebCore/css/CSSComputedStyleDeclaration.cpp although that didn't seem to be triggered by your test when I tried in Chromium...