Bug 44031 - [Qt] Save and restore shadow state in GraphicsContextQt
Summary: [Qt] Save and restore shadow state in GraphicsContextQt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ariya Hidayat
URL:
Keywords: HTML5, Qt, QtTriaged
Depends on: 44015
Blocks: 34479 44025
  Show dependency treegraph
 
Reported: 2010-08-15 07:46 PDT by Ariya Hidayat
Modified: 2010-08-16 05:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.90 KB, patch)
2010-08-15 15:56 PDT, Ariya Hidayat
no flags Details | Formatted Diff | Diff
Patch (10.90 KB, patch)
2010-08-15 17:18 PDT, Ariya Hidayat
no flags Details | Formatted Diff | Diff
Patch (11.39 KB, patch)
2010-08-16 00:02 PDT, Ariya Hidayat
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ariya Hidayat 2010-08-15 07:46:25 PDT
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.
Comment 1 Ariya Hidayat 2010-08-15 15:56:20 PDT
Created attachment 64459 [details]
Patch
Comment 2 WebKit Review Bot 2010-08-15 15:59:30 PDT
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.
Comment 3 Early Warning System Bot 2010-08-15 16:03:30 PDT
Attachment 64459 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3778194
Comment 4 Ariya Hidayat 2010-08-15 17:18:50 PDT
Created attachment 64461 [details]
Patch
Comment 5 Ariya Hidayat 2010-08-15 17:19:28 PDT
(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.
Comment 6 Early Warning System Bot 2010-08-15 17:25:35 PDT
Attachment 64461 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3717221
Comment 7 Ariya Hidayat 2010-08-15 17:41:21 PDT
(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,
Comment 8 Antonio Gomes 2010-08-15 20:46:34 PDT
Comment on attachment 64461 [details]
Patch

> +2010-08-15  Ariya Hidayat  <ariya@sencha.com>
> +
> +        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.
Comment 9 Ariya Hidayat 2010-08-15 23:21:54 PDT
Comment on attachment 64461 [details]
Patch

Manually committed r65393: http://trac.webkit.org/changeset/65393
Comment 10 Ariya Hidayat 2010-08-15 23:38:33 PDT
Reverted r65393 for reason:

Breaks some canvas tests

Committed r65394: <http://trac.webkit.org/changeset/65394>
Comment 11 WebKit Review Bot 2010-08-15 23:48:48 PDT
http://trac.webkit.org/changeset/65393 might have broken Qt Linux Release
Comment 12 Ariya Hidayat 2010-08-16 00:02:21 PDT
Created attachment 64468 [details]
Patch
Comment 13 Ariya Hidayat 2010-08-16 00:03:40 PDT
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());
 }
Comment 14 Antonio Gomes 2010-08-16 04:47:12 PDT
Comment on attachment 64468 [details]
Patch

r=me
Comment 15 Ariya Hidayat 2010-08-16 05:59:03 PDT
Comment on attachment 64468 [details]
Patch

Clearing flags on attachment: 64468

Committed r65420: <http://trac.webkit.org/changeset/65420>
Comment 16 Ariya Hidayat 2010-08-16 05:59:10 PDT
All reviewed patches have been landed.  Closing bug.