Summary: | [chromium] Move compositor HUD font atlas generation out of compositor core | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||
Component: | New Bugs | Assignee: | 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
James Robinson
2012-08-01 12:49:30 PDT
Created attachment 155864 [details]
Patch
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. (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. (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. Committed r124375: <http://trac.webkit.org/changeset/124375> |