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.
Created attachment 195570 [details] Patch
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();
(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.
Created attachment 196480 [details] Patch Now with less redundancy.
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 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
Created attachment 196484 [details] Patch
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
Created attachment 196489 [details] Patch
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 on attachment 196489 [details] Patch Attachment 196489 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17435027
(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.
(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 on attachment 196489 [details] Patch Attachment 196489 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17518335
Created attachment 196592 [details] Patch
Committed r147750: <http://trac.webkit.org/changeset/147750>