Summary: | Translucent text rendering on Chromium is broken | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen White <senorblanco> | ||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | brettw, senorblanco | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Windows XP | ||||||||||
Attachments: |
|
Description
Stephen White
2009-03-13 12:20:49 PDT
Created attachment 28591 [details]
Fix for TransparencyWin
FontChromiumWin was calling beginTransparencyLayer()/endTransparencyLayer(), with a TransparencyWin inside to do GDI ClearType rendering over an opaque background. TransparencyWin does its special sauce in the destructor, but it was being called too late to be used correctly in the layer. Put into uninit() instead, and call that explicitly.
Comment on attachment 28591 [details]
Fix for TransparencyWin
Why not just call it what it does? "compositeToParentLayer()" or "restoreContextAndCompositeToParentLayer()"
uninit seems like a really bad name for the function.
(In reply to comment #2) > (From update of attachment 28591 [details] [review]) > Why not just call it what it does? "compositeToParentLayer()" or > "restoreContextAndCompositeToParentLayer()" > > uninit seems like a really bad name for the function. I wanted to stress that it should pair up with init() (and that calling it is mandatory if you call init()). It's not compositing "to" anything. It's compositing over the background using ClearType, but even that is misleading, since it will get composited again over the background using text alpha in the endTransparencyLayer() call. Mind if we just call it "composite()"? (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 28591 [details] [review] [review]) > > Why not just call it what it does? "compositeToParentLayer()" or > > "restoreContextAndCompositeToParentLayer()" > > > > uninit seems like a really bad name for the function. > > I wanted to stress that it should pair up with init() (and that calling it is > mandatory if you call init()). I regretted being so disparaging in my original comment almost as soon as I hit "commit". :( > It's not compositing "to" anything. It's compositing over the background using > ClearType, but even that is misleading, since it will get composited again over > the background using text alpha in the endTransparencyLayer() call. > > Mind if we just call it "composite()"? composite() is OK. It's not particularly specific (you'd still have to read the composite() function to guess at what it did). It seems it would also make sense to ASSERT if possible that we'd already composited. We should probably be clearing m_savedOnDrawContext after restore now that this is a free function. composite() (or whatever you call it) should also be a private function, since no one should call it except for the destructor. (In reply to comment #4) > composite() is OK. It's not particularly specific (you'd still have to read > the composite() function to guess at what it did). It seems it would also make > sense to ASSERT if possible that we'd already composited. We should probably > be clearing m_savedOnDrawContext after restore now that this is a free > function. composite() (or whatever you call it) should also be a private > function, since no one should call it except for the destructor. Actually, no. This work isn't done in the destructor any more, since that was what was causing the problem (it's too late). It's called explicitly by FontChromiumWin, so it needs to be public. Created attachment 28594 [details]
Fix for TransparencyWin (2)
Changed uninit() to composite(). Reset m_savedOnDrawContext after restoring context. Asserted that it was reset in destructor.
Comment on attachment 28594 [details]
Fix for TransparencyWin (2)
Looks fine.
You can remove WARNING: NO TEST CASES ADDED OR CHANGED :)
Also, you should normally have the bug URL in teh ChangeLog.
Created attachment 28603 [details]
Fix for TransparencyWin (3)
Turns out I broke another layout test with this fix (LayoutTests/fast/forms/indeterminate.html), now fixed.
Modified ChangeLog as requested.
Comment on attachment 28603 [details]
Fix for TransparencyWin (3)
Seems that we could catch this kind of error with an !NDEBUG bool.
But this is OK.
Comment on attachment 28603 [details] Fix for TransparencyWin (3) > Index: WebCore/platform/graphics/chromium/TransparencyWin.h > =================================================================== > --- WebCore/platform/graphics/chromium/TransparencyWin.h (revision 41685) > +++ WebCore/platform/graphics/chromium/TransparencyWin.h (working copy) > @@ -134,6 +134,9 @@ public: > TransformMode transformMode, > const IntRect& region); > > + // Combines the source and destination bitmaps using the given mode. > + void composite(); You should comment on the conditions where this is function is required. Which layer modes is it required for? Also, the comments for those layer modes should say that calling composite before destruction is required. Second, now that this is required, it needs to be called in the other places that TransparencyWin is used. This is RenderThemeChromiumWin as well as webkit/tools/webcore_unit_tests/TransparencyWin_unittest.cpp in Chrome. I think that it will assert in both places otherwise. Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/platform/graphics/chromium/FontChromiumWin.cpp M WebCore/platform/graphics/chromium/TransparencyWin.cpp M WebCore/platform/graphics/chromium/TransparencyWin.h M WebCore/rendering/RenderThemeChromiumWin.cpp Committed r41692 (In reply to comment #10) > (From update of attachment 28603 [details] [review]) > You should comment on the conditions where this is function is required. Which > layer modes is it required for? Also, the comments for those layer modes should > say that calling composite before destruction is required. > > Second, now that this is required, it needs to be called in the other places > that TransparencyWin is used. This is RenderThemeChromiumWin as well as > webkit/tools/webcore_unit_tests/TransparencyWin_unittest.cpp in Chrome. I think > that it will assert in both places otherwise. Sigh. I'll re-open and you can fix this in a second patch which you can have Brett review. (In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 28603 [details] [review] [review]) > > You should comment on the conditions where this is function is required. Which > > layer modes is it required for? Also, the comments for those layer modes should > > say that calling composite before destruction is required. > > > > Second, now that this is required, it needs to be called in the other places > > that TransparencyWin is used. This is RenderThemeChromiumWin as well as > > webkit/tools/webcore_unit_tests/TransparencyWin_unittest.cpp in Chrome. I think > > that it will assert in both places otherwise. > > Sigh. I'll re-open and you can fix this in a second patch which you can have > Brett review. > Thanks. The RenderThemeChromiumWin change is already in there, and the unit tests Brett mentioned are in our webkit (not third_party), so the only thing that remains to do on the webkit.org side are comments, which I'll take care of on Monday. (In reply to comment #13) > Thanks. The RenderThemeChromiumWin change is already in there Whoops, I totally missed this! You should probably check this patch into Chromium's repo with the unit test change at the same time to avoid giving the merger a headache. Let me know if you have any questions. Comment on attachment 28594 [details]
Fix for TransparencyWin (2)
Clearing r+ on obsolete changes to move this out of the commit queue.
Comment on attachment 28603 [details]
Fix for TransparencyWin (3)
Clearing r+ on obsolete patches to move this out of the commit queue.
Code comments added in http://bugs.webkit.org/show_bug.cgi?id=26320. I'm surprised there was no layout test for this? (In reply to comment #18) > I'm surprised there was no layout test for this? > See above: LayoutTests/svg/text/text-text-08-b.svg LayoutTests/fast/css/hsla-color.html (I think acid3 was broken too). The patch attached to this bug was committed long ago, I just promised Brett I'd add some comments, which I finally got around to doing in 26320. |