Bug 60185 – [Cairo] Move the global alpha property from GraphicsContext to PlatformContextCairo

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Since Cairo is the only platform using the global alpha property of the GraphicsContextState, we should move this to PlatformContextCairo. This will reduce the number of #ifdefs in GraphicsContext.h.

[Martin Robinson]

Created attachment 92283 [details]
Patch

[Dirk Schulze]

Comment on attachment 92283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92283&action=review

Does plaformContext depend on the current state? Does save() and restore() push and pop the globalalpha like expected?
r- because of style issues.

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:120
> +            if (platformContext->globalAlpha() < 1.0f) {

s/1.0f/1/

looks like you need globalAlpha for every condition and mostly more than once. Maybe you can save it in a local variable before the if?

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:129
> +            if (platformContext->globalAlpha() < 1.0f) {

Ditto

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:150
> +            if (platformContext->globalAlpha() < 1.0f) {

Ditto

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.h:75
> +    float m_globalAlpha;

no initial value 1?

[Martin Robinson]

Created attachment 92321 [details]
Patch

[Martin Robinson]

Comment on attachment 92283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92283&action=review

Thanks very much for the quick review, Dirk. I did indeed neglect to address save()/restore() pairs in my previous attempt. I have fixed that in my latest patch by making the image mask stack a more general "state" stack. This matches what Chromium dose for PlatformContextSkia.

>> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:120
>> +            if (platformContext->globalAlpha() < 1.0f) {
> 
> s/1.0f/1/
> 
> looks like you need globalAlpha for every condition and mostly more than once. Maybe you can save it in a local variable before the if?

Fixed!

>> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:129
>> +            if (platformContext->globalAlpha() < 1.0f) {
> 
> Ditto

Fixed!

>> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:150
>> +            if (platformContext->globalAlpha() < 1.0f) {
> 
> Ditto

Fixed!

>> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.h:75
>> +    float m_globalAlpha;
> 
> no initial value 1?

Fixed this as well.

[Dirk Schulze]

Comment on attachment 92321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92321&action=review

r=me.

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:3
> + * Copyright (c) 2008, Google Inc. All rights reserved.

Why did you add googles copy right here?

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:143
> +float PlatformContextCairo::globalAlpha() const
> +{
> +    return m_state->m_globalAlpha;
> +}
> +
> +void PlatformContextCairo::setGlobalAlpha(float globalAlpha)
> +{
> +    m_state->m_globalAlpha = globalAlpha;
> +}

Is this common style for Gtk port? These one-liners can be moved to the headers IMHO. But I'm fine if it is the Gtk way to do it.

[Martin Robinson]

(In reply to comment #5)

> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:3
> > + * Copyright (c) 2008, Google Inc. All rights reserved.
> 
> Why did you add googles copy right here?

Some of these changes are similar enough to the Skia version that I thought it would be better to move the copyright. I was consulting the Skia version while writing this one.

> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:143
> > +float PlatformContextCairo::globalAlpha() const
> > +{
> > +    return m_state->m_globalAlpha;
> > +}
> > +
> > +void PlatformContextCairo::setGlobalAlpha(float globalAlpha)
> > +{
> > +    m_state->m_globalAlpha = globalAlpha;
> > +}
> 
> Is this common style for Gtk port? These one-liners can be moved to the headers IMHO. But I'm fine if it is the Gtk way to do it.

Putting these in the header file would require including GraphicsContext.h there as well potentially increasing compilation time. I'm happy to move them though, if you think it's a good trade off.

[Martin Robinson]

(In reply to comment #6)

> Putting these in the header file would require including GraphicsContext.h there as well potentially increasing compilation time. I'm happy to move them though, if you think it's a good trade off.

Actually, I'm wrong about this. They must be in the C++ file, since m_stack is only defined there.

[Dirk Schulze]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > Putting these in the header file would require including GraphicsContext.h there as well potentially increasing compilation time. I'm happy to move them though, if you think it's a good trade off.
> 
> Actually, I'm wrong about this. They must be in the C++ file, since m_stack is only defined there.

Ok, than I'm fine with the patch. Land it as it is.

[Martin Robinson]

Committed r87336: <http://trac.webkit.org/changeset/87336>