Bug 92901 – [chromium] Move compositor HUD font atlas generation out of compositor core

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Move compositor HUD font atlas generation out of compositor core

[James Robinson]

Created attachment 155864 [details]
Patch

[Adrienne Walker]

Comment on attachment 155864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155864&action=review

R=me, with some drive-by cleanup comments on code you're moving around.

> Source/WebCore/platform/graphics/chromium/CompositorHUDFontAtlas.cpp:43
> +#define FONT_HEIGHT 14

Unused?

> Source/WebCore/platform/graphics/chromium/CompositorHUDFontAtlas.cpp:59
> +    fontHeight = 14;
> +
> +    OwnPtr<SkCanvas> canvas = adoptPtr(skia::CreateBitmapCanvas(ATLAS_SIZE, ATLAS_SIZE, false /* opaque */));

This baked in font height and atlas size is kind of a mess, but it's not your fault.

[Shawn Singh]

(In reply to comment #2)
> (From update of attachment 155864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155864&action=review
> 
> R=me, with some drive-by cleanup comments on code you're moving around.
> 
> > Source/WebCore/platform/graphics/chromium/CompositorHUDFontAtlas.cpp:43
> > +#define FONT_HEIGHT 14
> 
> Unused?
> 
> > Source/WebCore/platform/graphics/chromium/CompositorHUDFontAtlas.cpp:59
> > +    fontHeight = 14;
> > +
> > +    OwnPtr<SkCanvas> canvas = adoptPtr(skia::CreateBitmapCanvas(ATLAS_SIZE, ATLAS_SIZE, false /* opaque */));
> 
> This baked in font height and atlas size is kind of a mess, but it's not your fault.

Ouch... that was clearly directed at me.  =(

At the time I coded this, we didn't (and we still don't, i think?) care about how flexible and powerful the HUD code was.  So I opted to make those variables into private macros so that they should be easy to make into run-time args that are given to the font atlas when we really want to.  As long as we also handle the error of when the font size cannot fully fit into the desired atlas size.

Feel free to make a bug on me if you had some reason for us to refactor it.

[James Robinson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 155864 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155864&action=review
> > 
> > R=me, with some drive-by cleanup comments on code you're moving around.
> > 
> > > Source/WebCore/platform/graphics/chromium/CompositorHUDFontAtlas.cpp:43
> > > +#define FONT_HEIGHT 14
> > 
> > Unused?

Will remove, thanks for catching it.

> > 
> > > Source/WebCore/platform/graphics/chromium/CompositorHUDFontAtlas.cpp:59
> > > +    fontHeight = 14;
> > > +
> > > +    OwnPtr<SkCanvas> canvas = adoptPtr(skia::CreateBitmapCanvas(ATLAS_SIZE, ATLAS_SIZE, false /* opaque */));
> > 
> > This baked in font height and atlas size is kind of a mess, but it's not your fault.
> 
> Ouch... that was clearly directed at me.  =(
> 
> At the time I coded this, we didn't (and we still don't, i think?) care about how flexible and powerful the HUD code was.  So I opted to make those variables into private macros so that they should be easy to make into run-time args that are given to the font atlas when we really want to.  As long as we also handle the error of when the font size cannot fully fit into the desired atlas size.
> 
> Feel free to make a bug on me if you had some reason for us to refactor it.

I don't think any action is required, it's good enough for what we need.

[James Robinson]

Committed r124375: <http://trac.webkit.org/changeset/124375>