Bug 92901

Summary: [chromium] Move compositor HUD font atlas generation out of compositor core
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, nduca, shawnsingh, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch enne: review+

Description James Robinson 2012-08-01 12:49:30 PDT
[chromium] Move compositor HUD font atlas generation out of compositor core
Comment 1 James Robinson 2012-08-01 12:51:14 PDT
Created attachment 155864 [details]
Patch
Comment 2 Adrienne Walker 2012-08-01 13:28:53 PDT
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.
Comment 3 Shawn Singh 2012-08-01 13:48:03 PDT
(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.
Comment 4 James Robinson 2012-08-01 15:19:45 PDT
(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.
Comment 5 James Robinson 2012-08-01 15:26:40 PDT
Committed r124375: <http://trac.webkit.org/changeset/124375>