Closed Bug 1470515 Opened 2 years ago Closed 2 years ago

ScaledFont plumbing for Android WebRender


(Core :: Graphics: WebRender, enhancement)

Not set



Tracking Status
firefox63 --- fixed


(Reporter: lsalzman, Assigned: lsalzman)



(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
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.
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.