985220 - remove the Windows platform font shaping backends (GDI/Uniscribe/DWrite) now that we use harfbuzz for all scripts

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

Bug 797405 was fixed for mozilla-28, and bug 964313 (the one exception to "all text" there) was fixed for mozilla-29. Once we're comfortable that these have stuck, without serious shaping regressions, we can remove the various Windows-platform shapers that are no longer being used. Less code = more better. :)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: remove the old GDI, Uniscribe and DWrite text-shaping code paths, as we now use HarfBuzz or Graphite for all shaping on Windows.

This removes all three of the old platform-specific shapers, leaving us with only HB and Gr2 for all text on Windows. Tryserver seems happy with it <https://tbpl.mozilla.org/?tree=Try&rev=2fbe76b3a361>, as is my local build. I suggest we plan to land this on Nightly after Firefox 29 has gone to Release (i.e. end of April; we'd be landing for Firefox 32), so as to have a couple of releases where we could still easily revert the harfbuzz.scripts pref change if problems are discovered.

Comment 2

[John Daggett (:jtd)]

Comment on attachment 8393439 [details] [diff] [review]
remove the old GDI, Uniscribe and DWrite text-shaping code paths, as we now use HarfBuzz or Graphite for all shaping on Windows.


Looks good! Couple small nits.

>          SOURCES += [
>              'gfxD2DSurface.cpp',
>              'gfxDWriteCommon.cpp',
>              'gfxDWriteFontList.cpp',
>              'gfxDWriteFonts.cpp',
> -            'gfxDWriteShaper.cpp',
>              'gfxDWriteTextAnalysis.cpp',
>          ]

I think the gfxDWriteTextAnalysis.* files can be jettisoned also.

>      if (!ok) {
>          if (!mPlatformShaper) {
>              CreatePlatformShaper();
> -            NS_ASSERTION(mPlatformShaper, "no platform shaper available!");
>          }
>          if (mPlatformShaper) {
>              ok = mPlatformShaper->ShapeText(aContext, aText, aOffset, aLength,
>                                              aScript, aShapedText);
>          }
>      }

Hmm, why remove this assertion? Not having a shaper seems sort of, er, not good...

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #2)

> >      if (!ok) {
> >          if (!mPlatformShaper) {
> >              CreatePlatformShaper();
> > -            NS_ASSERTION(mPlatformShaper, "no platform shaper available!");
> >          }
> >          if (mPlatformShaper) {
> >              ok = mPlatformShaper->ShapeText(aContext, aText, aOffset, aLength,
> >                                              aScript, aShapedText);
> >          }
> >      }
> 
> Hmm, why remove this assertion? Not having a shaper seems sort of, er, not
> good...

mPlatformShaper is used only for a platform-specific shaper, which on Windows will no longer exist - the only one left in the tree will be gfxCoreTextShaper on OS X. On non-OS X platforms, we'll -always- create a gfxHarfBuzzShaper; if that fails, the font is simply broken and we can't use it.

(I'm thinking of refactoring this slightly, to eliminate mPlatformShaper from the base gfxFont class altogether, and instead add an mCoreTextShaper to gfxMacFont. This will simplify the basic, cross-platform code, and move the management of the extra, platform-specific shaper entirely into the Mac code, as its only user. But I figured that was best left for a followup, after the Windows shaper removal is safely landed.)

Comment 4

[John Daggett (:jtd)]

Comment on attachment 8393439 [details] [diff] [review]
remove the old GDI, Uniscribe and DWrite text-shaping code paths, as we now use HarfBuzz or Graphite for all shaping on Windows.

ok, sounds fine.

r+ with the removal of the gfxDWriteTextAnalysis.* files.  If that's complicated I'd like to review this again.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #4)
> r+ with the removal of the gfxDWriteTextAnalysis.* files.  If that's
> complicated I'd like to review this again.

Should be fine; it looks like the #include "gfxDWriteTextAnalysis.h" in gfxDWriteFonts.cpp is redundant anyhow, and the only other user is the gfxDWriteShaper.cpp file that's going away.

As noted above, I plan to wait until the mozilla-32 cycle before actually landing this.

Comment 6

[Masatoshi Kimura [:emk]]

Are we ready to land?

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> Are we ready to land?

I've been considering it, but I thought it may be good to wait until FF29 has been out a few weeks. The patch needs a minor update, so I'll go ahead and do that in preparation for landing...

Comment 8

[Jonathan Kew [:jfkthame]]

Rebased and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b2dd6b5048

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59b2dd6b5048

Comment 10

[Jonathan Kew [:jfkthame]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf33b3e3d5e due to Thai regression described in bug 1020826.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/5cf33b3e3d5e

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - replace the use of GetGlyphIndicesW with ScriptGetCMap in gfxGDIFont::GetGlyph.

This is a followup modification to the original patch here that gives us the right glyph IDs for the Thai-codepage version of MS Sans Serif (the problem case from bug 1020826).

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - when zeroing the width of mark glyphs, also update their offset to overstrike preceding glyph.

Suggested fix for harfbuzz mark-width-zeroing behavior, needed for Thai bitmap font to render properly.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8437514 [details] [diff] [review]
pt 3 - when zeroing the width of mark glyphs, also update their offset to overstrike preceding glyph.

Review of attachment 8437514 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/harfbuzz/src/hb-ot-shape-complex-thai.cc
@@ +331,5 @@
>        return;
>  
> +    /* Make Nikhahit be recognized as a mark when zeroing widths. */
> +    unsigned int end = buffer->out_len;
> +    _hb_glyph_info_set_general_category (&buffer->out_info[end - 1], HB_UNICODE_GENERAL_CATEGORY_NON_SPACING_MARK);

Argh, this should of course be [end - 2] to hit the Nikhahit.

Comment 15

[Jonathan Kew [:jfkthame]]

Tryserver build with these patches, including the fix from comment 14 (in addition to relanding the previously backed-out patches here and in bugs 1018034 and 1018551): https://tbpl.mozilla.org/?tree=Try&rev=06db92bf63e2.

Bug 1020826 comment 16 confirms that this fixes the Thai regression we had from the initial landing here.

Comment 16

[Jonathan Kew [:jfkthame]]

Re-landed, with the two followup patches to fix the Thai regression we caused last time:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bced0023b3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d90825e1b51
https://hg.mozilla.org/integration/mozilla-inbound/rev/b35e88d9381e

I plan to reland the followup bugs 1018034 and 1018551 as well, but let's wait to confirm this behaves OK in Nightly first.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5bced0023b3a
https://hg.mozilla.org/mozilla-central/rev/0d90825e1b51
https://hg.mozilla.org/mozilla-central/rev/b35e88d9381e