Bug 56962 – [Chromium] Vertical Japanese text is not displayed on Snow Leopard

• Component: Text
• Status: RESOLVED FIXED

[Mihai Parparita]

Created attachment 86685 [details]
fast/dynamic/text-combine.html in ToT Chromium

The following tests are failing on Chromium on Snow Leopard due to vertical text not being rendered in the right place:

fast/blockflow/border-vertical-lr.html
fast/blockflow/broken-ideograph-small-caps.html
fast/blockflow/broken-ideographic-font.html
fast/blockflow/japanese-lr-selection.html
fast/blockflow/japanese-lr-text.html
fast/blockflow/japanese-rl-selection.html
fast/blockflow/japanese-rl-text-with-broken-font.html
fast/blockflow/japanese-rl-text.html
fast/blockflow/japanese-ruby-vertical-lr.html
fast/blockflow/japanese-ruby-vertical-rl.html
fast/blockflow/Kusa-Makura-background-canvas.html
fast/blockflow/vertical-align-table-baseline.html
fast/blockflow/vertical-baseline-alignment.html
fast/blockflow/vertical-font-fallback.html
fast/dynamic/text-combine.html
fast/repaint/japanese-rl-selection-clear.html
fast/repaint/japanese-rl-selection-repaint.html
fast/repaint/repaint-across-writing-mode-boundary.html
fast/ruby/base-shorter-than-text.html
fast/text/emphasis-combined-text.html
fast/text/justify-ideograph-vertical.html

Expected image: http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/fast/dynamic/text-combine-expected.png
Actual image: http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/dynamic/text-combine-actual.png

Looking at the actual result in ToT Chromium (see attachment), the vertical text is getting rendered, just in the wrong position. However, we do have the correct glyph positions when computing layout, shown by find-in-page highlighting the right area.

[Mihai Parparita]

Based on http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Frepaint%2Frepaint-across-writing-mode-boundary.html&showExpectations=true&showAllRuns=true&group=%40ToT%20-%20chromium.org it looks like this may have started to happen after http://trac.webkit.org/changeset/80582/

[Mihai Parparita]

It looks like this is caused by this block in FontMac.mm:

http://trac.webkit.org/changeset/80740/trunk/Source/WebCore/platform/graphics/mac/FontMac.mm

Unlike Safari (which builds separate binaries for Leopard and Snow Leopard), Chromium always builds with the 10.5 SDK  (and on Leopard?) so we don't end up on the SL path through that code, because BUILDING_ON_SNOW_LEOPARD is not true for us.

At least for Chromium this will have to be a run-time check. Besides ATSUI vs. CoreText (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp#L258), is there any other precedent for Chromium making a run-time switch where the Mac port does it at build time?

[Ojan Vafai]

I don't think it's reasonable to create a runtime configuration every time we use BUILDING_ON_SNOW_LEOPARD/BUILDING_ON_LEOPARD.

This bug also means that we don't correctly render elliptical gradients on Snow Leopard.

Chromium really just needs to create separate binaries for Snow Leopard and Leopard. I don't know the history here to know why we share a binary though.

[Mark Mentovai]

Chromium won’t be creating different binaries for different OS releases.

[Mihai Parparita]

(In reply to comment #4)
> Chromium won’t be creating different binaries for different OS releases.

In that case, for doing these checks at runtime, is using Gestalt with gestaltSystemVersionMajor/Minor still the way to go?

[Mark Mentovai]

If WebKit already has a function to get the OS version, it’s probably better to call that.

If you’re concerned about bloating the object code size for Apple-style WebKit, which is built for a specific OS release, you can use macros to hide your tests. In some cases, you can use this in conjunction with compiler optimizations to keep things clean and workable. For example:

#if BUILD_TIME_CHECK_FOR_LEOPARD
  bool useNewThing = false;
#elif BUILD_TIME_CHECK_FOR_SNOW_LEOPARD
  bool useNewThing = true;
#else
  bool useNewThing = RuntimeCheckForSnowLeopard();
#endif

  if (useNewThing) {
    // ...
  }

Depending on the macros available, it may be easier to write this as

#if CHROMIUM
  bool useNewThing = RuntimeCheckForSnowLeopard();
#elif BUILD_TIME_CHECK_FOR_LEOPARD
  bool useNewThing = false;
// ...

[Mihai Parparita]

Created attachment 86855 [details]
Patch

[Mihai Parparita]

With this patch the tests mentioned in the initial report will start to pass. Some of them will need rebaselines (for both mac and and chromium) since the results are now shifted by one pixel (presumably a side effect of r80582, the mac/ baselines haven't been updated since then).

Rebaselines are not in the uploaded patch since it's too big for Bugzilla.

[James Robinson]

Comment on attachment 86855 [details]
Patch

r=me

[Mihai Parparita]

Committed r81921: <http://trac.webkit.org/changeset/81921>