[Webkit-unassigned] [Bug 23526] [CAIRO] Support ImageBuffers clip operation on all Cairo ports

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23526





--- Comment #42 from Martin Robinson <mrobinson at webkit.org>  2011-02-24 08:31:47 PST ---
(In reply to comment #36)

Thanks for the comments!

> (From update of attachment 83492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83492&action=review
> 
> Looks great to me, I'll leave some notes, as I'm not yet certain whether it's r+, as I'm not too familiar with the Cairo port.
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:240
> > +    if (maskInformation.valid()) {
> 
> s/valid/isValid/

Done!

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:255
> > +    cairo_restore(m_data->cr);
> > +    m_data->restore();
> 
> Why do you need, to do anything if maskInformation was not valid at this point? If no mask is used at all, you're now doing one cairo_restore/restore more, is that desired/needed?

I just moved these two lines from the top of the method. We still need to restore the platform state even if no masking operation was started after the call to savePlatformState. restorePlatformState is just the most reasonable time to pop the group and apply the mask since we know this is where it will end. It's still a distinct operation from the save/restore pair though.

> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:59
> > +    bool valid() const { return m_maskSurface; }
> 
> s/valid/isValid/

Done!

> > Source/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:125
> >      ContextShadow shadow;
> >      Vector<ContextShadow> shadowStack;
> > +    Vector<ImageMaskInformation> m_maskImageStack;
> 
> This is inconsistent, either make all of them private (I think this is out of scope for this bug), or just name it maskImageStack as well, and put a FIXME: Make these private. above the ContextShadow line.

Changed this to be simply maskImageStack. Some of the members of this class use the m_namingScheme and some don't. You're right that another patch should address that. I've opened https://bugs.webkit.org/show_bug.cgi?id=55150.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.