Closed Bug 985220 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Text, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(3 files)

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. :)
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.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8393439 - Flags: review?(jdaggett)
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...
(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 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.
Attachment #8393439 - Flags: review?(jdaggett) → review+
(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.
Are we ready to land?
Flags: needinfo?(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...
Flags: needinfo?(jfkthame)
Rebased and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b2dd6b5048
Target Milestone: --- → mozilla32
https://hg.mozilla.org/mozilla-central/rev/59b2dd6b5048
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1018034
Blocks: 1018551
Depends on: 1020826
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf33b3e3d5e due to Thai regression described in bug 1020826.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/5cf33b3e3d5e
Target Milestone: mozilla32 → ---
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).
Attachment #8437509 - Flags: review?(jdaggett)
Suggested fix for harfbuzz mark-width-zeroing behavior, needed for Thai bitmap font to render properly.
Attachment #8437514 - Flags: review?(mozilla)
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.
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.
Attachment #8437509 - Flags: review?(jdaggett) → review+
Attachment #8437514 - Flags: review?(mozilla) → review+
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.
Target Milestone: --- → mozilla33
Depends on: 1045139
Depends on: 1105807
You need to log in before you can comment on or make changes to this bug.