[webkit-reviews] review denied: [Bug 83206] webkit fails IETC border-radius-content-edge-001 : [Attachment 155778] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 1 17:38:37 PDT 2012
Julien Chaffraix <jchaffraix at webkit.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 83206: webkit fails IETC border-radius-content-edge-001
https://bugs.webkit.org/show_bug.cgi?id=83206
Attachment 155778: Patch
https://bugs.webkit.org/attachment.cgi?id=155778&action=review
------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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-conten
t-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#Pix
eltesttips on how to do that.
More information about the webkit-reviews
mailing list