Bug 113506 – [Qt] Create ShadowBlur on demand.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 195570 [details]
Patch

[Noam Rosenthal]

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();

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Created attachment 196480 [details]
Patch

Now with less redundancy.

[WebKit Review Bot]

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.

[Build Bot]

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

[Allan Sandfeld Jensen]

Created attachment 196484 [details]
Patch

[Build Bot]

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

[Allan Sandfeld Jensen]

Created attachment 196489 [details]
Patch

[Build Bot]

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

[Build Bot]

Comment on attachment 196489 [details]
Patch

Attachment 196489 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17435027

[Allan Sandfeld Jensen]

(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.

[Noam Rosenthal]

(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...

[Build Bot]

Comment on attachment 196489 [details]
Patch

Attachment 196489 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17518335

[Allan Sandfeld Jensen]

Created attachment 196592 [details]
Patch

[Allan Sandfeld Jensen]

Committed r147750: <http://trac.webkit.org/changeset/147750>