Summary: | Text rendered with a simple (i.e. 0px blur) shadow inside a transparency layer has a double shadow | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||
Component: | Layout and Rendering | Assignee: | Tim Horton <thorton> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, dglazkov, mrobinson, rniwa, webkit-bug-importer, webkit.review.bot, zimmermann | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 67746 | ||||||||||||
Bug Blocks: | 78332 | ||||||||||||
Attachments: |
|
Description
Tim Horton
2011-09-02 17:31:14 PDT
Created attachment 106229 [details]
testcase
Created attachment 106233 [details]
initial patch
Comment on attachment 106233 [details] initial patch Attachment 106233 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9583831 New failing tests: svg/custom/simple-text-double-shadow.svg Windows build failure: 3>..\platform\win\ScrollbarThemeWin.cpp(274) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>..\platform\win\ScrollbarThemeWin.cpp(328) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>..\platform\win\ScrollbarThemeWin.cpp(387) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>c:\cygwin\home\buildbot\webkit\source\webcore\rendering\RenderThemeWin.cpp(677) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>..\plugins\win\PluginViewWin.cpp(639) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' Created attachment 106477 [details]
fix windows build
Comment on attachment 106477 [details] fix windows build Attachment 106477 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9601403 New failing tests: svg/custom/simple-text-double-shadow.svg Landed in 94714; now for grabbing new baselines and checking for build failures! It seems like this patch caused a whole bunch of tests to crash on GTK: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html (In reply to comment #9) > It seems like this patch caused a whole bunch of tests to crash on GTK: > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html On second thought, this might be an issue with the bot. Other GTK+ bots are fine even after your changeset. (In reply to comment #10) > (In reply to comment #9) > > It seems like this patch caused a whole bunch of tests to crash on GTK: > > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html > > On second thought, this might be an issue with the bot. Other GTK+ bots are fine even after your changeset. Nope. The same thing started to happen on Windows XP (Debug): http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r94714%20(31934)/results.html Reopening due to rollout. Quick debugging on gtk-linux shows that m_transparencyCount rolls over to 32768. Not sure why; it's only manipulated in one place which has an assert (which isn't being hit) to guard against this (also, stepping through and printing m_transparencyCount shows that it's not a problem there, it becomes garbage in between). I'll debug more carefully tonight, since it's less clear what's going on now... Also, I found an extra m_transparencyCount(0) on GraphicsContextPlatformPrivateCairo which would break gtk-windows build, so I'll remove that too. (that's certainly not causing this problem, though). Created attachment 106901 [details]
Fix crashes on Windows and Gtk
Turns out WindowsCG, WindowsCairo and Cairo overload the GraphicsContext constructor, so I just needed to add my initialization there (showed up instantly in valgrind).
Comment on attachment 106901 [details]
Fix crashes on Windows and Gtk
review+ on the fix; Simon is still the reviewer on the change itself
Comment on attachment 106901 [details] Fix crashes on Windows and Gtk Clearing flags on attachment: 106901 Committed r94897: <http://trac.webkit.org/changeset/94897> All reviewed patches have been landed. Closing bug. This patch caused Qt bug 78332, uncovered by my repaint.js refactoring. See my comment there: "Hmpf, the problem is the m_transparencyCount generalization. Qt maintains a different count "layerCount" returned by isInTransparentLayer(), and it calls endTransparencyLayer too often, compared to all other ports - to support image clipping. Cairo has similar needs (mask image operation during restore()) but doesn't suffer from the problem - can this be reused instead? I hope Zoltan can have a look." The Qt code uses for (i=0; i < layerCount) endTransparencyLayer(). Layer count may be higher than m_transparencyCount, leading to the assertion that m_transparenyCount should always be > 0. Why is that? Qt doesn't always perform --m_layerCount, as endTransparencyLayer() does for m_transparencyCount. The whole code is only needed to properly support ImageBuffer::clip() - Zoltan Herczeg is currently working on eliminating that, replacing by composite operations. Once that's done the custom layer count stuff could be removed from Qt again, fixing the assertion. That's probably the easiest - the current code from Tim makes a lot of sense! (Note: Cairo/Gtk don't suffer from this problem, they don't maintain a separated 'layer count', but rely on m_transparencyCount) Comment on attachment 106477 [details] fix windows build Cleared Simon Fraser's review+ from obsolete attachment 106477 [details] so that this bug does not appear in http://webkit.org/pending-commit. Niko, is there a reason you reopened this? The fix is still in, and bug 78332 is mysteriously fixed. I'm going to re-close it, feel free to let me know/reopen again if there was a reason. |