[webkit-reviews] review denied: [Bug 45599] [Cairo] Generalize ContextShadow from the Qt port and use it for shadow code : [Attachment 67804] Patch with Alex's suggestions

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 45599: [Cairo] Generalize ContextShadow from the Qt port and use it for
shadow code
https://bugs.webkit.org/show_bug.cgi?id=45599

Attachment 67804: Patch with Alex's suggestions
https://bugs.webkit.org/attachment.cgi?id=67804&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=67804&action=review

Ok, great patch. I would like to see Ariya or someother guy of the Qt folks to
look at the qt changes. Otherwise looks good, please fix the style issues. Did
not look at the style bot results. r- for the issues mentioned above. If no Qt
guy is available, I look at the qt changes again.

> WebCore/platform/graphics/ContextShadow.cpp:33
> +#include "MathExtras.h"
> +#include "Timer.h"

shouldn'T it be <wtf/MathExtras.h>? not sure about timer. Why do you need
timer?

> WebCore/platform/graphics/ContextShadow.cpp:57
> -    if (!color.isValid() || !color.alpha()) {
> +    if (!m_color.isValid()) {

Why not chechink for alpha?

> WebCore/platform/graphics/ContextShadow.cpp:76
> +    m_offset = FloatSize(0, 0);

Just FloatSize()

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:49
> +// ContextShadow needs a scratch image as the buffer for the blur filter.
> +// Instead of creating and destroying the buffer for every operation,
> +// we create a buffer which will be automatically purged via a timer.
> +class PurgeScratchBufferTimer : public TimerBase {
> +private:

Ah here you need the timer, can the include get removed on th platform
independent file?