Bug 101949 - [Chromium] Refactor theme font lookup into a factory
Summary: [Chromium] Refactor theme font lookup into a factory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Scott Violet
URL:
Keywords:
Depends on: 102177
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-12 10:16 PST by Scott Violet
Modified: 2012-11-14 13:56 PST (History)
4 users (show)

See Also:


Attachments
Fix (32.36 KB, patch)
2012-11-12 10:27 PST, Scott Violet
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff
Fixes style issues (32.32 KB, patch)
2012-11-12 15:33 PST, Scott Violet
no flags Details | Formatted Diff | Diff
Patch for landing (32.33 KB, patch)
2012-11-13 15:19 PST, Scott Violet
no flags Details | Formatted Diff | Diff
Patch for landing (32.34 KB, patch)
2012-11-13 15:22 PST, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (32.16 KB, patch)
2012-11-14 08:26 PST, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (32.42 KB, patch)
2012-11-14 09:58 PST, Scott Violet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Violet 2012-11-12 10:16:20 PST
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.
Comment 1 Scott Violet 2012-11-12 10:27:51 PST
Created attachment 173669 [details]
Fix
Comment 2 WebKit Review Bot 2012-11-12 10:30:34 PST
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.
Comment 3 Darin Fisher (:fishd, Google) 2012-11-12 14:00:54 PST
Comment on attachment 173669 [details]
Fix

R=me, but you need to fix up the style warnings.
Comment 4 Scott Violet 2012-11-12 15:33:07 PST
Created attachment 173739 [details]
Fixes style issues
Comment 5 Tony Chang 2012-11-13 14:50:23 PST
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.
Comment 6 Scott Violet 2012-11-13 15:19:57 PST
Created attachment 174007 [details]
Patch for landing
Comment 7 Scott Violet 2012-11-13 15:22:56 PST
Created attachment 174008 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-11-13 20:06:40 PST
Comment on attachment 174008 [details]
Patch for landing

Clearing flags on attachment: 174008

Committed r134524: <http://trac.webkit.org/changeset/134524>
Comment 9 WebKit Review Bot 2012-11-13 20:06:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Review Bot 2012-11-13 22:13:49 PST
Re-opened since this is blocked by bug 102177
Comment 11 Kentaro Hara 2012-11-13 22:19:03 PST
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....
Comment 12 Scott Violet 2012-11-14 08:26:10 PST
Created attachment 174165 [details]
Patch
Comment 13 Tony Chang 2012-11-14 09:40:03 PST
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()?
Comment 14 Scott Violet 2012-11-14 09:58:49 PST
Created attachment 174184 [details]
Patch
Comment 15 Scott Violet 2012-11-14 09:59:41 PST
I was worried about call order, so I went with DEFINE_STATIC_LOCAL.
Comment 16 WebKit Review Bot 2012-11-14 13:56:41 PST
Comment on attachment 174184 [details]
Patch

Clearing flags on attachment: 174184

Committed r134662: <http://trac.webkit.org/changeset/134662>
Comment 17 WebKit Review Bot 2012-11-14 13:56:45 PST
All reviewed patches have been landed.  Closing bug.