Summary: | Chromium port doesn't support opacity on complex text | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brett Wilson (Google) <brettw> | ||||||
Component: | Platform | Assignee: | Brett Wilson (Google) <brettw> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Attachments: |
|
Description
Brett Wilson (Google)
2009-03-23 08:01:41 PDT
Created attachment 28852 [details]
Patch
Comment on attachment 28852 [details]
Patch
ChangeLog needed.
I'm surprised there isn't a test to cover this already?
WK style is "init()":
void Init();
I'm unclear as to why Init needs to be separate from construction? (And I'm also not sure why a generalized "init" function is preferred over the old initForGDI())
Otherwise this looks fine. I think mostly it just needs a ChangeLog to document what you're doing (so the reviewer can more easily understand what you're doing). ;)
Comment on attachment 28852 [details] Patch This looks good, except for these nits: Don't forget the ChangeLog entry. > + TransparencyAwareFontPainter(GraphicsContext* context, const FloatPoint& point); No need for argument names in declaration unless they need additional explanation. > +protected: > + // Called by our subclass' constructor to initialize GDI if necessary. This > + // is a separate function so it can be called after the subclass finishes > + // construction (it calls virtual functions). > + void Init(); init(); > + public: > + TransparencyAwareGlyphPainter(GraphicsContext* context, > + const SimpleFontData* font, > + const GlyphBuffer& glyphBuffer, > + int from, int numGlyphs, > + const FloatPoint& point); Same here, args don't need names, except for "from" and "numGlyphs". > + bool drawGlyphs(int numGlyphs, const WORD* glyphs, const int* advances, > + int startAdvance) const; Ditto. > + return IntRect(m_point.x() - (m_font->ascent() + m_font->descent()) / 2, > + m_point.y() - m_font->ascent() - m_font->lineGap(), > + totalWidth + m_font->ascent() + m_font->descent(), > + m_font->lineSpacing()); Is this computation the same for Uniscribe, just with different params? Maybe we could have a local static here to keep it in one place? > + TransparencyAwareUniscribePainter(GraphicsContext* context, > + const Font* font, > + const TextRun& run, > + int from, int to, > + const FloatPoint& point); Same here about args. (In reply to comment #2) > I'm unclear as to why Init needs to be separate from construction? (And I'm > also not sure why a generalized "init" function is preferred over the old > initForGDI()) This is documented in the comment above the Init declaration: + // Called by our subclass' constructor to initialize GDI if necessary. This + // is a separate function so it can be called after the subclass finishes + // construction (it calls virtual functions). Created attachment 28936 [details]
Patch v2
Comment on attachment 28936 [details]
Patch v2
Nice.
|