Bug 44031 – [Qt] Save and restore shadow state in GraphicsContextQt

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ariya Hidayat]

This is the next step after https://bugs.webkit.org/show_bug.cgi?id=44006. We should save and restore shadow parameters properly. We will not use GraphicsContextState for this because we may want to cache some stuff useful for shadow blur.

[Ariya Hidayat]

Created attachment 64459 [details]
Patch

[WebKit Review Bot]

Attachment 64459 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/qt/GraphicsContextQt.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 64459 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3778194

[Ariya Hidayat]

Created attachment 64461 [details]
Patch

[Ariya Hidayat]

(In reply to comment #3)
> Attachment 64459 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/3778194

This is bogus, since this patch requires https://bugs.webkit.org/show_bug.cgi?id=44015 first to be applied.

[Early Warning System Bot]

Attachment 64461 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3717221

[Ariya Hidayat]

(In reply to comment #6)
> Attachment 64461 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/3717221

Please ignore this, the patch can't be built before https://bugs.webkit.org/show_bug.cgi?id=44015,

[Antonio Gomes]

Comment on attachment 64461 [details]
Patch

> [phone]  Ariya Hidayat  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Save and restore shadow state in GraphicsContextQt
> +        https://bugs.webkit.org/show_bug.cgi?id=44031
> +
> +        * platform/graphics/qt/GraphicsContextQt.cpp:

Ariya, could you enrich you ChangeLog?
  
> +
> +// This is to track and keep the shadow state. We use this rather than
> +// using GraphicsContextState to allow possible optimizations (right now
> +// only to determine the shadow type, but in future it might covers things
> +// like cached scratch image, persistent shader, etc).

/s/covers/cover

> +        : color(c)
> +        , blurRadius(qRound(r))
> +        , offset(dx, dy)
> +    {
> +        // The type of shadow is decided by the blur radius, shadow offset, and shadow color.
> +        if (!color.isValid() || !color.alpha()) {
> +            // Can't paint the shadow with invalid or invisible color.
> +            type = NoShadow;
> +        } else {
> +            if (r > 0) {
> +                // Shadow is always blurred, even the offset is zero.
> +                type = BlurShadow;
> +            } else {
> +                if (offset.isNull()) {
> +                    // Without blur and zero offset means the shadow is fully hidden.
> +                    type = NoShadow;
> +                } else {
> +                    if (color.alpha() > 0)
> +                        type = AlphaSolidShadow;
> +                    else
> +                        type = OpaqueSolidShadow;
> +                }
> +            }
> +        }
> +    }


If you use "else if"  instead of "else { \n\TAB\ if .." you would look better, identation-wise.

> +
> +    void clear()
> +    {
> +        type = NoShadow;
> +        color = QColor();
> +        blurRadius = 0;
> +        offset = QPointF(0, 0);

Lets use QPoingF() here.

r=me with the above fixed! Nice split up / group up.

[Ariya Hidayat]

Comment on attachment 64461 [details]
Patch

Manually committed r65393: http://trac.webkit.org/changeset/65393

[Ariya Hidayat]

Reverted r65393 for reason:

Breaks some canvas tests

Committed r65394: <http://trac.webkit.org/changeset/65394>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65393 might have broken Qt Linux Release

[Ariya Hidayat]

Created attachment 64468 [details]
Patch

[Ariya Hidayat]

Comment on attachment 64468 [details]
Patch

The updated patch should fix the problem with canvas tests (the reason I rolled out the previous patch, http://trac.webkit.org/changeset/65394).

Here is the small diff of the fix:

diff --git a/WebCore/platform/graphics/qt/GraphicsContextQt.cpp b/WebCore/platform/graphics/qt/GraphicsContextQt.cpp
index 43fc32f..d30ea66 100644
--- a/WebCore/platform/graphics/qt/GraphicsContextQt.cpp
+++ b/WebCore/platform/graphics/qt/GraphicsContextQt.cpp
@@ -920,9 +920,10 @@ void GraphicsContext::setPlatformShadow(const FloatSize& size, float blur, const
         // Meaning that this graphics context is associated with a CanvasRenderingContext
         // We flip the height since CG and HTML5 Canvas have opposite Y axis
         m_common->state.shadowSize = FloatSize(size.width(), -size.height());
+        m_data->shadow = ContextShadowParameter(color, blur, size.width(), -size.height());
+    } else {
+        m_data->shadow = ContextShadowParameter(color, blur, size.width(), size.height());
     }
-
-    m_data->shadow = ContextShadowParameter(color, blur, size.width(), size.height());
 }

[Antonio Gomes]

Comment on attachment 64468 [details]
Patch

r=me

[Ariya Hidayat]

Comment on attachment 64468 [details]
Patch

Clearing flags on attachment: 64468

Committed r65420: <http://trac.webkit.org/changeset/65420>

[Ariya Hidayat]

All reviewed patches have been landed.  Closing bug.