Bug 23340 – Add remaining bits of platform/graphics/chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Dimitri Glazkov (Google)]

SimpleFontData, ThemeHelper, UniscribeHelper and friends.

[Dimitri Glazkov (Google)]

Created attachment 26744 [details]
patch v1

 WebCore/ChangeLog                                  |   76 ++
 .../chromium/SimpleFontDataChromiumWin.cpp         |  171 ++++
 .../graphics/chromium/SimpleFontDataLinux.cpp      |  160 ++++
 .../graphics/chromium/ThemeHelperChromiumWin.cpp   |  104 +++
 .../graphics/chromium/ThemeHelperChromiumWin.h     |   98 +++
 .../platform/graphics/chromium/UniscribeHelper.cpp |  902 ++++++++++++++++++++
 .../platform/graphics/chromium/UniscribeHelper.h   |  399 +++++++++
 .../graphics/chromium/UniscribeHelperTextRun.cpp   |  138 +++
 .../graphics/chromium/UniscribeHelperTextRun.h     |   96 +++
 9 files changed, 2144 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 26744 [details]
patch v1

Indent (and static_cast)
 static inline float scaleEmToUnits(float x, int unitsPerEm)
 50 {
 51   return unitsPerEm ? x / (float)unitsPerEm : x;
 52 }

No need for the c-style cast:
126     // TEXTMETRICS have this.  Set m_treatAsFixedPitch based off that.
 127     HDC dc = GetDC((HWND)0);
0 works fine w/o cast

This code is copied at least twice:
59     TEXTMETRIC textMetric = {0};
 60     if (!GetTextMetrics(dc, &textMetric)) {
 61         if (ChromiumBridge::ensureFontLoaded(m_font.hfont())) {
 62             // Retry GetTextMetrics.
 63             // FIXME: Handle gracefully the error if this call also fails.
 64             // See http://crbug.com/6401.
 65             if (!GetTextMetrics(dc, &textMetric))
 66                 ASSERT_NOT_REACHED();
 67         }
 68     }
maybe it should be a static function?

Darin fisher learned I was wrong here:
   else
 71         // hack taken from the Windows port
 72         m_xHeight = static_cast<float>(m_ascent) * 0.56;
comments in else/if blocks supposedly cause them to be treated as multi-line by the style guides.  At least that was darin adlers opinion which darin fisher learned earlier this week.

spacing:
 static bool treatAsSpace(UChar c)
 46 {
 47    return c == ' ' || c == '\t' || c == '\n' || c == 0x00A0;
 48 }
 49 

numItems:
 478         int num_items = 0;
I think the code really means numberOfGlyphs? but I'm not sure.

{ on next line:
5     virtual bool nextWinFontData(HFONT*, SCRIPT_CACHE**, SCRIPT_FONTPROPERTIES**, int* ascent) {
 346         return false;
 347     }

Looks fine.

[Darin Adler]

(In reply to comment #2)
> Darin fisher learned I was wrong here:
>    else
>  71         // hack taken from the Windows port
>  72         m_xHeight = static_cast<float>(m_ascent) * 0.56;
> comments in else/if blocks supposedly cause them to be treated as multi-line by
> the style guides.  At least that was darin adlers opinion which darin fisher
> learned earlier this week.

Yes, right, we clarified this a while back. What is considered a one-line control cause is the number of lines of code, including comments, not the number of statements. The guideline is for the human being reading the code, not for the compiler.

If someone's keeping a list of things we should clarify in the code style guidelines document, I guess that should be one of them.

[Dimitri Glazkov (Google)]

Comments addressed, committed in http://trac.webkit.org/changeset/39941