23882 2009-02-10 16:44:52 -0800 GraphicsContextSkia draws round rects as solid rects 2009-02-11 14:01:24 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 1 sky sky brettw oldest_to_newest 109215 0 sky 2009-02-10 16:44:52 -0800 This can be seen with the test LayoutTests/fast/css/shadow-multiple.html . 109301 1 sky 2009-02-11 08:15:26 -0800 Once this line is fixed, the layout test LayoutTests/fast/box-shadow/basic-shadows.html starts failing. This is because we need to check if the width/height of the rounded rect is greater than the width/height of the rounded rect. If it is, then we should just draw a rect. 109309 2 27563 sky 2009-02-11 09:13:31 -0800 Created attachment 27563 Fix for 23882 109315 3 brettw 2009-02-11 09:45:46 -0800 > if the total radius along a given axis is greater than the size of > the axis to draw, a solid rect should be drawn. I assume this is the behavior of CG? It seems weird that it would do this for the entire rect if just a single corner is "too smooth." What does the Skia output look like in this case without your change? It's possible this is an aspect of CG we don't want to emulate. 109316 4 sky 2009-02-11 09:54:04 -0800 (In reply to comment #3) > > if the total radius along a given axis is greater than the size of > > the axis to draw, a solid rect should be drawn. > > I assume this is the behavior of CG? Yes. This behavior can be seen with the layout test LayoutTests/fast/box-shadow/border-radius-big.html . > It seems weird that it would do this for > the entire rect if just a single corner is "too smooth." What does the Skia > output look like in this case without your change? Experimentation shows it does this for the whole rect, not just the corner(s). > It's possible this is an aspect of CG we don't want to emulate. I don't have a strong feeling here. This is an edge case that I can't imagine comes up much. But I do tend to think Skia should match CG as close as possible. 109322 5 27563 eric 2009-02-11 10:37:16 -0800 Comment on attachment 27563 Fix for 23882 This block still doesn't make sense to me, even after reading the bug: + if (topLeft.width() + topRight.width() > rect.width() || + bottomLeft.width() + bottomRight.width() > rect.width() || + topLeft.height() + bottomLeft.height() > rect.height() || + topRight.height() + bottomRight.height() > rect.height()) { + // If the width/height along an axis is > than the width/height of the + // rectangle, just draw a rect. + fillRect(rect, color); + return; + } i.e. What behavior does that change? Why are we changing it (to match CG's behavior it seems). What is "width/height long an axis" Seems we need a better comment there to make more sense to the casual reader (who knows less about our graphics code than even I do). Otherwise this change looks great. 109330 6 sky 2009-02-11 11:06:09 -0800 (In reply to comment #5) > (From update of attachment 27563 [review]) > This block still doesn't make sense to me, even after reading the bug: > > + if (topLeft.width() + topRight.width() > rect.width() || > + bottomLeft.width() + bottomRight.width() > rect.width() || > + topLeft.height() + bottomLeft.height() > rect.height() || > + topRight.height() + bottomRight.height() > rect.height()) { > + // If the width/height along an axis is > than the width/height of the > + // rectangle, just draw a rect. > + fillRect(rect, color); > + return; > + } > > i.e. What behavior does that change? This effectively gives us the old behavior (of always filling a rect) if the sum of the radii along an axis are bigger than the size of the rect. This method (fillRoundedRect) is invoked to draw the shadow. The method that draws the background uses Path::createRoundedRectangle to create the path for the background. Path::createRoundedRectangle creates a rectangle under the same conditions: if (width < topLeftRadius.width() + topRightRadius.width() || width < bottomLeftRadius.width() + bottomRightRadius.width() || height < topLeftRadius.height() + bottomLeftRadius.height() || height < topRightRadius.height() + bottomRightRadius.height()) // If all the radii cannot be accommodated, return a rect. return createRectangle(rectangle); If fillRoundedRect isn't updated like this it means we draw a round shadow for a solid background. > Why are we changing it (to match CG's > behavior it seems). What is "width/height long an axis" > Seems we need a better comment there to make more sense to the casual reader > (who knows less about our graphics code than even I do). My comment is wrong. I'll update it (along with a style change) and attach a new patch. Thanks! 109333 7 27566 sky 2009-02-11 11:13:19 -0800 Created attachment 27566 Fix for 23882 Improves comment and changes style. 109365 8 27566 eric 2009-02-11 13:41:08 -0800 Comment on attachment 27566 Fix for 23882 Looks good. will land. 109367 9 eric 2009-02-11 14:01:24 -0800 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/platform/graphics/skia/GraphicsContextSkia.cpp Committed r40869 27563 2009-02-11 09:13:31 -0800 2009-02-11 11:13:19 -0800 Fix for 23882 23882.patch text/plain 2325 sky SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MDgzOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMDktMDItMTAgIFNjb3R0IFZpb2xldCAgPHNreUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzg4MgorICAgICAgICBHcmFwaGlj c0NvbnRleHRTa2lhIGRyYXdzIHJvdW5kIHJlY3RzIGFzIHNvbGlkIHJlY3RzCisKKyAgICAgICAg Rml4ZXMgdHdvIGJ1Z3MgaW4gU2tpYSdzIEdyYXBoaWNzQ29udGV4dDo6ZmlsbFJvdW5kZWRSZWN0 OgorICAgICAgICAuIGZpbGxSb3VuZGVkUmVjdCBoYWQgYW4gZXh0cmEgY2FsbCB0byBmaWxsUmVj dCwgcmVzdWx0aW5nIGluIGFsd2F5cworICAgICAgICAgIGRyYXdpbmcgYSBzb2xpZCByZWN0YW5n bGUuCisgICAgICAgIC4gaWYgdGhlIHRvdGFsIHJhZGl1cyBhbG9uZyBhIGdpdmVuIGF4aXMgaXMg Z3JlYXRlciB0aGFuIHRoZSBzaXplIG9mCisgICAgICAgICAgdGhlIGF4aXMgdG8gZHJhdywgYSBz b2xpZCByZWN0IHNob3VsZCBiZSBkcmF3bi4KKworICAgICAgICBUaGUgbGF5b3V0IHRlc3RzIG91 dFRlc3RzL2Zhc3QvY3NzL3NoYWRvdy1tdWx0aXBsZS5odG1sIGFuZAorICAgICAgICBMYXlvdXRU ZXN0cy9mYXN0L2JveC1zaGFkb3cvYmFzaWMtc2hhZG93cy5odG1sIGNvdmVyIHRoaXMuCisKKyAg ICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL0dyYXBoaWNzQ29udGV4dFNraWEuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6R3JhcGhpY3NDb250ZXh0OjpmaWxsUm91bmRlZFJlY3QpOgorCiAy MDA5LTAyLTEwICBEYXJpbiBGaXNoZXIgIDxkYXJpbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAg UmV2aWV3ZWQgYnkgRXJpYyBTZWlkZWwuCkluZGV4OiBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L3NraWEvR3JhcGhpY3NDb250ZXh0U2tpYS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9za2lhL0dyYXBoaWNzQ29udGV4dFNraWEuY3BwCShyZXZpc2lvbiA0MDgy NikKKysrIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9HcmFwaGljc0NvbnRleHRTa2lh LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzc3LDYgKzc3NywxNiBAQCB2b2lkIEdyYXBoaWNzQ29u dGV4dDo6ZmlsbFJvdW5kZWRSZWN0KGNvCiAgICAgICAgIC8vIFNlZSBmaWxsUmVjdCgpLgogICAg ICAgICBDbGlwUmVjdFRvQ2FudmFzKCpwbGF0Zm9ybUNvbnRleHQoKS0+Y2FudmFzKCksIHIsICZy KTsKIAorICAgIGlmICh0b3BMZWZ0LndpZHRoKCkgKyB0b3BSaWdodC53aWR0aCgpID4gcmVjdC53 aWR0aCgpIHx8CisgICAgICAgIGJvdHRvbUxlZnQud2lkdGgoKSArIGJvdHRvbVJpZ2h0LndpZHRo KCkgPiByZWN0LndpZHRoKCkgfHwKKyAgICAgICAgdG9wTGVmdC5oZWlnaHQoKSArIGJvdHRvbUxl ZnQuaGVpZ2h0KCkgPiByZWN0LmhlaWdodCgpIHx8CisgICAgICAgIHRvcFJpZ2h0LmhlaWdodCgp ICsgYm90dG9tUmlnaHQuaGVpZ2h0KCkgPiByZWN0LmhlaWdodCgpKSB7CisgICAgICAgIC8vIElm IHRoZSB3aWR0aC9oZWlnaHQgYWxvbmcgYW4gYXhpcyBpcyA+IHRoYW4gdGhlIHdpZHRoL2hlaWdo dCBvZiB0aGUKKyAgICAgICAgLy8gcmVjdGFuZ2xlLCBqdXN0IGRyYXcgYSByZWN0LgorICAgICAg ICBmaWxsUmVjdChyZWN0LCBjb2xvcik7CisgICAgICAgIHJldHVybjsKKyAgICB9CisKICAgICBT a1BhdGggcGF0aDsKICAgICBhZGRDb3JuZXJBcmMoJnBhdGgsIHIsIHRvcFJpZ2h0LCAyNzApOwog ICAgIGFkZENvcm5lckFyYygmcGF0aCwgciwgYm90dG9tUmlnaHQsIDApOwpAQCAtNzg2LDcgKzc5 Niw2IEBAIHZvaWQgR3JhcGhpY3NDb250ZXh0OjpmaWxsUm91bmRlZFJlY3QoY28KICAgICBTa1Bh aW50IHBhaW50OwogICAgIHBsYXRmb3JtQ29udGV4dCgpLT5zZXR1cFBhaW50Rm9yRmlsbGluZygm cGFpbnQpOwogICAgIHBsYXRmb3JtQ29udGV4dCgpLT5jYW52YXMoKS0+ZHJhd1BhdGgocGF0aCwg cGFpbnQpOwotICAgIHJldHVybiBmaWxsUmVjdChyZWN0LCBjb2xvcik7CiB9CiAKIFRyYW5zZm9y bWF0aW9uTWF0cml4IEdyYXBoaWNzQ29udGV4dDo6Z2V0Q1RNKCkgY29uc3QK 27566 2009-02-11 11:13:19 -0800 2009-02-11 13:41:08 -0800 Fix for 23882 23882.patch text/plain 2414 sky SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MDgzOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMDktMDItMTAgIFNjb3R0IFZpb2xldCAgPHNreUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzg4MgorICAgICAgICBHcmFwaGlj c0NvbnRleHRTa2lhIGRyYXdzIHJvdW5kIHJlY3RzIGFzIHNvbGlkIHJlY3RzCisKKyAgICAgICAg Rml4ZXMgdHdvIGJ1Z3MgaW4gU2tpYSdzIEdyYXBoaWNzQ29udGV4dDo6ZmlsbFJvdW5kZWRSZWN0 OgorICAgICAgICAuIGZpbGxSb3VuZGVkUmVjdCBoYWQgYW4gZXh0cmEgY2FsbCB0byBmaWxsUmVj dCwgcmVzdWx0aW5nIGluIGFsd2F5cworICAgICAgICAgIGRyYXdpbmcgYSBzb2xpZCByZWN0YW5n bGUuCisgICAgICAgIC4gaWYgdGhlIHRvdGFsIHJhZGl1cyBhbG9uZyBhIGdpdmVuIGF4aXMgaXMg Z3JlYXRlciB0aGFuIHRoZSBzaXplIG9mCisgICAgICAgICAgdGhlIGF4aXMgdG8gZHJhdywgYSBz b2xpZCByZWN0IHNob3VsZCBiZSBkcmF3bi4KKworICAgICAgICBUaGUgbGF5b3V0IHRlc3RzIG91 dFRlc3RzL2Zhc3QvY3NzL3NoYWRvdy1tdWx0aXBsZS5odG1sIGFuZAorICAgICAgICBMYXlvdXRU ZXN0cy9mYXN0L2JveC1zaGFkb3cvYmFzaWMtc2hhZG93cy5odG1sIGNvdmVyIHRoaXMuCisKKyAg ICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL0dyYXBoaWNzQ29udGV4dFNraWEuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6R3JhcGhpY3NDb250ZXh0OjpmaWxsUm91bmRlZFJlY3QpOgorCiAy MDA5LTAyLTEwICBEYXJpbiBGaXNoZXIgIDxkYXJpbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAg UmV2aWV3ZWQgYnkgRXJpYyBTZWlkZWwuCkluZGV4OiBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L3NraWEvR3JhcGhpY3NDb250ZXh0U2tpYS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9za2lhL0dyYXBoaWNzQ29udGV4dFNraWEuY3BwCShyZXZpc2lvbiA0MDgy NikKKysrIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9HcmFwaGljc0NvbnRleHRTa2lh LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzc3LDYgKzc3NywxNyBAQCB2b2lkIEdyYXBoaWNzQ29u dGV4dDo6ZmlsbFJvdW5kZWRSZWN0KGNvCiAgICAgICAgIC8vIFNlZSBmaWxsUmVjdCgpLgogICAg ICAgICBDbGlwUmVjdFRvQ2FudmFzKCpwbGF0Zm9ybUNvbnRleHQoKS0+Y2FudmFzKCksIHIsICZy KTsKIAorICAgIGlmICh0b3BMZWZ0LndpZHRoKCkgKyB0b3BSaWdodC53aWR0aCgpID4gcmVjdC53 aWR0aCgpCisgICAgICAgICAgICB8fCBib3R0b21MZWZ0LndpZHRoKCkgKyBib3R0b21SaWdodC53 aWR0aCgpID4gcmVjdC53aWR0aCgpCisgICAgICAgICAgICB8fCB0b3BMZWZ0LmhlaWdodCgpICsg Ym90dG9tTGVmdC5oZWlnaHQoKSA+IHJlY3QuaGVpZ2h0KCkKKyAgICAgICAgICAgIHx8IHRvcFJp Z2h0LmhlaWdodCgpICsgYm90dG9tUmlnaHQuaGVpZ2h0KCkgPiByZWN0LmhlaWdodCgpKSB7Cisg ICAgICAgIC8vIE5vdCBhbGwgdGhlIHJhZGlpIGZpdCwgcmV0dXJuIGEgcmVjdC4gVGhpcyBtYXRj aGVzIHRoZSBiZWhhdmlvciBvZgorICAgICAgICAvLyBQYXRoOjpjcmVhdGVSb3VuZGVkUmVjdGFu Z2xlLiBXaXRob3V0IHRoaXMgd2UgYXR0ZW1wdCB0byBkcmF3IGEgcm91bmQKKyAgICAgICAgLy8g c2hhZG93IGZvciBhIHNxdWFyZSBib3guCisgICAgICAgIGZpbGxSZWN0KHJlY3QsIGNvbG9yKTsK KyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIFNrUGF0aCBwYXRoOwogICAgIGFkZENvcm5l ckFyYygmcGF0aCwgciwgdG9wUmlnaHQsIDI3MCk7CiAgICAgYWRkQ29ybmVyQXJjKCZwYXRoLCBy LCBib3R0b21SaWdodCwgMCk7CkBAIC03ODYsNyArNzk3LDYgQEAgdm9pZCBHcmFwaGljc0NvbnRl eHQ6OmZpbGxSb3VuZGVkUmVjdChjbwogICAgIFNrUGFpbnQgcGFpbnQ7CiAgICAgcGxhdGZvcm1D b250ZXh0KCktPnNldHVwUGFpbnRGb3JGaWxsaW5nKCZwYWludCk7CiAgICAgcGxhdGZvcm1Db250 ZXh0KCktPmNhbnZhcygpLT5kcmF3UGF0aChwYXRoLCBwYWludCk7Ci0gICAgcmV0dXJuIGZpbGxS ZWN0KHJlY3QsIGNvbG9yKTsKIH0KIAogVHJhbnNmb3JtYXRpb25NYXRyaXggR3JhcGhpY3NDb250 ZXh0OjpnZXRDVE0oKSBjb25zdAo=