Bug 24757 – Chromium port doesn't support opacity on complex text

• Component: Platform
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

It isn't hooked up to the TransparencyWin object which handles the text transparency.

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=8768

[Brett Wilson (Google)]

Created attachment 28852 [details]
Patch

[Eric Seidel (no email)]

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). ;)

[Dimitri Glazkov (Google)]

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.

[Brett Wilson (Google)]

(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).

[Brett Wilson (Google)]

Created attachment 28936 [details]
Patch v2

[Dimitri Glazkov (Google)]

Comment on attachment 28936 [details]
Patch v2

Nice.

[Brett Wilson (Google)]

Fixed in r41978