[CAIRO] globalAlpha has to be stored and restored

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Schulze]

globalAlpha has to be stored and restored after calling save() or restore().

[Dirk Schulze]

Created attachment 24673 [details]
save/restore globalAlpha

I added a stack (vector) to GraphicsContext to save and restore globalAlpha values on calling ctx.save() or ctx.restore().

[Dirk Schulze]

Created attachment 24706 [details]
save/restore globalAlpha

there was a little bug in GraphicsContext::setAlpha()

[Alp Toker]

(In reply to comment #2)
> Created an attachment (id=24706) [edit]
> save/restore globalAlpha
> 
> there was a little bug in GraphicsContext::setAlpha()
> 

GraphicsContext already keeps a stack with internal state (see GraphicsContext.cpp GraphicsContext::save() and GraphicsContextPrivate.h). I was thinking it might be better to add an 'alpha' field to that and let GraphicsContext do the work for us so we don't need to maintain our own stack of alpha values for GraphicsContextCairo. Could you take a look into that?

[Dirk Schulze]

Created attachment 24717 [details]
save/restore globalAlpha

(In reply to comment #3)
> GraphicsContext already keeps a stack with internal state (see
> GraphicsContext.cpp GraphicsContext::save() and GraphicsContextPrivate.h). I
> was thinking it might be better to add an 'alpha' field to that and let
> GraphicsContext do the work for us so we don't need to maintain our own stack
> of alpha values for GraphicsContextCairo. Could you take a look into that?

I spoke to esedeidel via IRC as I implemented the current version of globalAlpha. And he didn't like the idea to change GraphicsContextPrivate.h. A globalAlpha variable is only needed on cairo and would waste space for other platforms.
Or we use #if PLATFORM(CAIRO).

This second patch uses the alternative. Only One patch should be reviewed.

[Alp Toker]

Comment on attachment 24717 [details]
save/restore globalAlpha

This version of the patch looks pretty good. It makes sense to use the WebCore graphics state stack here rather than maintaining separate stacks that have to be maintained manually. Some comments..


>Index: WebCore/platform/graphics/GraphicsContextPrivate.h
>===================================================================
>--- WebCore/platform/graphics/GraphicsContextPrivate.h	(revision 37888)
>+++ WebCore/platform/graphics/GraphicsContextPrivate.h	(working copy)
>@@ -50,6 +50,9 @@ namespace WebCore {
>             : textDrawingMode(cTextFill)
>             , strokeStyle(SolidStroke)
>             , strokeThickness(0)

This would usually be '1.0f':

>+#if PLATFORM(CAIRO)
>+            , globalAlpha(1)
>+#endif


I think we'll still need to go through this code some time later and make sure we're using globalAlpha consistently everywhere it's needed, and to avoid creating temporary surfaces (which can be inefficient) where globalAlpha can just be multiplied with the pen color's alpha, but that doesn't really block this getting landed.

Going to wait for the build to get fixed before r+'ing and to give anyone else who's interested a chance to comment.

[Alp Toker]

Comment on attachment 24706 [details]
save/restore globalAlpha

r-. Other patch you proposed is the way to go.

[Alp Toker]

Comment on attachment 24717 [details]
save/restore globalAlpha

r=me

Will change the '0' to '1.0f'.

[Alp Toker]

Landed in r38001.

Dirk: I did notice a performance issue we might want to look at.

   case SolidColorSpace:
        if (strokeColor().alpha()) {
            setColor(cr, strokeColor());
            if (m_common->state.globalAlpha < 1.0f) {
                cairo_push_group(cr);
                cairo_paint_with_alpha(cr, m_common->state.globalAlpha);
                cairo_pop_group_to_source(cr);
            }
            cairo_stroke(cr);
        }
        break;

^ Temporary groups are pretty expensive in Cairo IIRC, especially if they're not clipped. It would be better to just change setColor(cr, strokeColor()) to set the cairo color to the right alpha value and stroke etc. directly. Anyway, that's a different issue for a different bug, just thought I'd let you know.

Cheers