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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Takashi Sakamoto <tasak at google.com>  2012-06-15 04:32:43 PST ---
Thank you for reviewing.

(In reply to comment #6)
> (From update of attachment 144021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144021&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        care of any radii's scale. So it probably causes to shrink the radii too much.
> 
> Some of the images have some red pixels, is this the cause for that? Shouldn't you solve this issue too instead of just mentioning it here?

Talking about the test: fast/borders/border-radius-image.html, the left box shows how img works with no border-radius. So it should have red pixels.
This border-radius-image is for testing three cases which are not checked by ietestcenter's test:
(1) check this patch doesn't affect img rendering if no border-radius.
(2) check this patch correctly treats img with rounded border. The border's left width, right width, top width and bottom width are not the same.
(3) check this patch correctly treats img with no padding and with rounded border.

Firstly I think, I need only border-radius-content-edge-001.htm. However, I found that I created new bugs while creating this patch. So I think, I need these three test cases.

Talking about ietestcenter/css3/borderbackgrounds/border-radius-content-edge-001.htm, there are still small red pixels left on rounded corners. However, I compared the result with Firefox's one and found that Firefox also shows small red pixels.

> > Source/WebCore/rendering/RenderImage.cpp:439
> > +        LayoutUnit leftWidth = style()->borderLeftWidth();
> > +        LayoutUnit rightWidth = style()->borderRightWidth();
> > +        LayoutUnit topWidth = style()->borderTopWidth();
> > +        LayoutUnit bottomWidth = style()->borderBottomWidth();
> 
> Borders are unsigned, please avoid those unneeded conversions to LayoutUnit. Also let's avoid abbreviation, those are border width, not just width (which can be anything)
> 
> > Source/WebCore/rendering/RenderImage.cpp:441
> > +        LayoutRect rectWithBorder(rect.x() - leftWidth, rect.y() - topWidth,
> > +                                  rect.width() + leftWidth + rightWidth, rect.height() + topWidth + bottomWidth);
> 
> It would be neat to add a test for vertical writing mode (unless already covered). This should work as painting assumes rectangles in device coordinates and not logical ones.
> 
> > Source/WebCore/rendering/RenderImage.cpp:444
> > +            rectWithBorder.move(-paddingLeft(), -paddingTop());
> > +            rectWithBorder.expand(paddingLeft() + paddingRight(), paddingTop() + paddingBottom());
> 
> rectWithBorder.inflate(paddingLeft(), paddingTop());
> rectWithBorder.expand(paddingRight(), paddingBottom());

I think, inflate does't work as I expected. I mean,

inflateX(dx) increases size by dx * 2:
  dx <----|---- original size ---| ----> dx

What I want to do is
  paddingLeft <----|---- original size ----|-----> paddingRight


> > Source/WebCore/rendering/RenderImage.cpp:450
> > +            clipRect.setRect(alignedRect);
> 
> As a whole, we do a lot of recurrent computation here (shrink / expanding to account for paddings or margin). I don't know of any smarter way of doing it but maybe someone else may. At least exposing a way of expanding a rect using a LayoutBoxExtend and adding the ad-hoc getter in RenderStyle should reduce the risk of error.

I see. I modified to use LayoutBoxExtent.

Best regards,
Takashi Sakamoto

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