Bug 101949 – [Chromium] Refactor theme font lookup into a factory

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Scott Violet]

Long term we would like to allow RenderThemeChromiumLinux to run on both windows and linux. However the font lookup should remain platform specific. We need to refactor font code out of RenderThemeChromiumSkia into platform specific methods.

[Scott Violet]

Created attachment 173669 [details]
Fix

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:50:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:56:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:75:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:101:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:102:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:147:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderThemeChromiumFontProvider.cpp:34:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 8 in 9 files


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

[Darin Fisher (:fishd, Google)]

Comment on attachment 173669 [details]
Fix

R=me, but you need to fix up the style warnings.

[Scott Violet]

Created attachment 173739 [details]
Fixes style issues

[Tony Chang]

Comment on attachment 173739 [details]
Fixes style issues

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

> Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:51
> +void RenderThemeChromiumFontProvider::getFont(int propId, FontDescription& fontDescription)

Nit: It's not that common to prefix a method with 'get' in WebKit. Something like initializeSystemFont might be more descriptive.

[Scott Violet]

Created attachment 174007 [details]
Patch for landing

[Scott Violet]

Created attachment 174008 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174008 [details]
Patch for landing

Clearing flags on attachment: 174008

Committed r134524: <http://trac.webkit.org/changeset/134524>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102177

[Kentaro Hara]

It looks like this patch caused the following build error:

third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:37:24: error: declaration
      requires a global constructor [-Werror,-Wglobal-constructors]
static FontDescription smallSystemFont;
                       ^~~~~~~~~~~~~~~
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:37:24: error: declaration
      requires an exit-time destructor [-Werror,-Wexit-time-destructors]
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:37:24: error: declaration
      requires a global destructor [-Werror,-Wglobal-constructors]
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:38:24: error: declaration
      requires a global constructor [-Werror,-Wglobal-constructors]
static FontDescription menuFont;
                       ^~~~~~~~
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:38:24: error: declaration
      requires an exit-time destructor [-Werror,-Wexit-time-destructors]
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:38:24: error: declaration
      requires a global destructor [-Werror,-Wglobal-constructors]
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:39:24: error: declaration
      requires a global constructor [-Werror,-Wglobal-constructors]
static FontDescription labelFont;
                       ^~~~~~~~~
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:39:24: error: declaration
      requires an exit-time destructor [-Werror,-Wexit-time-destructors]
third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.cpp:39:24: error: declaration
      requires a global destructor [-Werror,-Wglobal-constructors]
9 errors generated.
make: *** [out/Release/obj.target/webcore_rendering/third_party/WebKit/Source/WebCore/rendering/RenderThemeChromiumFontProviderLinux.o] Error 1
make: *** Waiting for unfinished jobs....

[Scott Violet]

Created attachment 174165 [details]
Patch

[Tony Chang]

Comment on attachment 174165 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeChromiumFontProviderWin.cpp:47
> +static FontDescription smallSystemFont;
> +static FontDescription menuFont;
> +static FontDescription labelFont;

These look like static initializers too, although MSVC won't complain about it.  Hmm, maybe make these pointers and initialize them lazily in systemFont()?

[Scott Violet]

Created attachment 174184 [details]
Patch

[Scott Violet]

I was worried about call order, so I went with DEFINE_STATIC_LOCAL.

[WebKit Review Bot]

Comment on attachment 174184 [details]
Patch

Clearing flags on attachment: 174184

Committed r134662: <http://trac.webkit.org/changeset/134662>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.