[Webkit-unassigned] [Bug 83206] webkit fails IETC border-radius-content-edge-001

Wed Aug 1 17:38:39 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=83206

Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #155778|review?                     |review-
               Flag|                            |

--- Comment #13 from Julien Chaffraix <jchaffraix at webkit.org>  2012-08-01 17:38:38 PST ---
(From update of attachment 155778)
View in context: https://bugs.webkit.org/attachment.cgi?id=155778&action=review

r- mostly for tripping between logical and physical coordinate. Could you explain why you can't extend the existing helper functions (getRounderInnerBorderFor and al.) for your need?

> Source/WebCore/ChangeLog:10
> +        clipping if an image has border-radius. I compared the below test's
> +        result with a result obtained by using Firefox.

And what was the result of the comparison?

> Source/WebCore/ChangeLog:20
> +        doesn't include any padding), the radii's scale of the calculrated

typo: calculated.

> Source/WebCore/ChangeLog:24
> +        Talking about the above test, the scale is 0.96 < 1.0. However
> +        radii.shrink() doesn't take care of any radii's scale. So it probably
> +        causes to shrink the radii too much.

This probably should go in the LayoutTest ChangeLog as it describes why the image is failing. Please file a bug about that before landing and mention that it is covered by this bug in the ChangeLog.

> Source/WebCore/rendering/RenderImage.cpp:450
> +    GraphicsContextStateSaver clipToBorderStateSaver(*context, hasRoundedBorder);

This should be moved inside the if below and you can remove the |hasRoundedBorder| check.

> Source/WebCore/rendering/RenderImage.cpp:454
> +        rectWithBorder.move(-borderLeft(), -borderTop());
> +        rectWithBorder.expand(borderLeft() + borderRight(), borderTop() + borderBottom());

I feel like you are fighting the API here. getRoundedInnerBorderFor will re-add the borders back in the case where no padding was set on your image. My bet is that you can just pass the args as |false| but nothing prevents you from actually making a variant that works in your case, instead of implementing everything inline here.

> Source/WebCore/rendering/RenderImage.cpp:461
> +            radii.shrink(paddingBorderBox.before(style()), paddingBorderBox.after(style()), paddingBorderBox.start(style()), paddingBorderBox.end(style()));

This line is wrong. You need to use the physical coordinates (top / left / ...) not the logical one (before / after / ...). You would see that if you had a test case involving some vertical writing-mode, which I already advised you to add but you missed the cue.

The easiest way is to add a new class to your test similar to #border_radius_padding but which has: -webkit-writing-mode: vertical-rl;

> LayoutTests/ChangeLog:16
> +        * platform/chromium-win/ietestcenter/css3/bordersbackgrounds/border-radius-content-edge-001-expected.png: Removed.

Note that Qt also has a pixel baseline for this test which we will need to update. If possible, it's better before if not you will have to coordinate with some people to avoid breaking Qt.

> LayoutTests/fast/borders/border-radius-image.html:27
> +<body>

A test should explain what you are trying to achieve. Here it's extremely difficult to do so.

Some questions that should be answered:
* What is expected here?
* What are you even testing? 

As you are using a pixel test, you don't want that to be dumped as text in the image. There are several tricks on http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Pixeltesttips on how to do that.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.