This requires webrender support, tracked here: https://github.com/servo/webrender/issues/1690

There are ~400 reftest failures associated with this feature.

40 more loose failures

This implements the Gecko-side changes for WebRender writing modes. It depends on WR PR https://github.com/servo/webrender/pull/2288 The main thing is that normal Gecko writing modes are implemented by a DrawTarget transform, while doing the actual glyph layout in local space. However, we're trying to avoid using DT transforms to do this for WebRender. To that effect, support for a specifying FontInstanceFlags::(TRANSPOSE/FLIP_X/FLIP_Y) via GlyphOptions was added, so that this can be temporarily set on the TextDrawTarget while we need to render these differently oriented glyphs. The catch is that we still need to the layout step in the transformed/vertical direction instead of horizontally in local space, so there's a bit of extra miss in thebes/gfxFont therein. As part of this, I also made synthetic italics specified via this new GlyphOptions flags mechanism rather than having to laboriously plumb that through every single individual ScaledFont constructor.

Comment on attachment 8908775 [details] reftest failures.txt Old data, obsoleting

Comment on attachment 8941937 [details] [diff] [review] support text writing modes with WebRender Review of attachment 8941937 [details] [diff] [review]: ----------------------------------------------------------------- Disclaimer: I skimmed over all the changes to oblique passing; it seems fine but I'm basically assuming that if it's not it will be obvious. This patch is really subtle, and I don't fully understand the changes to gfxFont.cpp (would need to spend several more hours going over all the flags and subroutines involved). I don't think we should land this without at least more comments/docs. I know this file is already super gnarly but I'd like to not exacerbate it too much (up until now the changes we've made when textDrawer is present have been fairly tame/local). Are there tests that really stress the oblique+vertical cases that seem so special here? ::: gfx/thebes/gfxFont.cpp @@ +1900,1 @@ > inlineCoord += aBuffer.mRunParams.isRTL ? -space : space; These two lines next to eachother really screams that something terrible is happening. @@ +2128,5 @@ > + fontParams.isVerticalFont = > + aOrientation == gfx::ShapedTextFlags::TEXT_ORIENT_VERTICAL_UPRIGHT; > + fontParams.layoutDirection = 1.0f; > + baselineDir = 1.0f; > + } These two branches are really subtle/delicate, can we add a comment here (or somewhere?) ((as of writing this comment I don't yet understand it)) @@ +2154,5 @@ > + wr::FontInstanceFlags::FLIP_Y : > + wr::FontInstanceFlags::FLIP_X)); > + if (aTextRun->UseCenterBaseline()) { > + const Metrics& metrics = GetMetrics(eHorizontal); > + float baseAdj = (metrics.emAscent - metrics.emDescent) / 2; hmm, it feels like some common code wants to be refactored here, but it seems like it's going to be messy either way. @@ -2217,5 @@ > - if (mStyle.baselineOffset != 0.0) { > - baseline += > - mStyle.baselineOffset * aTextRun->GetAppUnitsPerDevUnit(); > - } > - Why isn't this needed anymore in the !textDrawer case? ::: layout/generic/TextDrawTarget.h @@ +111,5 @@ > + { > + if (!mTarget) { > + mTarget = aTarget; > + mFlags = aTarget->GetWRGlyphFlags(); > + } The fact that we're genuinely relying on this to keep the oldest save feels really subtle... is this a common idiom with the other autosavers? Can you leave a comment explaining that doing this isn't a bug? (when I first saw this I was going to suggest an assertion)

Version 2. This should address all the review comments, I believed. I added a bunch of comments and tried to refactor things a bit to make them clearer.

Comment on attachment 8942976 [details] [diff] [review] support text writing modes with WebRender Review of attachment 8942976 [details] [diff] [review]: ----------------------------------------------------------------- \o/ this is great! Just some minor typos and a couple things to double check before landing. ::: gfx/thebes/gfxFont.cpp @@ +2019,5 @@ > + if (aFontParams.needsOblique) { > + if (textDrawer) { > + glyphFlagsRestore.Save(textDrawer); > + textDrawer->SetWRGlyphFlags(textDrawer->GetWRGlyphFlags() & > + ~wr::FontInstanceFlags::SYNTHETIC_ITALICS); Shouldn't this be `flags | SYTHETIC_ITALICS`? (as is it looks like you're *unsetting* the flag?) @@ +2149,5 @@ > + -1.0f : > + (aOrientation == gfx::ShapedTextFlags::TEXT_ORIENT_VERTICAL_SIDEWAYS_RIGHT ? > + 1.0f : 0.0f)); > + // If we're rendering a sideways run, we need to push a rotation transform to the context. > + if (sidewaysDir != 0.0f) { Wow this is so much more clear than the old condition! :) @@ +2154,5 @@ > if (textDrawer) { > + // For WebRender, we can't use a DrawTarget transform and must instead use flags > + // that locally transform the glyph, without affecting the glyph origin. The glyph > + // origins must thus be offset in the transformed directions (instead of local-space > + // directions). Modify the advance and baseline directions must to account for the delete "must" (typo?) @@ +2166,5 @@ > + // rotated right. > + fontParams.advanceDirection *= sidewaysDir; > + // The baseline direction (moving down) is negated relative to the > + // advance direction for sideways transforms. > + baselineDir *= -sidewaysDir; This could be an `=`, right? not sure if that's more clear. @@ +2169,5 @@ > + // advance direction for sideways transforms. > + baselineDir *= -sidewaysDir; > + > + glyphFlagsRestore.Save(textDrawer); > + // Set the transform flags according. Both sideways rotations transpose X and Y, accordingly @@ +2207,5 @@ > // [1] See http://www.microsoft.com/typography/otspec/base.htm > if (aTextRun->UseCenterBaseline()) { > + const Metrics& metrics = GetMetrics(eHorizontal); > + float baseAdj = (metrics.emAscent - metrics.emDescent) / 2; > + baseline += baseAdj * aTextRun->GetAppUnitsPerDevUnit() * baselineDir; Why didn't the old PreTranslate need this unit conversion?

(In reply to Alexis Beingessner [:Gankro] from comment #7) > Comment on attachment 8942976 [details] [diff] [review] > support text writing modes with WebRender > > Review of attachment 8942976 [details] [diff] [review]: > ----------------------------------------------------------------- > > \o/ this is great! > > Just some minor typos and a couple things to double check before landing. > > ::: gfx/thebes/gfxFont.cpp > @@ +2019,5 @@ > > + if (aFontParams.needsOblique) { > > + if (textDrawer) { > > + glyphFlagsRestore.Save(textDrawer); > > + textDrawer->SetWRGlyphFlags(textDrawer->GetWRGlyphFlags() & > > + ~wr::FontInstanceFlags::SYNTHETIC_ITALICS); > > Shouldn't this be `flags | SYTHETIC_ITALICS`? (as is it looks like you're > *unsetting* the flag?) It was supposed to unset it while drawing the missing glyphs. But technically, given how I am going to do missing glyphs, this is not even necessary to unset. So I'll just remove this bit of code entirely. > @@ +2149,5 @@ > > + -1.0f : > > + (aOrientation == gfx::ShapedTextFlags::TEXT_ORIENT_VERTICAL_SIDEWAYS_RIGHT ? > > + 1.0f : 0.0f)); > > + // If we're rendering a sideways run, we need to push a rotation transform to the context. > > + if (sidewaysDir != 0.0f) { > > Wow this is so much more clear than the old condition! :) > > @@ +2154,5 @@ > > if (textDrawer) { > > + // For WebRender, we can't use a DrawTarget transform and must instead use flags > > + // that locally transform the glyph, without affecting the glyph origin. The glyph > > + // origins must thus be offset in the transformed directions (instead of local-space > > + // directions). Modify the advance and baseline directions must to account for the > > delete "must" (typo?) > > @@ +2166,5 @@ > > + // rotated right. > > + fontParams.advanceDirection *= sidewaysDir; > > + // The baseline direction (moving down) is negated relative to the > > + // advance direction for sideways transforms. > > + baselineDir *= -sidewaysDir; > > This could be an `=`, right? not sure if that's more clear. Yes, but for parallelism with advanceDirection I just did the same there. > @@ +2169,5 @@ > > + // advance direction for sideways transforms. > > + baselineDir *= -sidewaysDir; > > + > > + glyphFlagsRestore.Save(textDrawer); > > + // Set the transform flags according. Both sideways rotations transpose X and Y, > > accordingly > > @@ +2207,5 @@ > > // [1] See http://www.microsoft.com/typography/otspec/base.htm > > if (aTextRun->UseCenterBaseline()) { > > + const Metrics& metrics = GetMetrics(eHorizontal); > > + float baseAdj = (metrics.emAscent - metrics.emDescent) / 2; > > + baseline += baseAdj * aTextRun->GetAppUnitsPerDevUnit() * baselineDir; > > Why didn't the old PreTranslate need this unit conversion? Because the point is initially in app units, while the matrix is in device units. The metrics here are meant to be in device units, so when originally it was just modifying the matrix, no scaling was needed. But since we're modifying the point coordinate here, we need to convert from device units back to app units (hence GetAppUnitsPerDevUnit).

Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/58b31f942b51 support text writing modes with WebRender. r=gankro

Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5cb791416b7 only invert oblique transform on missing glyph for vertical writing mode. r=me