Bug 113506 - [Qt] Create ShadowBlur on demand.
Summary: [Qt] Create ShadowBlur on demand.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-28 08:12 PDT by Allan Sandfeld Jensen
Modified: 2013-04-05 08:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (29.77 KB, patch)
2013-03-28 08:17 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (29.75 KB, patch)
2013-04-04 08:42 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (31.15 KB, patch)
2013-04-04 09:09 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (31.15 KB, patch)
2013-04-04 10:03 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (31.16 KB, patch)
2013-04-05 01:13 PDT, Allan Sandfeld Jensen
noam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-03-28 08:12:11 PDT
GraphicsContext for Qt keeps a single ShadowBlur object around and up to date. This leads to problems where an active ShadowBlur object can accidentially change its own values because it is disabling shadows on the parent graphics context (seen in the last patch in bug 90082 and in the patch for 111736).

We should instead like the other ports do, create the ShadowBlur object on demand when needed. This also means that the active ShadowBlur is its own object and doesn't get affected by changing shadow values on the GraphicsContext.
Comment 1 Allan Sandfeld Jensen 2013-03-28 08:17:00 PDT
Created attachment 195570 [details]
Patch
Comment 2 Noam Rosenthal 2013-04-04 07:39:06 PDT
Comment on attachment 195570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195570&action=review

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:626
> +            ShadowBlur shadow(FloatSize(m_state.shadowBlur, m_state.shadowBlur), m_state.shadowOffset, m_state.shadowColor, m_state.shadowColorSpace, m_state.shadowsIgnoreTransforms);

This kind of repeats itself... don't you want to do something like:
OwnPtr<ShadowBlur> shadow = m_state.createShadowBlur();
Comment 3 Allan Sandfeld Jensen 2013-04-04 08:16:42 PDT
(In reply to comment #2)
> (From update of attachment 195570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195570&action=review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:626
> > +            ShadowBlur shadow(FloatSize(m_state.shadowBlur, m_state.shadowBlur), m_state.shadowOffset, m_state.shadowColor, m_state.shadowColorSpace, m_state.shadowsIgnoreTransforms);
> 
> This kind of repeats itself... don't you want to do something like:
> OwnPtr<ShadowBlur> shadow = m_state.createShadowBlur();

I don't like to allocate pointers for something that belongs on the stack, but I could perhaps make the ShadowBlur constructor just take the GraphicsContextState as an argument, making it much cleaner.
Comment 4 Allan Sandfeld Jensen 2013-04-04 08:42:36 PDT
Created attachment 196480 [details]
Patch

Now with less redundancy.
Comment 5 WebKit Review Bot 2013-04-04 08:53:03 PDT
Attachment 196480 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/ShadowBlur.cpp', u'Source/WebCore/platform/graphics/ShadowBlur.h', u'Source/WebCore/platform/graphics/cairo/FontCairo.cpp', u'Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp', u'Source/WebCore/platform/graphics/qt/FontQt.cpp', u'Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp', u'Source/WebCore/platform/graphics/qt/ImageQt.cpp', u'Source/WebCore/platform/graphics/qt/StillImageQt.cpp']" exit_code: 1
Source/WebCore/platform/graphics/ShadowBlur.cpp:198:  Extra space before ) in if  [whitespace/parens] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Build Bot 2013-04-04 09:03:04 PDT
Comment on attachment 196480 [details]
Patch

Attachment 196480 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17464065
Comment 7 Allan Sandfeld Jensen 2013-04-04 09:09:50 PDT
Created attachment 196484 [details]
Patch
Comment 8 Build Bot 2013-04-04 09:46:05 PDT
Comment on attachment 196484 [details]
Patch

Attachment 196484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17497088
Comment 9 Allan Sandfeld Jensen 2013-04-04 10:03:08 PDT
Created attachment 196489 [details]
Patch
Comment 10 Build Bot 2013-04-04 11:16:57 PDT
Comment on attachment 196489 [details]
Patch

Attachment 196489 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17421044
Comment 11 Build Bot 2013-04-04 11:19:05 PDT
Comment on attachment 196489 [details]
Patch

Attachment 196489 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17435027
Comment 12 Allan Sandfeld Jensen 2013-04-04 11:33:46 PDT
(In reply to comment #11)
> (From update of attachment 196489 [details])
> Attachment 196489 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17435027

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/ShadowBlur.cpp:200:15: error: unused variable 'shadowBlur' [-Werror,-Wunused-variable]
        float shadowBlur = radiusToLegacyRadius(state.shadowBlur);
              ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/ShadowBlur.cpp:201:34: error: use of undeclared identifier 'shadowBlur'
        m_blurRadius = FloatSize(shadowBlur, shadowBlur);

Yeah, that makes complete sense, clang.
Comment 13 Noam Rosenthal 2013-04-04 22:15:17 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 196489 [details] [details])
> > Attachment 196489 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17435027
> 
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/ShadowBlur.cpp:200:15: error: unused variable 'shadowBlur' [-Werror,-Wunused-variable]
>         float shadowBlur = radiusToLegacyRadius(state.shadowBlur);
>               ^
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/ShadowBlur.cpp:201:34: error: use of undeclared identifier 'shadowBlur'
>         m_blurRadius = FloatSize(shadowBlur, shadowBlur);
> 
> Yeah, that makes complete sense, clang.

It does, you forgot {}, the first line is inside the if statement and the second one isnt...
Comment 14 Build Bot 2013-04-04 22:18:01 PDT
Comment on attachment 196489 [details]
Patch

Attachment 196489 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17518335
Comment 15 Allan Sandfeld Jensen 2013-04-05 01:13:16 PDT
Created attachment 196592 [details]
Patch
Comment 16 Allan Sandfeld Jensen 2013-04-05 08:50:39 PDT
Committed r147750: <http://trac.webkit.org/changeset/147750>