Closed Bug 1470515 Opened Last year Closed Last year

ScaledFont plumbing for Android WebRender

Categories

(Core :: Graphics: WebRender, enhancement)

All
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

Details

Attachments

(1 file)

WebRender currently can't work on Android because we don't have the necessary ScaledFont hooks in place.
Android was using ScaledFontCairo, which was only ever used for Android at this point, being mostly vestigial. Likewise, it was under the illusion that it was FontType::SKIA, for reasons that make no sense anymore.

As a start, I made this a ScaledFontFreeType, which is actually FontType::FREETYPE, so that it makes sense to someone trying to read the code.

I cleaned up some code sharing between UnscaledFont(FreeType/Fontconfig) and NativeFontResource(FreeType/Fontconfig).

Finally, ScaledFontFreeType now supports the necessary WR hooks that will let it work with recording/playback as required.
Attachment #8987149 - Flags: review?(rhunt)
Comment on attachment 8987149 [details] [diff] [review]
refactor ScaledFontFreeType for Android WR

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

This looks good to me, just a couple of nits.

::: gfx/2d/UnscaledFontFreeType.cpp
@@ +88,5 @@
> +    return false;
> +  }
> +
> +  const char* path = mFile.c_str();
> +  size_t pathLength = strlen(path);

Can we use mFile.size() here? I'm aware this is copied code, but it'd be nice to fix this.

::: gfx/2d/UnscaledFontFreeType.h
@@ +52,5 @@
>  
>    FT_Face GetFace() const { return mFace; }
>    const char* GetFile() const { return mFile.c_str(); }
>    uint32_t GetIndex() const { return mIndex; }
> +  const RefPtr<NativeFontResource> & GetNativeFontResource() const { return mNativeFontResource; }

nit: Move the & over one character

::: gfx/thebes/gfxFT2FontList.cpp
@@ +79,5 @@
>  // allocate memory to uncompress a font from omnijar.
>  class AutoFTFace {
>  public:
>      explicit AutoFTFace(FT2FontEntry* aFontEntry)
> +        : mFace(nullptr), mFontDataBuf(nullptr), mDataLength(0), mOwnsFace(false)

Is mDataLength needed? I don't see it used anywhere.
Attachment #8987149 - Flags: review?(rhunt) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a20ed6e2fee
refactor ScaledFontFreeType for Android WR. r=rhunt
Fixed the nits. mDataLength is however needed as it is still accessed in some places via the DataLength() accessor.
https://hg.mozilla.org/mozilla-central/rev/4a20ed6e2fee
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.