Bug 113506

Summary: [Qt] Create ShadowBlur on demand.
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: Layout and RenderingAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: bruno.abinader, buildbot, d-r, noam, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch noam: review+

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>