Bug 60185

Summary: [Cairo] Move the global alpha property from GraphicsContext to PlatformContextCairo
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, gns, krit, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Description Martin Robinson 2011-05-04 10:46:10 PDT
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.
Comment 1 Martin Robinson 2011-05-04 10:59:06 PDT
Created attachment 92283 [details]
Patch
Comment 2 Dirk Schulze 2011-05-04 11:12:08 PDT
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?
Comment 3 Martin Robinson 2011-05-04 14:54:12 PDT
Created attachment 92321 [details]
Patch
Comment 4 Martin Robinson 2011-05-04 14:56:07 PDT
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.
Comment 5 Dirk Schulze 2011-05-23 01:38:38 PDT
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.
Comment 6 Martin Robinson 2011-05-25 09:29:27 PDT
(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.
Comment 7 Martin Robinson 2011-05-25 09:30:42 PDT
(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.
Comment 8 Dirk Schulze 2011-05-25 10:12:10 PDT
(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.
Comment 9 Martin Robinson 2011-05-25 16:54:11 PDT
Committed r87336: <http://trac.webkit.org/changeset/87336>