New renderer for SVGRectElement

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

New renderer for SVGRectElement

[Renata Hodovan]

Created attachment 103073 [details]
Proposed patch

This patch is just a draft so I didn't update the layout tests (but almost all SVG related tests need it, because DRT will dump RenderSVGRect instead of RenderSVGPath if the SVG graphics contains rect object).

[Gyuyoung Kim]

Comment on attachment 103073 [details]
Proposed patch

Attachment 103073 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9305852

[WebKit Review Bot]

Comment on attachment 103073 [details]
Proposed patch

Attachment 103073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9321104

[Gustavo Noronha (kov)]

Comment on attachment 103073 [details]
Proposed patch

Attachment 103073 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9301984

[WebKit Review Bot]

Comment on attachment 103073 [details]
Proposed patch

Attachment 103073 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9301986

[Dirk Schulze]

Comment on attachment 103073 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103073&action=review

We need performance tests for this. If we have stupid graphic libs that recreate a path internally for every fillRect or strokeRect operation, this would be significant slower! That's why I'd begin with circles/ellipses, there is a very good Test to check the performance for circles, but it can indeed be adapted to rects. http://themaninblue.com/experiment/AnimationBenchmark/svg

But again, we need to test this on all platforms! I'll be willing to run the tests on mac as well, if that helps. I can try CG but maybe Skia as well.

The following comment are not a complete review. I have more comments later, we just need to clarify some fact first. 

A lot of code looks like code duplication, can't you just call shapeSpecific functions in RenderSVGBasicShape? Do we need all these paint, layout, fillAndStroke, nodeAtPoint functions again?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:189
> +    if (RenderSVGResource* fillPaintingResource = RenderSVGResource::fillPaintingResource(this, style, fallbackColor)) {

Is it really a fallback _color_ ? Can't a gradient or pattern be a fallback as well? I have to look for that myself first. Seems not to be your problem anyway here.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:196
> +                Path path;
> +                path.addRoundedRect(m_rect, FloatSize(m_rxValue, m_ryValue));
> +                context->fillPath(path);

That is a bad idea. On every Paint operation we have to recreate the path to draw it. This is significant slower than the current behavior, where we store the path. We should fallback to BasicShape in this situation. Or you could store a RefPtr<Path> like we do in BasicShape as well.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:207
> +                    Path path;
> +                    path.addRoundedRect(m_rect, FloatSize(m_rxValue, m_ryValue));
> +                    context->fillPath(path);

ditto.

[Nikolas Zimmermann]

Comment on attachment 103073 [details]
Proposed patch

I only had a quick glance over this patch, though I'm imaging the amount of new code that would be needed if we had RenderSVGRect/Circle/Ellipse/... A bit too much overhead for my taste.

My initial thought would be to have sth like RenderSVGShape, that takes an enum ShapeMode { ShapeRect, ShapeCircle, ShapeEllipse, ...}
to have one single class for all shapes. RenderSVGShape could even inherit from RenderSVGPath (fallback for eg. rectangles with rounded corners, etc.).

What do you think?

[Dirk Schulze]

Comment on attachment 103073 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103073&action=review

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:59
> +    if (determinePointLocation(point, m_rect) != OutsideShape)
> +        return true;
> +    return false;

This is not correct for rects with rounded corners.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:72
> +    const SVGRenderStyle* svgStyle = style()->svgStyle();
> +    float strokeWidth = svgStyle->strokeWidth().value(static_cast<SVGRectElement*>(node()));
> +    FloatRect innerStrokeRect = m_rect;
> +    FloatRect outerStrokeRect = m_rect;
> +    innerStrokeRect.inflate(-strokeWidth / 2);
> +    outerStrokeRect.inflate(strokeWidth / 2);
> +    if (determinePointLocation(point, innerStrokeRect) != InsideShape && determinePointLocation(point, outerStrokeRect) != OutsideShape)
> +        return true;
> +    return false;

This will cause regressions as well. First it is not correct for rects with rounded corners, second: it will fire events for gaps in dash arrays as well.

[Renata Hodovan]

Created attachment 103487 [details]
Proposed patch

After discussing with Niko on IRC we declared the introduction of the new RenderSVGRect in three steps:
1) Adding a pure RenderSVGRect class inherited from RenderSVGPath. It only contains two functions (isSVGRect() and renderName()) what it can identify itself with. This requires the update a mass of expected text files.
2) The most part of RenderSVGPath will be moved to RenderSVGShape and it will be inherited from RenderSVGShape.
3) Decouple RenderSVGRect to use RenderSVGShape as base instead of RenderSVGPath

This patch perform the first step.
I couldn't upload the whole expected update, because of the size of diff. It was ~13Mb, but bugzilla can receive patches maximum with 10Mb (even if i check the Big patch option)

[Dirk Schulze]

Your patch is too big for pretty diff.  The only change on DRT seems to be the change from RenderSVGPath to RenderSVGRect, can you post the tests in another patch please?

[Dirk Schulze]

Comment on attachment 103487 [details]
Proposed patch

I'd like to know how you want to deal with rounded rects before you add a new renderer named RenderSVGRect. See comment #8 rounded rects should be RenderSVGPath (or Shape), otherwise we would get performance issues and regressions on pointer events.

Btw. where is RenderSVGRect.cpp in your patch?

[Renata Hodovan]

Created attachment 103498 [details]
Proposed patch

First part

[Renata Hodovan]

(In reply to comment #11)
> (From update of attachment 103487 [details])
> I'd like to know how you want to deal with rounded rects before you add a new renderer named RenderSVGRect. See comment #8 rounded rects should be RenderSVGPath (or Shape), otherwise we would get performance issues and regressions on pointer events.
Yeah, I didn't write too much detail about our plans. Btw you are right. Rounded rects could be properly rendered just via path object. So we will have a RefPtr<Path> member in the base class and we can fallback to it if it's needed. (this was your idea too :) )

> Btw. where is RenderSVGRect.cpp in your patch?
Yes, I missed to add them in my previous patch, but I fixed it.

[Renata Hodovan]

It seems I killed ews again B-) Maybe should I split the patch into three parts?

[Dirk Schulze]

Comment on attachment 103498 [details]
Proposed patch

I asked you to just upload the changes in WebCore, without DRT results.

The rounded corner issue still exists. I'd create an RenderSVGPath renderer for that case like discussed on IRC.

RenderSVGRect inherits from RenderSVGPath. Do you plan to use RefPtr<Path> for special cases on RenderSVGRect? If so I'm fine. RenderSVGRect would be faster in calculating bounding boxes, but falls back to RenderSVGPath logic when it comes to pointer events. We can just handle pointer events for simple fill and stroke operations. If we have dashed arrays or special line caps, we need to fallback to RenderSVGPath logic.

Also I thought Niko did not want a new class but switches in RenderSVGPath? Did you change his mind? :P I prefer the classes, but again, we need to be careful.

For the long term. Which renderer do you plan to create? I would think of RenderSVGRect and RenderSVGEllipse, circle could be handled by ellipses as well. I don't see other renderers, right?

[Renata Hodovan]

> I asked you to just upload the changes in WebCore, without DRT results.
Sorry, should I send it again?
 
> The rounded corner issue still exists. I'd create an RenderSVGPath renderer for that case like discussed on IRC.
>
> RenderSVGRect inherits from RenderSVGPath. 
That's right. But only in this first step. Our goal is to inherit the renderers of all SVG shapes (Path's to) from RenderSVGShape what will contain a lot of code from the current RenderSVGPath. This common renderer will have the RefPtr<Path> member for the cases what need fallback.

> Do you plan to use RefPtr<Path> for special cases on RenderSVGRect? If so I'm fine. RenderSVGRect would be faster in calculating bounding boxes, but falls back to RenderSVGPath logic when it comes to pointer events. We can just handle pointer events for simple fill and stroke operations. If we have dashed arrays or special line caps, we need to fallback to RenderSVGPath logic.
> 
> Also I thought Niko did not want a new class but switches in RenderSVGPath? Did you change his mind? :P I prefer the classes, but again, we need to be careful.
Yeah, Niko gave me his blessing :P

> For the long term. Which renderer do you plan to create? I would think of RenderSVGRect and RenderSVGEllipse, circle could be handled by ellipses as well. I don't see other renderers, right?
Rect, ellipse, line, path...

[Dirk Schulze]

(In reply to comment #16)
> > I asked you to just upload the changes in WebCore, without DRT results.
> Sorry, should I send it again?
RenderSVGPath to RenderSVGShape is just a renaming. I really do fear regressions with pointer events. I wrote it in multiple comments before. I'd like to hear your ideas how to avoid that! Again, painting is not the problem, and I'm fine with these changes as long as we do not have performance regressions on cairo, cg, qt, skia.

But how do you want to manage pointer events on stroke? Your first indention definitely doesn't work. You need a path to determine if a pointer is on the stroke or not!

And if we have to create a path, the only benefit would be painting. This would be a progression anyway. But I'd like to have a concept before creating new classes.

[Renata Hodovan]

Created attachment 104154 [details]
Draft patch

This is just a draft patch. I tried to follow Krit's instructions and this is how I can imagine the introduction of the new renderers. Any comments are welcomed. :)

[Renata Hodovan]

Sorry for the licences of RenderSVGPath... copy-paste error... I'll fix them ofc :)

[Nikolas Zimmermann]

Comment on attachment 104154 [details]
Draft patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104154&action=review

First extensive round of review! :-)

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:-50
> -class BoundingRectStrokeStyleApplier : public StrokeStyleApplier {
> -public:

Did you use "svn cp RenderSVGPath.cpp RenderSVGShape.cpp" as first thing to preserve the history of everything that you're moving?
Otherwhise you'll have to redo it :-)

> Source/WebCore/rendering/svg/RenderSVGPath.h:3
> + * Copyright (C) 2011 University of Szeged
> + * Copyright (C) 2011 Renata Hodovan ([e-mail])

You already commented about the wrong license changes, I just wanted to re-mention it.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:40
> +    , m_rect(FloatRect())

Not needed.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:54
> +FloatRect RenderSVGRect::shapeBoundingRect()

This should be const, no?
Why use a different terminology here btw, I'd say "objectBoundingBox" is the right name.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:64
> +    SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
> +    SVGRectElement* rect = static_cast<SVGRectElement*>(element);

Why cast to SVGStyledTrafoElem first?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:68
> +    if (m_hasRx || m_hasRy || !style()->svgStyle()->strokeDashArray().isEmpty()) {
> +       RenderSVGShape::createShape();

The fallback path should be described with a comment.
Is this enough? What about combinations of stroke-linecap="join" and different miter limits? I'm sure Dirk can come up with more cases.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:80
> +    float xValue = rect->x().value(rect);
> +    float yValue = rect->y().value(rect);

Side note: This code is derived from SVGRectElement::toPathData.
It should be possible to eliminate this method as well now - aka. move it into RenderSVGRect, and use it from RenderSVGShape when path fallback mode is enabled.
Needs a bit more investigation, but I wanted to mention it so we won't forget.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:82
> +    FloatRect frect(xValue, yValue, widthValue, heightValue);
> +    m_rect = frect;

Avoid this temporary variable, just use m_rect = FloatRect(...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:85
> +FloatRect RenderSVGRect::strokeBoundingShape()

Here a const should be added as well, and this could be named "strokeBoundingBox" for consistency.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:93
> +    float strokeWidth = style()->svgStyle()->strokeWidth().value(static_cast<SVGRectElement*>(node()));
> +    BoundingRectStrokeStyleApplier strokeStyle(this, style());
> +    FloatRect strokeRect = m_rect;
> +    strokeRect.inflate(strokeWidth / 2);
> +    return strokeRect;

This deserves a comment as well, that this approximation always holds true for the cases we're handling with in non-fallback path mode.
The strokeWidth calculation should be moved to either a free static inline function taking a RenderStyle* or a private member function, as it's duplicated in several places.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:101
> +    if (!hasPath())
> +        context->fillRect(m_rect);
> +    else
> +        RenderSVGShape::fillShape(context);

I'd use the early return style for the hasPath() case consistently. If you don't want this, swap conditions to save one negation.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:111
> +    float strokeWidth = style()->svgStyle()->strokeWidth().value(static_cast<SVGRectElement*>(node()));
> +    context->strokeRect(m_rect, strokeWidth);

You could avoid the temporary variable loosing readability. Hm, I guess all our compilers will optimize that out anyway, so it's probably fine.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:123
> +    float strokeWidth = svgStyle->strokeWidth().value(static_cast<SVGRectElement*>(node()));
> +    FloatRect innerStrokeRect = m_rect;
> +    FloatRect outerStrokeRect = m_rect;
> +    innerStrokeRect.inflate(-strokeWidth / 2);
> +    outerStrokeRect.inflate(strokeWidth / 2);
> +    if (determinePointLocation(point, innerStrokeRect) != InsideShape && determinePointLocation(point, outerStrokeRect) != OutsideShape)
> +        return true;

While this is beautiful readable, it deserves a comment as well and could be compactified further with the use of ternary operator.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:131
> +    if (determinePointLocation(point, m_rect) != OutsideShape)
> +        return true;
> +    return false;

You can fold this to be more compact with the ternary operator.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:137
> +    if (rect.x() <= point.x() && rect.maxX() >= point.x() && rect.y() <= point.y() && rect.maxY() >= point.y())

if (rect.contains(point)) is equivalent.

The whole function is more readable if you'd use
if (!rect.contains(point))
    return OutsideShape;
first.

> Source/WebCore/rendering/svg/RenderSVGRect.h:38
> +    RenderSVGRect(SVGStyledTransformableElement*);

This should be explicit, so RenderSVGRect(12) is not constructable.

> Source/WebCore/rendering/svg/RenderSVGRect.h:46
> +    void clearShape();
> +    void createShape();

clearShape is only ever used in conjuncation with createShape. It seems superfluous to call m_path.clear() and then assiging m_path to a new RefPtr afterwards, same for clearing m_rect and then re-assigning it eventually.
We should rather watch out that createShape() clears m_rect on failure. Then clearShape() can be removed alltogether.

> Source/WebCore/rendering/svg/RenderSVGRect.h:47
> +    bool isEmpty() { return pathPtr() ? RenderSVGShape::isEmpty() : m_rect.isEmpty(); };

How is that supposed to work? pathPtr() ASSERTS that m_path.get() is true. So you'll never hit the m_rect.isEmpty() branch.
Did you test this with a debug build?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:69
> +    m_path = adoptPtr(new Path());

s/Path()/Path/.

Hm, we have to watch out for the possible impact of that change. Previously Path was allocated on the stack in RenderSVGPath -- you're changing this.
Path is fast malloced though I'm worried about fragmentation issues when allocing/deallocing thousands of <path> dynamically.
I'd love if you could benchmark the impact on memory consumption/fragmentation.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:73
> +    element->toPathData(path());

This toPathData() method could be a virtual method of RenderSVGShape.
SVGRectElement::toPathData() can then move to RenderSVGRect and override toPathData().
This way we can slowly eliminate the toPathData() spread in svg/ and have shape/path creation completly encapsulated in the render tree.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:76
> +bool RenderSVGShape::isEmpty()

This could be const as well, no?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:113
> +bool RenderSVGShape::fillContains(const FloatPoint& point, bool requiresFill, WindRule fillRule)

I guess you really only moved this method from RenderSVGPath w/o any fundamental change?
Could it be const?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:114
> +{

Hmm, isn't m_fillBoundingBox of RenderSVGShape and RenderSVGRect::m_fillBoundingBox equal all the time? (when non-fallback mode is used).
Seems unfortunate to duplicate the storage. (Though I see why it's needed atm).

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:115
> +    if (!m_fillBoundingBox.contains(point))

Ah here you're using the FloatRect contains method, should do that as well in RenderSVGRect, as I demanded there.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:125
> +bool RenderSVGShape::strokeContains(const FloatPoint& point, bool requiresStroke)

I guess you really only moved this method from RenderSVGPath w/o any fundamental change?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:153
> +        clearShape();
> +        createShape();

As said before, this should be folded into a single call.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:226
> +    tempPath = path();

tempPath = m_path is faster.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:268
> +        usePath = pathPtr();

usePath = &m_path is faster.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:272
> +        usePath = pathPtr();

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:277
> +        strokeShape(context);

Early return here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:278
> +    } else if (fallbackColor.isValid()) {

Early return if fallbackColor is invalid.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:394
> +    m_fillBoundingBox = shapeBoundingRect();

Maybe RenderSVGRects shapeBoundingRect should calculate the x/y/w/h FloatRect here, instead of returning m_rect.
This way we could save the additional m_rect in RenderSVGRect, maybe?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:413
> +    if (this->isSVGPath()) {

this-> is not needed.

> Source/WebCore/rendering/svg/RenderSVGShape.h:74
> +    void setNeedsPathUpdate() { m_needsShapeUpdate = true; }

This needs a rename!

> Source/WebCore/rendering/svg/RenderSVGShape.h:107
> +    virtual bool isEmpty();

const.

> Source/WebCore/rendering/svg/RenderSVGShape.h:111
> +    virtual FloatRect shapeBoundingRect();
> +    virtual FloatRect strokeBoundingShape();

const + rename.

> Source/WebCore/rendering/svg/RenderSVGShape.h:116
> +    Path& path()

This shouldn't be needed.

> Source/WebCore/rendering/svg/RenderSVGShape.h:122
> +    Path* pathPtr()

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.h:128
> +    bool hasPath() { return m_path.get(); }

const.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:228
>                      // When the layout size changed and when using relative values tell the RenderSVGPath to update its Path object

the comment is outdated now, please update.

> Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:293
> +    if (object.isSVGShape()) {
>          const RenderSVGPath& path = static_cast<const RenderSVGPath&>(object);

Be careful here! this is wrong and crashy. Needs investigation. I'd immediately think you want to have two isSVGPath/isSVGRect branches
and then share code for in "writeSVGShape" that are common to path and rect.

> Source/WebCore/svg/SVGRectElement.cpp:-169
> -

I'd leave this out.

> Source/WebCore/svg/SVGRectElement.cpp:-187
> -

Ditto.

> Source/WebCore/svg/SVGRectElement.cpp:203
> +    // By default, any subclass is expected to do path-based drawing

The comment is not needed I think.

> Source/WebCore/svg/SVGRectElement.h:25
> +#include "RenderSVGRect.h"

You should move this include to the cpp file.

[Renata Hodovan]

Created attachment 104684 [details]
Draft patch

It's still a draft patch (without changelog and layout test updates).

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:68
>> +       RenderSVGShape::createShape();
> 
> The fallback path should be described with a comment.
> Is this enough? What about combinations of stroke-linecap="join" and different miter limits? I'm sure Dirk can come up with more cases.

I'm waiting for the other fallback ideas :)

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:80
>> +    float yValue = rect->y().value(rect);
> 
> Side note: This code is derived from SVGRectElement::toPathData.
> It should be possible to eliminate this method as well now - aka. move it into RenderSVGRect, and use it from RenderSVGShape when path fallback mode is enabled.
> Needs a bit more investigation, but I wanted to mention it so we won't forget.

I agree with this but I think it should be made in a follow-up patch (I should do the same with the toPathData() funtions of the other SVG elements, which are not touched by this patch)

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:226
>> +    tempPath = path();
> 
> tempPath = m_path is faster.

m_path is an OwnPtr, so tempPath = m_path doesn't work. I could change it to tempPath = *m_path; if you like it better.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:268
>> +        usePath = pathPtr();
> 
> usePath = &m_path is faster.

The same problem here. Maybe: usePath = m_path.get();  ?

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:272
>> +        usePath = pathPtr();
> 
> Ditto.

The same again...

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:394
>> +    m_fillBoundingBox = shapeBoundingRect();
> 
> Maybe RenderSVGRects shapeBoundingRect should calculate the x/y/w/h FloatRect here, instead of returning m_rect.
> This way we could save the additional m_rect in RenderSVGRect, maybe?

I think we need to store this because m_fillBoundingBox is used at several places.

>> Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:293
>>          const RenderSVGPath& path = static_cast<const RenderSVGPath&>(object);
> 
> Be careful here! this is wrong and crashy. Needs investigation. I'd immediately think you want to have two isSVGPath/isSVGRect branches
> and then share code for in "writeSVGShape" that are common to path and rect.

This part really was a bit messy. I refactored it.

All the other suggestions are fixed. Besides I made a little refatoring around the fallback of strokeContains.

[WebKit Review Bot]

Attachment 104684 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/rendering/svg/RenderSVGShape.cpp:145:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 104684 [details]
Draft patch

Attachment 104684 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9464788

[WebKit Review Bot]

Comment on attachment 104684 [details]
Draft patch

Attachment 104684 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9465840

[Collabora GTK+ EWS bot]

Comment on attachment 104684 [details]
Draft patch

Attachment 104684 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9468732

[Early Warning System Bot]

Comment on attachment 104684 [details]
Draft patch

Attachment 104684 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9468731

[Renata Hodovan]

Created attachment 104687 [details]
Draft patch

[Gyuyoung Kim]

Comment on attachment 104687 [details]
Draft patch

Attachment 104687 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9461897

[WebKit Review Bot]

Comment on attachment 104687 [details]
Draft patch

Attachment 104687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9467813

[Collabora GTK+ EWS bot]

Comment on attachment 104687 [details]
Draft patch

Attachment 104687 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9463844

[WebKit Review Bot]

Comment on attachment 104687 [details]
Draft patch

Attachment 104687 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9468742

[Renata Hodovan]

Created attachment 104841 [details]
Draft patch

Build fix

[WebKit Review Bot]

Comment on attachment 104841 [details]
Draft patch

Attachment 104841 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9477706

New failing tests:
css2.1/20110323/replaced-intrinsic-002.htm
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
fast/backgrounds/size/contain-and-cover.html
svg/W3C-I18N/text-anchor-dirNone-anchorMiddle.svg
svg/W3C-I18N/g-dirRTL-ubNone.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorStart.svg
fast/repaint/svg-layout-root-style-attr-update.html
svg/W3C-I18N/text-anchor-dirLTR-anchorMiddle.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg
svg/W3C-I18N/text-anchor-dirNone-anchorEnd.svg
svg/W3C-I18N/g-dirLTR-ubOverride.svg
svg/W3C-I18N/g-dirLTR-ubNone.svg
svg/W3C-I18N/g-dirRTL-ubOverride.svg
fast/forms/implicit-submission.html
css2.1/20110323/replaced-intrinsic-001.htm

[Renata Hodovan]

Created attachment 105479 [details]
Propsed patch

SVG resources were used incorrectly in my previous patch (postApplyResource() was never called and every resources were considered as SolidColorResource).
With this patch we don't have regressions anymore and we get 4-5% performace improvement by this test: http://www.themaninblue.com/experiment/AnimationBenchmark/html/ (ofc I substituted the circles with rects)

[Renata Hodovan]

> With this patch we don't have regressions anymore and we get 4-5% performace improvement by this test: http://www.themaninblue.com/experiment/AnimationBenchmark/html/ (ofc I substituted the circles with rects)

This one is the right test: http://www.themaninblue.com/experiment/AnimationBenchmark/svg/

[Dirk Schulze]

Comment on attachment 105479 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105479&action=review

Some more comments.

> Source/WebCore/rendering/svg/RenderSVGPath.h:34
> +class RenderSVGPath : public RenderSVGShape {

Add a FIXME that you plan to remove RenderSVGPath with all the changes in DRT afterwards in a followup.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:51
> +    SVGRectElement* rect = static_cast<SVGRectElement*>(node());

Can you add an assert here for rect please?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:55
> +    if (m_hasRx || m_hasRy || !style()->svgStyle()->strokeDashArray().isEmpty()) {

just create the path for rects with rounded corners. More later on this review...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:96
> +    if (hasPath())
> +        RenderSVGShape::fillShape(context);
> +    else
> +        context->fillRect(m_rect);

even if we have to create paths for pointer events, it could be faster to use context->fillRect instead of filling the path (of course just if fillRect respects dashArray and different styles; you have to check this first). We just need path for rects with rounded corners.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:105
> +    if (hasPath()) {
> +        RenderSVGShape::strokeShape(context);
> +        return;
> +    }
> +    context->strokeRect(m_rect, this->strokeWidth());

Ditto

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:108
> +bool RenderSVGRect::strokeContainsSlowCase(const FloatPoint& point)

what is the fast case?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:110
> +    if (hasPath())

You just created a path for dash arrays. But there is more stuff to respect: lineCap, mitterLimit. The W3C test suite has more examples. This is not needed for stroking or filling a rect, since the graphic libraries support it with rects automatically. But you have to think about this on pointer events.

I'd just create the path, for _pointer events_ if we have either have a dahsarray, some special settings for mitterlimit or lineCap, and not on every layout call. Use the stored rects as much as possible. Just fallback to the path if there is no other way to determine something. Would it make sense to store a boolean for isPath() in RenderShape instead of checking the pointer? Not sure if we loose time on the check.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:121
> +    FloatRect innerStrokeRect = m_rect;
> +    FloatRect outerStrokeRect = m_rect;
> +    innerStrokeRect.inflate(-strokeWidth / 2);
> +    outerStrokeRect.inflate(strokeWidth / 2);
> +    return (determinePointLocation(point, innerStrokeRect) != InsideShape
> +            && determinePointLocation(point, outerStrokeRect) != OutsideShape) ?  true : false;

you should store inner and outer rect. But don't forget to clear the rects on style changes. This could be

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:126
> +    return (determinePointLocation(point, m_rect) != OutsideShape) ? true : false;

isn't m_rect.contains(point.x(), point.y()) enough here? Also, you are missing the rounded corner case.

And I'd like to see a test case for pointer events with rounded corners, if you didn't see regressions.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:134
> +    // Test whether the rect contains the certain point or not
> +    if (!rect.contains(point))
> +        return OutsideShape;
> +    return (rect.x() < point.x() && rect.maxX() > point.x() && rect.y() < point.y() && rect.maxY() > point.y()) ? InsideShape : OnStroke;

Can we add a comprises() method in FloatRect, that does not include the boundingBox of the rect? So we would not need this method here.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
> +    if (/*path && */!(resourceMode & ApplyToTextMode)) {

can you remove this comment?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:75
> +    return path().isEmpty();

why not just m_path->isEmpty();?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:91
> +    return path().strokeBoundingRect(&strokeStyle);

m_path->stroke...

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:102
> +    return path().strokeContains(&applier, point);

m_path->strokeC...

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:107
> +    return path().contains(point, fillRule);

m_path->contains(...

> Source/WebCore/rendering/svg/RenderSVGShape.h:108
> +protected:

The order should be public, protected, private sections

> Source/WebCore/rendering/svg/RenderSVGShape.h:127
> +    Path* pathPtr() const
> +    {
> +        ASSERT(m_path.get());
> +        return m_path.get();
> +    };

Looks like you don't use it. Remove it please.

[Renata Hodovan]

Created attachment 105641 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105479&action=review

>> Source/WebCore/rendering/svg/RenderSVGPath.h:34
>> +class RenderSVGPath : public RenderSVGShape {
> 
> Add a FIXME that you plan to remove RenderSVGPath with all the changes in DRT afterwards in a followup.

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:51
>> +    SVGRectElement* rect = static_cast<SVGRectElement*>(node());
> 
> Can you add an assert here for rect please?

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:55
>> +    if (m_hasRx || m_hasRy || !style()->svgStyle()->strokeDashArray().isEmpty()) {
> 
> just create the path for rects with rounded corners. More later on this review...

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:96
>> +        context->fillRect(m_rect);
> 
> even if we have to create paths for pointer events, it could be faster to use context->fillRect instead of filling the path (of course just if fillRect respects dashArray and different styles; you have to check this first). We just need path for rects with rounded corners.

Okay, fillRect() can handle these styles. Updated.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:105
>> +    context->strokeRect(m_rect, this->strokeWidth());
> 
> Ditto

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:108
>> +bool RenderSVGRect::strokeContainsSlowCase(const FloatPoint& point)
> 
> what is the fast case?

This virtual function will contain the shape specific calculation of strokeContains() operation.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:110
>> +    if (hasPath())
> 
> You just created a path for dash arrays. But there is more stuff to respect: lineCap, mitterLimit. The W3C test suite has more examples. This is not needed for stroking or filling a rect, since the graphic libraries support it with rects automatically. But you have to think about this on pointer events.
> 
> I'd just create the path, for _pointer events_ if we have either have a dahsarray, some special settings for mitterlimit or lineCap, and not on every layout call. Use the stored rects as much as possible. Just fallback to the path if there is no other way to determine something. Would it make sense to store a boolean for isPath() in RenderShape instead of checking the pointer? Not sure if we loose time on the check.

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:121
>> +            && determinePointLocation(point, outerStrokeRect) != OutsideShape) ?  true : false;
> 
> you should store inner and outer rect. But don't forget to clear the rects on style changes. This could be

I'm not sure it's worth to store them constatly, so I left out this modification fo far. But if you are sure, I will do it ofc. Btw where should I catch the style change in this case?

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:126
>> +    return (determinePointLocation(point, m_rect) != OutsideShape) ? true : false;
> 
> isn't m_rect.contains(point.x(), point.y()) enough here? Also, you are missing the rounded corner case.
> 
> And I'd like to see a test case for pointer events with rounded corners, if you didn't see regressions.

It's enuogh. I just wanted to use a uniform way to determine the location of the points. But determainPointLocation() is fade into the past, so this solution is a trash too.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:134
>> +    return (rect.x() < point.x() && rect.maxX() > point.x() && rect.y() < point.y() && rect.maxY() > point.y()) ? InsideShape : OnStroke;
> 
> Can we add a comprises() method in FloatRect, that does not include the boundingBox of the rect? So we would not need this method here.

Done.

>> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
>> +    if (/*path && */!(resourceMode & ApplyToTextMode)) {
> 
> can you remove this comment?

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:75
>> +    return path().isEmpty();
> 
> why not just m_path->isEmpty();?

I do already :P

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:91
>> +    return path().strokeBoundingRect(&strokeStyle);
> 
> m_path->stroke...

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:102
>> +    return path().strokeContains(&applier, point);
> 
> m_path->strokeC...

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:107
>> +    return path().contains(point, fillRule);
> 
> m_path->contains(...

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.h:108
>> +protected:
> 
> The order should be public, protected, private sections

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.h:127
>> +    };
> 
> Looks like you don't use it. Remove it please.

Done.

[Dirk Schulze]

Comment on attachment 105641 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105641&action=review

Some more snippets.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect.svg:19
> +<rect x="40" y="40" rx="30" ry="30" width="400" height="300" pointer-events="visibleStroke" />

the test looks good, just missing the mousmove commends :)

> Source/WebCore/platform/graphics/FloatRect.cpp:67
> +        return (x() < point.x() && maxX() > point.x() && y() < point.y() && maxY() > y()) ? true : false;

remove ? true : false and the unnecessary braces.

> Source/WebCore/platform/graphics/FloatRect.cpp:68
> +    return false;

what about early return if rect does not contain point?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:110
> +    // To deceide the stroke containing we create two rects which represent the inner and
> +    // the outer stroke borders. The containing exists, if the point is between them.
> +    float strokeWidth = this->strokeWidth();

You forgot to check for mitterlimit, dash array and line cap, if they do not use the default values, you might need to create the path and check the style in RenderSVGShape::strokeContainsSlowCase.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:116
> +    return ((!innerStrokeRect.contains(point) || !innerStrokeRect.comprises(point))
> +            && outerStrokeRect.contains(point)) ?  true : false;

Remove ?  true : false; and unnecessary braces.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:122
> +    if (isPath())
> +        return RenderSVGShape::fillContainsSlowCase(point, fillRule);

Because you might create a path just for strokeContains (because of the different stroke styles), it might be unnecessary to call RenderSVGShape::fillContainsSlowCase. You just need this call for rounded corners. Check for the rounded corner instead.

[Dirk Schulze]

Comment on attachment 105641 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105641&action=review

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:90
> +    if (isPath())
> +        return RenderSVGShape::strokeBoundingBox();
> +    // If we inflate the m_rect with the half size strokeWidth, we get the strokeBoundingBox in non-fallback path mode.
> +    BoundingRectStrokeStyleApplier strokeStyle(this, style());
> +    FloatRect strokeRect = m_rect;
> +    strokeRect.inflate(this->strokeWidth() / 2);
> +    return strokeRect;

we might need a fallback for dash array. If dash array was set, we might get no stroke, or at least one side of the rect does not need to have a stroke. Hard to determine that without the help of the underlaying graphic platform.

Also we don't need to the path just for rounded corners... The same for mitterlimit and other styles... Just dash array needs a fallback to path.

[Renata Hodovan]

Created attachment 105940 [details]
Propsed patch

[Dirk Schulze]

Comment on attachment 105940 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105940&action=review

Can you check if the cursor "hand"is just visible over the visible parts of the rect on this example please? :

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<clipPath id="clip">
    <rect width="55" height="110"/>        
</clipPath>
<a xlink:href="#">
<rect x="5" y="5" width="100" height="100" stroke="blue" fill="green" stroke-width="10" clip-path="url(#clip)"/>
</a>
</svg>

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:50
> +    // Clear m_rect.
> +    m_rect = FloatRect();

Can you call it m_boundingBox?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:55
> +    // Fallback to the parent if rect has special features.

Fallback to RenderSVGShape...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:73
> +    // To deceide the stroke containing we create two rects which represent the inner and
> +    // the outer stroke borders. The containing exists, if the point is between them.

To decide if the stroke contains a point.... A stroke contains the point, if ...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:93
> +    // If we inflate the m_rect with the half size strokeWidth, we get the strokeBoundingBox in non-fallback path mode.

s/m_rect/bounding box/

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:95
> +    FloatRect strokeRect = m_rect;
> +    strokeRect.inflate(this->strokeWidth() / 2);

hm, isn't it just m_outerStrokeRect?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:109
> +bool RenderSVGRect::strokeContainsSlowCase(const FloatPoint& point)

I really dislike this name, because this is clearly not the slow case. What about shapeDependentStrokeContains?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:115
> +bool RenderSVGRect::fillContainsSlowCase(const FloatPoint& point, const WindRule& fillRule) const

same here, what about shapeDependentFillContains?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:166
> +    if (m_needsBoundariesUpdate)
> +        updateCachedBoundariesInParents = true;

Don't you need to clear the rects for RenderSVGRect here as well and look if you must set/remove fallback flag? I thought this is called for stroke style changes.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:179
> +    if (needsShapeUpdate || m_needsBoundariesUpdate) {
> +        updateCachedBoundaries();
> +        m_needsBoundariesUpdate = false;
> +    }

Maybe you can make updateCachedBoundaries() virtual and add it ti RenderSVGRect as well. After calling this function you have to decide if you need to update boundaries or may need to call RenderSVGShape::updateCachedBoundaries(). Make sure that you don't do work twice. Would be insane to refresh all boundaries in createShape() and right afterwards in updateCacheBoundaries().

> Source/WebCore/rendering/svg/RenderSVGShape.h:85
> +    void setIsFillFallback() { m_fillFallback = true; }
> +    bool isFillFallback() const { return m_fillFallback; }

Shouldn't setIsFallback take a argument, so that you can reset it later if needed?


I couldn't review everything. I'll definitely come back to the review, but must go... I didn't not check the correct use of isFallback right now. Still some comments that need to address.

[Dirk Schulze]

Comment on attachment 105940 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105940&action=review

Second part of the review. The handling of fallback looks good to me.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect.svg:21
> +<svg xmlns="http://www.w3.org/2000/svg">
> +<defs>
> +
> +<style type="text/css"><![CDATA[
> +rect {
> +	fill: yellow;
> +	fill-opacity: 0.5;
> +	stroke: green;
> +	stroke-width: 20px;
> +	stroke-linejoin: bevel;
> +	stroke-dasharray: 50 15;
> +}
> +rect:hover {
> +	stroke: red;
> +} 
> +]]></style>
> +</defs>
> +
> +<rect x="40" y="40" rx="30" ry="30" width="400" height="300" pointer-events="visibleStroke" />
> +
> +</svg>

I'd like to see automatized dumpAsText tests. You added manual tests. See LayoutTests/svg/custom/pointer-events-on-svg-with-pointer.xhtml

> LayoutTests/svg/custom/pointer-events-with-linecaps-and-miterlimits.svg:22
> +<svg xmlns="http://www.w3.org/2000/svg">
> +<defs>
> +
> +<style type="text/css"><![CDATA[
> +rect {
> +	fill: none;
> +	fill-opacity: 0.5;
> +	stroke: green;
> +	stroke-width: 40px;
> +	stroke-miterlimit: 1;
> +	stroke-linecap: round;
> +	stroke-dasharray: 50 54;
> +}
> +rect:hover {
> +	stroke: red;
> +} 
> +]]></style>
> +</defs>
> +
> +<rect x="40" y="40" width="400" height="300" pointer-events="visibleStroke" />
> +
> +</svg>

ditto.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:86
> +    if (isFillFallback())
> +        return RenderSVGShape::objectBoundingBox();
> +    return m_rect;

hm, why do you need the fallback here? rounded corners don't matter for the bounding box. Ah I guess because you created a Path, you don't have information.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:111
> +    return (!m_innerStrokeRect.contains(point) || !m_innerStrokeRect.comprises(point))

Isn't it enough ti check for !m_innerStrokeRect.comprises(point)? Doesn't comprises just check if a point is inside the rect boundaries?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:56
> +    , m_needsBoundariesUpdate(false) // default is false, the cached rects are empty from the beginning
> +    , m_needsShapeUpdate(true) // default is true, so we grab a Path object once from SVGStyledTransformableElement
> +    , m_needsTransformUpdate(true) // default is true, so we grab a AffineTransform object once from SVGStyledTransformableElement

Can you change the style of these comments to sentences.

[Renata Hodovan]

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:179
> > +    if (needsShapeUpdate || m_needsBoundariesUpdate) {
> > +        updateCachedBoundaries();
> > +        m_needsBoundariesUpdate = false;
> > +    }
> 
> Maybe you can make updateCachedBoundaries() virtual and add it ti RenderSVGRect as well. After calling this function you have to decide if you need to update boundaries or may need to call RenderSVGShape::updateCachedBoundaries(). Make sure that you don't do work twice. Would be insane to refresh all boundaries in createShape() and right afterwards in updateCacheBoundaries().
updateCacheBoundaries() contains only general tasks, except the part with markers. I can separate it into a new virtual function, but the others should stay where they are now. In my opinion at least :)

> I'd like to see automatized dumpAsText tests. You added manual tests. See LayoutTests/svg/custom/pointer-events-on-svg-with-pointer.xhtml
This test works with SVG elements and not attributes. How can I bind attributes, like stroke, to an eventlistener?

[Dirk Schulze]

(In reply to comment #43)
> > > Source/WebCore/rendering/svg/RenderSVGShape.cpp:179
> > > +    if (needsShapeUpdate || m_needsBoundariesUpdate) {
> > > +        updateCachedBoundaries();
> > > +        m_needsBoundariesUpdate = false;
> > > +    }
> > 
> > Maybe you can make updateCachedBoundaries() virtual and add it ti RenderSVGRect as well. After calling this function you have to decide if you need to update boundaries or may need to call RenderSVGShape::updateCachedBoundaries(). Make sure that you don't do work twice. Would be insane to refresh all boundaries in createShape() and right afterwards in updateCacheBoundaries().
> updateCacheBoundaries() contains only general tasks, except the part with markers. I can separate it into a new virtual function, but the others should stay where they are now. In my opinion at least :)
I'm fine with this idea as well.

> 
> > I'd like to see automatized dumpAsText tests. You added manual tests. See LayoutTests/svg/custom/pointer-events-on-svg-with-pointer.xhtml
> This test works with SVG elements and not attributes. How can I bind attributes, like stroke, to an event listener?
You don't need dynamic testing of stroke. You just want to know if an event listener fires if you use pointer-events. Some pseudo code:

<rect id="fallback" width="200" height="200" fill="green" onclick="shouldFire()"/>
<rect id="strokedRect" x="5" y="5" width="190" height="190" fill="none" stroke="blue" stroke-width="10" pointer-events="visibleStroke" style="<yourStrokeStyle>" onclick="shouldNotFire()"/>
<script>
function shouldFire() {
  alert("great it works!");
}
function shouldNotFire() {
  alert("Oops! Something is wrong");
}
</script>

You have to move the pointer of the mouse to a point on the stroke of the second rect, where the pointer _is not_ visible and fire the mouse click event.

[Renata Hodovan]

Created attachment 106727 [details]
Propsed patch

[Andreas Kling]

Comment on attachment 106727 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106727&action=review

A little drive-by nitpicking on my way to work:

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> +    bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> +    bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> +    // Fallback to RenderSVGShape if rect has special features.
> +    if (hasRx || hasRy) {

This would be slightly more efficient if written as:
if (rect->hasAttribute(SVGNames::rxAttr) || rect->hasAttribute(SVGNames::ryAttr) {
(Since you don't use hasRx or hasRy for anything but this.)

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:89
> +FloatRect RenderSVGRect::strokeBoundingBox()

This method reads like it should be const, just like objectBoundingBox().
Maybe I'm missing something?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:106
> +bool RenderSVGRect::shapeDependentStrokeContains(const FloatPoint& point)

Ditto.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:111
> +bool RenderSVGRect::shapeDependentFillContains(const FloatPoint& point, const WindRule& fillRule) const

There's no benefit to passing an enum (like WindRule) as a const-reference. "const WindRule fillRule" would be just fine.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:107
> +bool RenderSVGShape::shapeDependentFillContains(const FloatPoint& point, const WindRule& fillRule) const

Same comment as earlier about "const WindRule&"

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:112
> +bool RenderSVGShape::fillContains(const FloatPoint& point, bool requiresFill, WindRule fillRule)

"WindRule fillRule" could be marked const since the function does not modify it. NABD really.

> Source/WebCore/rendering/svg/RenderSVGShape.h:97
> +    // Hit-detection seperated for the fill and the stroke

seperated -> separated

> Source/WebCore/rendering/svg/RenderSVGShape.h:130
> +    bool m_needsBoundariesUpdate : 1;
> +    bool m_needsShapeUpdate : 1;
> +    bool m_needsTransformUpdate : 1;
> +    bool m_fillFallback : 1;
> +    bool m_strokeContainsFallBack : 1;

You should move these to the end of the member list to maximize space efficiency.

[Dirk Schulze]

Comment on attachment 106727 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106727&action=review

It definitely looks a lot better and you're nearly done! But still some more comments.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect.xhtml:14
> +	stroke-dasharray: 50 15;

se later comment.

> LayoutTests/svg/custom/pointer-events-with-linecaps-and-miterlimits.xhtml:14
> +	stroke-dasharray: 20 50;

lion changed the behavior of dash array. Not sure if we can influence it in GraphicsContextCG (just noticed it on running pixel tests). Maybe we should leave out dash array and check just the corner (11, 11). Not that we have to roll out the patch because lion comments about failures.

> Source/WebCore/ChangeLog:9
> +        handle the fallback cases (e.g.: rounded rects, special strokes, pointer events, etc.)

Missing dot at the end of the sentence :)

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:52
> +    if (style()->svgStyle()->hasMarkers()) {
>          FloatRect markerBounds = calculateMarkerBoundsIfNeeded();
>          if (!markerBounds.isEmpty())
> -            m_strokeAndMarkerBoundingBox.unite(markerBounds);
> +            strokeBoundingBox().unite(markerBounds);

Oh that looks wrong! That makes no sense. You get a FloatRect from strokeBoundingBox() and make a union with markerBounds but neither return it nor store it? What about m_strokeAndMarkerBoundingBox? Why not store the values here?

Doesn't it fire on a test? maybe on dynamically removing the markers of a path? Is so, I'd like to see another test.

> Source/WebCore/rendering/svg/RenderSVGPath.h:44
> +    void inflateWithMarkerBounds();

put a virtual in front of this.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:67
> +    float widthValue = rect->width().value(rect);
> +    if (widthValue <= 0)
> +        return;
> +
> +    float heightValue = rect->height().value(rect);
> +    if (heightValue <= 0)
> +        return;

What about:

FloatSize boundingBoxSize(rect->width().value(rect), rect->height().value(rect));
if (boundingBoxSize.isEmpty())
    return;

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:71
> +    float xValue = rect->x().value(rect);
> +    float yValue = rect->y().value(rect);
> +    m_boundingBox = FloatRect(xValue, yValue, widthValue, heightValue);

m_boundingBox = FloatRect(FloatPoint(rect->x().value(rect), rect->y().value(rect)), boundingBoxSize);

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:392
> +void RenderSVGShape::inflateWithMarkerBounds()
> +{
> +}

remove it here.

> Source/WebCore/rendering/svg/RenderSVGShape.h:112
> +    virtual void inflateWithMarkerBounds();

empty braces behind the function.

[Renata Hodovan]

Created attachment 107031 [details]
Propsed patch

[Renata Hodovan]

Created attachment 107032 [details]
Propsed patch

[WebKit Review Bot]

Attachment 107031 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Did you forgot to address kliegs mentions?

[Nikolas Zimmermann]

Comment on attachment 107032 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107032&action=review

You're making good progress, still lots of comments from a first quick round of review. Still didn't check everything in detail.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect-expected.txt:1
> +PASSED: fallbackRect had pointer

No ChangeLog for LayoutTests included?

> LayoutTests/svg/custom/pointer-events-on-svg-with-pointer.xhtml:6
> +<defs>

What's this change about? Needs explanation in the ChangeLog.

> Source/WebCore/ChangeLog:6
> +        Adding a new framework to support the rendering of SVG shapes.

I'd propose: Adding a new framework to support more efficient rendering of SVG shapes.

> Source/WebCore/ChangeLog:8
> +        The main logic takes place in RenderSVGShape class and every renderer will
> +        inherit from it. Since its code base is mostly moved from RenderSVGPath it's suitable to

Well this is not correct. RenderSVGEllipse/Rect etc will inherit from it, but not RenderSVGText etc. It's worded a bit unfortunate, please fix it :-)

> Source/WebCore/ChangeLog:10
> +

In general this ChangeLog should be more descriptive, this is a heavy change and thus deserves a good explaination.

> Source/WebCore/platform/graphics/FloatRect.h:149
> +    bool comprises(const FloatPoint&) const;

Interssting, never heard the term "comprises" before. The only difference to contains is < -> <= right?
Couldn't this be an enum passed to contains() named "ContainsMode"?

> Source/WebCore/rendering/RenderObject.h:359
> +    virtual bool isSVGLine() const { return false; }

Line? This is not yet in, is it?

> Source/WebCore/rendering/svg/RenderSVGPath.h:35
> +// FIXME: RenderSVGPath will be removed when all SVG shapes will be rendered via RenderSVGShape.
> +class RenderSVGPath : public RenderSVGShape {

Huh, really? I thought it would still stay. It would be confusing if RenderSVGShape would be the main renderer for <path>, and RenderSVGRect inheriting from RenderSVGShape for <rect>.
I think we should only let "leaf" classes be used for specific svg elements like <circle>, <rect>, <path>.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:49
> +    // Clear m_boundingBox.

Does this serve a comment? It's not really helpful, it doesn't state a reason.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> +    bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> +    bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> +    // Fallback to RenderSVGShape if rect has special features.
> +    if (hasRx || hasRy) {

I'd rather see the fallback code decision in a seperated function:
bool shouldFallbackToPathRendering().

Hm, I thought Dirk mentioned more corner cases, you're not handling them here?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:86
> +    if (isFillFallback() && !style()->svgStyle()->strokeDashArray().isEmpty())

Why that extra condition? If the strokeDashArray is empty and isFillFallback is true, shouldn't RenderSVGshape be asked for its stroke bbox??

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:98
> +    context->strokeRect(m_boundingBox, this->strokeWidth());

s/this->//

> Source/WebCore/rendering/svg/RenderSVGRect.h:46
> +    virtual bool isEmpty() const { return hasPath() ? RenderSVGShape::isEmpty() : m_boundingBox.isEmpty(); };

Here hasPath() is used not isFillFallback(), is that on purpose?
You should make this a multi-line function, and/or move in the cpp file.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:191
> +    if (/*path && */!(resourceMode & ApplyToTextMode)) {

You can remove the comment.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
> +    if (!(resourceMode & ApplyToTextMode)) {

We could add an early return here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:139
> +        if (!m_path.get())

The .get() is superfluous.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> +            RenderSVGShape::createShape();

This should be encapsulated in a descriptive named function, otherwhise we'll wonder why only these conditions are checked.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> +        return RenderSVGShape::shapeDependentStrokeContains(point);

You want to avoid calling the shapeDependent function from the derived class, thus that RenderSVGShape:: prefix here?
This is easy to misread.
The whole stuff should be commented more, hard to follow at the moment.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> +            filter->postApplyResource(static_cast<RenderSVGShape*>(object), paintInfo.context, ApplyToDefaultMode, 0);

That's not a good idea, to blindly cast here!
This is also used for eg. RenderSVGContainer or Text who call finish/preparerenderSVGContent.
How is this supposed to work?

[Renata Hodovan]

Created attachment 107330 [details]
Propsed patch

> > Source/WebCore/ChangeLog:6
> > +        Adding a new framework to support the rendering of SVG shapes.
> 
> I'd propose: Adding a new framework to support more efficient rendering of SVG shapes.
OK.

> > Source/WebCore/ChangeLog:8
> > +        The main logic takes place in RenderSVGShape class and every renderer will
> > +        inherit from it. Since its code base is mostly moved from RenderSVGPath it's suitable to
> 
> Well this is not correct. RenderSVGEllipse/Rect etc will inherit from it, but not RenderSVGText etc. It's worded a bit unfortunate, please fix it :-)
OK.

> > Source/WebCore/ChangeLog:10
> > +
> 
> In general this ChangeLog should be more descriptive, this is a heavy change and thus deserves a good explaination.
> 
> > Source/WebCore/platform/graphics/FloatRect.h:149
> > +    bool comprises(const FloatPoint&) const;
> 
> Interssting, never heard the term "comprises" before. The only difference to contains is < -> <= right?
> Couldn't this be an enum passed to contains() named "ContainsMode"?
I don't think so it would be a good idea. Contains() is used in many places but compraises() just twice. What about if I give it a better name, eg.: isInsideRect() ?

> > Source/WebCore/rendering/RenderObject.h:359
> > +    virtual bool isSVGLine() const { return false; }
> 
> Line? This is not yet in, is it?
Ooops, I've started to work on SVGLines and this snippet remained here. Deleted.

> > Source/WebCore/rendering/svg/RenderSVGPath.h:35
> > +// FIXME: RenderSVGPath will be removed when all SVG shapes will be rendered via RenderSVGShape.
> > +class RenderSVGPath : public RenderSVGShape {
> 
> Huh, really? I thought it would still stay. It would be confusing if RenderSVGShape would be the main renderer for <path>, and RenderSVGRect inheriting from RenderSVGShape for <rect>.
OK.

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:49
> > +    // Clear m_boundingBox.
> 
> Does this serve a comment? It's not really helpful, it doesn't state a reason.
OK.

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> > +    bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> > +    bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> > +    // Fallback to RenderSVGShape if rect has special features.
> > +    if (hasRx || hasRy) {
> 
> I'd rather see the fallback code decision in a seperated function:
> bool shouldFallbackToPathRendering().
This function would be used from only this point. I could add an inline function what got two boolean and made a decison according to them. It seems useless, so I left out this modification now, but the comment above is updated.

> Hm, I thought Dirk mentioned more corner cases, you're not handling them here?
There are handled in different places. We talked about it before. We distinguish two different fallback cases. One for painting and an other for pointer-events. Painting fallback is needed just by rounded corners and pointer events by rounded rects and special strokes.

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:86
> > +    if (isFillFallback() && !style()->svgStyle()->strokeDashArray().isEmpty())
> 
> Why that extra condition? If the strokeDashArray is empty and isFillFallback is true, shouldn't RenderSVGshape be asked for its stroke bbox??
StrokeBoundingBox() is called by handling pointer events and as I mentioned in my previous comment we need to fallback both of rounded corners and special strokes.
 
> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:98
> > +    context->strokeRect(m_boundingBox, this->strokeWidth());
> 
> s/this->//
Ok.

> > Source/WebCore/rendering/svg/RenderSVGRect.h:46
> > +    virtual bool isEmpty() const { return hasPath() ? RenderSVGShape::isEmpty() : m_boundingBox.isEmpty(); };
> 
> Here hasPath() is used not isFillFallback(), is that on purpose?
> You should make this a multi-line function, and/or move in the cpp file.
I have to refer again to the two different fallback cases. It's possible that we don't have fallback by painting, because we don't have rounded rect. At the same time we have dashed stroke with pointer event, so m_path is filled.

> > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:191
> > +    if (/*path && */!(resourceMode & ApplyToTextMode)) {
> 
> You can remove the comment.
OK.

> > Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
> > +    if (!(resourceMode & ApplyToTextMode)) {
> 
> We could add an early return here.
OK. 

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:139
> > +        if (!m_path.get())
> 
> The .get() is superfluous.
OK.

> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> > +            RenderSVGShape::createShape();
> 
> This should be encapsulated in a descriptive named function, otherwhise we'll wonder why only these conditions are checked.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> > +        return RenderSVGShape::shapeDependentStrokeContains(point);
> 
> You want to avoid calling the shapeDependent function from the derived class, thus that RenderSVGShape:: prefix here?
Exactly! In this case I want to be sure that the ancestor is called because the derived class isn't able to handle the problem (we need fallback). The same situation like by your previous comment.

> This is easy to misread.
> The whole stuff should be commented more, hard to follow at the moment.
> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> > +            filter->postApplyResource(static_cast<RenderSVGShape*>(object), paintInfo.context, ApplyToDefaultMode, 0);
> 
> That's not a good idea, to blindly cast here!
> This is also used for eg. RenderSVGContainer or Text who call finish/preparerenderSVGContent.
> How is this supposed to work?
You are right. It wasn't common enough. It's updated.

[Dirk Schulze]

Comment on attachment 107330 [details]
Propsed patch

You did not address klieg's comments - again! https://bugs.webkit.org/show_bug.cgi?id=65769#c46

[Dirk Schulze]

Comment on attachment 107330 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107330&action=review

Maybe Niko has more comments to RenderTreeAsText. r- because of missing changes of kling and some more comments. The code looks great in general.

> Source/WebCore/ChangeLog:58
> +        (WebCore::RenderSVGRect::shapeDependentStrokeContains):
> +        (WebCore::RenderSVGRect::shapeDependentFillContains):

Niko is right. A line by line comment what the new functions do is not a bad idea.

> Source/WebCore/platform/graphics/FloatRect.cpp:76
> +bool FloatRect::isInsideRect(const FloatPoint& point) const
> +{
> +    if (!contains(point))
> +        return false;
> +    return x() < point.x() && maxX() > point.x() && y() < point.y() && maxY() > y();
> +}

Niko mentioned to add a new argument to contains(const FloatPoint& point, Enum ...). The default value could be the normal contains. (Even if I'm not sure if that is really needed.)

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> +    ASSERT(rect);
> +    bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> +    bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> +    // Fallback to RenderSVGShape if rect has rounded corners.

Please add a new line before comments. Makes it easier to read the code. Please consider to write 
if (rect->hasAttribute(SVGNames::rxAttr) || rect->hasAttribute(SVGNames::ryAttr)) instead like mentioned by kling.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:68
> +    m_boundingBox = FloatRect(FloatPoint(rect->x().value(rect), rect->y().value(rect)), boundingBoxSize);
> +    // To decide if the stroke contains a point we create two rects which represent the inner and

ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:273
> +            else if (shape)
> +                shape->isPaintingFallback() ? context->fillPath(shape->path()) : shape->fillShape(context);
> +        } else if (resourceMode & ApplyToStrokeMode) {
> +            if (path)
> +                context->strokePath(*path);
> +            else if (shape)
> +                shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);

The code style rules mention not to use 'else if' is it needed here? can we just use if here?

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:200
> +        if (path)
>              context->strokePath(*path);
> +        else if (shape)
> +            shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:92
> +    if (resourceMode & ApplyToFillMode) {
> +        if (path)
>              context->fillPath(*path);
> -        else if (resourceMode & ApplyToStrokeMode)
> +        else if (shape)
> +            shape->isPaintingFallback() ? context->fillPath(shape->path()) : shape->fillShape(context);
> +    } else if (resourceMode & ApplyToStrokeMode) {
> +        if (path)
>              context->strokePath(*path);
> +        else if (shape)
> +            shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:175
> +        setisPaintingFallback(false);

setIsPaintingFallback, is with big I.

> Source/WebCore/rendering/svg/RenderSVGShape.h:80
> +        ASSERT(m_path.get());

Can the .get() removed here?

[Renata Hodovan]

Created attachment 107331 [details]
Propsed patch

(In reply to comment #54)
> (From update of attachment 107330 [details])
> You did not address klieg's comments - again! https://bugs.webkit.org/show_bug.cgi?id=65769#c46
To tell the truth, yes :$ Sorry, I forgot it. Updated.

[Renata Hodovan]

(In reply to comment #55)
> (From update of attachment 107330 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107330&action=review
> 
> Maybe Niko has more comments to RenderTreeAsText. r- because of missing changes of kling and some more comments. The code looks great in general.
> 
> > Source/WebCore/ChangeLog:58
> > +        (WebCore::RenderSVGRect::shapeDependentStrokeContains):
> > +        (WebCore::RenderSVGRect::shapeDependentFillContains):
> 
> Niko is right. A line by line comment what the new functions do is not a bad idea.
> 
> > Source/WebCore/platform/graphics/FloatRect.cpp:76
> > +bool FloatRect::isInsideRect(const FloatPoint& point) const
> > +{
> > +    if (!contains(point))
> > +        return false;
> > +    return x() < point.x() && maxX() > point.x() && y() < point.y() && maxY() > y();
> > +}
> 
> Niko mentioned to add a new argument to contains(const FloatPoint& point, Enum ...). The default value could be the normal contains. (Even if I'm not sure if that is really needed.)
> 
> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> > +    ASSERT(rect);
> > +    bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> > +    bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> > +    // Fallback to RenderSVGShape if rect has rounded corners.
> 
> Please add a new line before comments. Makes it easier to read the code. Please consider to write 
> if (rect->hasAttribute(SVGNames::rxAttr) || rect->hasAttribute(SVGNames::ryAttr)) instead like mentioned by kling.
> 
> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:68
> > +    m_boundingBox = FloatRect(FloatPoint(rect->x().value(rect), rect->y().value(rect)), boundingBoxSize);
> > +    // To decide if the stroke contains a point we create two rects which represent the inner and
> 
> ditto.
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:273
> > +            else if (shape)
> > +                shape->isPaintingFallback() ? context->fillPath(shape->path()) : shape->fillShape(context);
> > +        } else if (resourceMode & ApplyToStrokeMode) {
> > +            if (path)
> > +                context->strokePath(*path);
> > +            else if (shape)
> > +                shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);
> 
> The code style rules mention not to use 'else if' is it needed here? can we just use if here?
> 
> > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:200
> > +        if (path)
> >              context->strokePath(*path);
> > +        else if (shape)
> > +            shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);
> 
> Ditto.
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:92
> > +    if (resourceMode & ApplyToFillMode) {
> > +        if (path)
> >              context->fillPath(*path);
> > -        else if (resourceMode & ApplyToStrokeMode)
> > +        else if (shape)
> > +            shape->isPaintingFallback() ? context->fillPath(shape->path()) : shape->fillShape(context);
> > +    } else if (resourceMode & ApplyToStrokeMode) {
> > +        if (path)
> >              context->strokePath(*path);
> > +        else if (shape)
> > +            shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);
> 
> Ditto.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:175
> > +        setisPaintingFallback(false);
> 
> setIsPaintingFallback, is with big I.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.h:80
> > +        ASSERT(m_path.get());
> 
> Can the .get() removed here?
Oohh, I've just read this second comment, but have to run. I'll update it tonight or tomorrow :)

[Renata Hodovan]

Created attachment 107500 [details]
Propsed patch

Style bot will be red because of the line:
bool contains(const FloatPoint&, ContainsMode containsMode = ContainsRect) const;
containsMode should be removed, but it would be really ugly solution for me...

[WebKit Review Bot]

Attachment 107500 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1

Source/WebCore/platform/graphics/FloatRect.h:154:  The parameter name "containsMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to comment #58)
> Style bot will be red because of the line:
> bool contains(const FloatPoint&, ContainsMode containsMode = ContainsRect) const;
> containsMode should be removed, but it would be really ugly solution for me...

I suppose it’s a matter of taste. I like it much better without containsMode.

    bool contains(const FloatPoint&, ContainsMode = ContainsRect) const;

Maybe it’s that you’re not used to code that looks like that and I am. We definitely have lines like that in many places in WebKit.

[Renata Hodovan]

Created attachment 107524 [details]
Proposed patch

Fix style error and FloatRect::contains() function.

[Dirk Schulze]

Comment on attachment 107500 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107500&action=review

Looks great. I just found some snippets.

> Source/WebCore/ChangeLog:47
> +        (WebCore::RenderSVGPath::inflateWithMarkerBounds):Unite the markerBounds with strokeBoundingBox.

space after ':'

> Source/WebCore/ChangeLog:64
> +        (WebCore::RenderSVGResource::postApplyResource):A new shape argument was added to allow shape specific faster painting.

ditto.

>> Source/WebCore/platform/graphics/FloatRect.h:154
>> +    bool contains(const FloatPoint&, ContainsMode containsMode = ContainsRect) const;
> 
> The parameter name "containsMode" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree to reni. ContainsMode = ContainsRect looks strange to me as well. On the other hand: it is no code duplication.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:268
> +            else if (shape)
> +                shape->isPaintingFallback() ? context->fillPath(shape->path()) : shape->fillShape(context);

Can't shape->fillShape(context) look itself if it is using the fallback? Someone reading the resources code might not understand the sense of this. 
Same for the other resources.

When else do we get a path that is not a shape here? For texts? I think so. Hm.. don't see a way to avoid passing RenderSVGShape*. Still the checkup should be done in the renderer.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:274
> +            if (path)
> +                context->strokePath(*path);
> +            else if (shape)
> +                shape->isPaintingFallback() ? context->strokePath(shape->path()) : shape->strokeShape(context);

Ditto.

Marking it for r- because of shape->fillShape for now. If it should not be possible to change that, write a comment. Great work!!

[Dirk Schulze]

Comment on attachment 107524 [details]
Proposed patch

See comment before.

[Renata Hodovan]

Created attachment 107681 [details]
Propsed patch

[Renata Hodovan]

The WIN bot is red because I missed to set back the ASSERT's condition in RenderSVGResourceFilter::postApplyResource() from shape to object. It didn't turn out in release build, sorry. Should I upload it again?

[Andreas Kling]

Comment on attachment 107681 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107681&action=review

> Source/WebCore/platform/graphics/FloatRect.h:154
> +    bool contains(const FloatPoint&, ContainsMode = ContainsRect) const;

I am not a fan of this API for three reasons:

1) We're out-of-lining FloatRect::contains() which may have performance implications. Do we know it doesn't? Could we keep it inline?
2) The enum values in ContainsMode (ContainsRect and InsideRect) are bad. What is the difference between ContainsRect and InsideRect? The names of the modes should make it perfectly clear what they do, I shouldn't have to look inside FloatRect.cpp for this information.
3) I don't see the InsideRect mode used anywhere!

> Source/WebCore/rendering/svg/RenderSVGPath.h:36
> -    explicit RenderSVGPath(SVGStyledTransformableElement*);
> +    RenderSVGPath(SVGStyledTransformableElement*);

Why no longer explicit?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:112
> +    return !m_innerStrokeRect.contains(point, FloatRect::ContainsRect);

Since ContainsRect is the default value of the 2nd parameter, you can (and should) omit it here.

[Renata Hodovan]

Created attachment 109287 [details]
Propsed patch

Calculating of bounding boxes are updated and a bug is filed about the shadow drawing of fillRect() function (https://bugs.webkit.org/show_bug.cgi?id=68899). It affects 3 tests. I plan to skip them until the problem will be solved.

[Dirk Schulze]

Comment on attachment 109287 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109287&action=review

The patch looks great in general. Please make sure that the commented out code doesn't cause regressions and remove it before landing!

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:740
> +

unnecessary change.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:57
> +    if (rect->hasAttribute(SVGNames::rxAttr) || rect->hasAttribute(SVGNames::ryAttr) || nonScalingStroke/* || static_cast<SVGElement*>(node())->customStyleForRenderer()->svgStyle()->shadow()*/) {

I don't understand the commented out code. Please add a fix me what you expect in the future and remove that code. Add a ling to a bug report as well please. I hope this won't cause regressions? If so you can't land this patch.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:428
> +        m_strokeAndMarkerBoundingBox.unite(/*outerStrokeBoundingBox()*/strokeBoundingBox());

Do not land dead code. Are you sure strokeBoundingBox() is doing the same like outerStrokeBoundingBox()? I assume that the original code was added, because strokBoundingBox() might not give the correct result at this point? Can you clarify this first please?

> Source/WebCore/rendering/svg/RenderSVGShape.h:100
> +    /*virtual FloatRect outerStrokeBoundingBox() const
> +    {
> +        BoundingRectStrokeStyleApplier strokeStyle(this, style());
> +        return m_path->strokeBoundingRect(&strokeStyle);
> +    }*/

Why is this commented out? Dead code shouldn't land. Remove it please.

[Nikolas Zimmermann]

Comment on attachment 109287 [details]
Propsed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109287&action=review

Okay, I'm revoking Dirks r+. I want to see 0 failures from cr-linux pixel test EWS first. This patch is too important.
I also have some more comments, and like to see a final patch w/o any commented code etc.

> Source/WebCore/rendering/svg/RenderSVGRect.h:38
> +    explicit RenderSVGRect(SVGStyledTransformableElement*);

Shouldn't this be SVGRectElement* ?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-103
> -

Unncessary change.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-271
> -

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-296
> -    

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:268
> +            if (path)
> +                context->fillPath(*path);
> +            else if (shape)
> +                shape->fillShape(context);

I guess at some point, we want to stop passing const Path* path to postApplyResource completly and only pass/use the RenderSVGShape object right?
This currently looks a bit weird. Maybe a FIXME would clarify it.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:187
> +void RenderSVGResourcePattern::postApplyResource(RenderObject*, GraphicsContext*& context, unsigned short resourceMode, const Path* path, const RenderSVGShape* shape)

Same here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:66
> +     m_path = adoptPtr(path);

Can't this be written as , m_path(adopPtr(path)) ?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:75
> +    m_path = adoptPtr(new Path);

Can m_path already be set here? Can we assert m_path is null before

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:147
> +        if (!m_path)
> +            RenderSVGShape::createShape();

Aha, so I think we can safely ASSERT(!m_path) in createShape, right?

> Source/WebCore/rendering/svg/RenderSVGShape.h:55
> +    void strokeStyle(GraphicsContext* gc)

s/gc/context/

[Renata Hodovan]

Created attachment 109781 [details]
Propsed patch

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:268
> +            if (path)
> +                context->fillPath(*path);
> +            else if (shape)
> +                shape->fillShape(context);

I guess at some point, we want to stop passing const Path* path to postApplyResource completly and only pass/use the RenderSVGShape object right?
This currently looks a bit weird. Maybe a FIXME would clarify it.

We need to pass const Path* to postApplyResource() because it's called from SVGTextRunRenderingContext::drawSVGGlyphs() as well which is not inherited from RenderSVGShape(). So it needs to get the path value from the parameter list.

@krit: Sorry about the dead codes, I missed to delete them after the usage of the different bounding boxes was clarified. I runned layout tests with this version, so deleting the comments won't cause any regressions (btw I tested it again).

All the other propsals are fulfilled.

[Renata Hodovan]

I don't understand what's up with chromium ews.. these failures seem unrelated to my patch. Help me pls! O_o

[Adam Barth]

Not sure.  Something about your patch is confusing the EWS.  I wouldn't worry too much about it.

[Adam Barth]

Comment on attachment 109781 [details]
Propsed patch

I bet this patch causes a massive number of image test to fail, which is what's causing the problem.  I'm R-ing this patch to let the bot get to the purple state so that it's not burning up tons of server resources.  Please feel free to re-nominate for review after the bot goes purple.

[Renata Hodovan]

(In reply to comment #73)
> (From update of attachment 109781 [details])
> I bet this patch causes a massive number of image test to fail, which is what's causing the problem.  I'm R-ing this patch to let the bot get to the purple state so that it's not burning up tons of server resources.  Please feel free to re-nominate for review after the bot goes purple.

I tried to manage a chromium build by myself, but I faced a build error. It seems chromium doesn't see the newly added files (RenderSVGRect and RenderSVGShape). Tracking the build log I realized they aren't in the compile queue. An observation: WebCore/WebCore.gyp/webcore_svg.target.chromium.mk contains RenderSVGPath.o but it doesn't have the new files objects. Any idea what could be the problem?

[Renata Hodovan]

> I tried to manage a chromium build by myself, but I faced a build error. It seems chromium doesn't see the newly added files (RenderSVGRect and RenderSVGShape). Tracking the build log I realized they aren't in the compile queue. An observation: WebCore/WebCore.gyp/webcore_svg.target.chromium.mk contains RenderSVGPath.o but it doesn't have the new files objects. Any idea what could be the problem?

I've found an interesting thing... there is a line in Source/WebCore/gyp/WebCore.gyp:

['exclude', 'rendering/svg/[^/]+\\.cpp']

After changing "exclude" to "include" the build was successfull. It's a hack ofc and I'm not sure how it worked. Any idea?

[Renata Hodovan]

Created attachment 111444 [details]
Proposed patch

Shadow related MAC problem is solved. The problem was css shadows (--webkit-svg-shadow) should be applied via transparency layer and not via the shape's renderer. It's turned off now and the tests run fine. (Comment is also added to this part.)

[Dirk Schulze]

(In reply to comment #76)
> Created an attachment (id=111444) [details]
> Proposed patch
> 
> Shadow related MAC problem is solved. The problem was css shadows (--webkit-svg-shadow) should be applied via transparency layer and not via the shape's renderer. It's turned off now and the tests run fine. (Comment is also added to this part.)

That sounds great! Did you fix the masking/clipping problem on chromium as well?

[Dirk Schulze]

Comment on attachment 111444 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111444&action=review

Still great patch ;) I just have some questions to your new added code.

> Source/WebCore/ChangeLog:23
> +        This patch introduces a new common base class called RenderSVGShape which
> +        replaces the RenderSVGPath. This new base class has the same purpose
> +        as the replaced class and has specialized descendants for common
> +        shapes (like Rectangles and Circles), which allows faster painting
> +        of these shapes when certain conditions are fulfilled. On some
> +        benchmark programs we have seen 5% speedup.
> +
> +        The biggest motivation of this refactor is taking advantage
> +        of faster primitive drawing in the most common and frequent
> +        cases. However in some rare cases, like painting rounded
> +        rects, we need to fallback to the original code path, which
> +        is fully kept in the RenderSVGShape base class. Some other
> +        cases, like dashed strokes, can be painted but mouse pointer
> +        events cannot be handled by the descendant classes. A different
> +        fallback mechanism is used in such cases which redirects
> +        only the pointer event handling to the base class.
> +
> +        Reviewed by NOBODY (OOPS!).

Unsure right now, but shouldn't the review comment be right after the bug title and link? :P

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:3
> + * Copyright (C) 2011 Renata Hodovan ([e-mail])

Wrong braces.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:98
> +        // --webkit-svg-shadow should be applied via transparency layer,

Just -webkit-svg-shadow.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:106
> +        if (context->hasShadow()) {
> +            context->save();
> +            context->clearShadow();
> +            context->fillRect(m_boundingBox);
> +            context->restore();
> +            return;
> +        }

If I understand Simon correctly, you should use GraphicsContextStateSaver here. But maybe you can read the shadow data, clear it on the context and write it back afterwards? Wouldn't that be more performant?
 
> Source/WebCore/rendering/svg/RenderSVGRect.cpp:116
> +        context->strokeRect(m_boundingBox, strokeWidth());

Don't we have the same problem with -webkit-svg-shadow here?

[Zoltan Herczeg]

> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:116
> > +        context->strokeRect(m_boundingBox, strokeWidth());
> 
> Don't we have the same problem with -webkit-svg-shadow here?

Hey Krit! Actuaully the logic is flawed here. In other words, there is an ugly hack here. Would be a good idea to fix it someday.

According to the GraphicsContext object of WebKit the shadow is enabled, but the higher level Mac CG object knows the other way, its shadow property is disabled. So you actually draw a shadowed object without shadow >:]

That happens with lines.

However, the FillRect has a custom shadow drawing method with ShadowBlur. It detects whether the shadow must be drawn depending on the GraphicsContext, not the Mac specific CG object, so it thinks a shadow must be drawn here! (By the way, is it possible to query the shadow status from the CG object? We checked the help of XCode and we found many setters, but no getters. It seems pretty lame to me.)

[Dirk Schulze]

(In reply to comment #79)
> > > Source/WebCore/rendering/svg/RenderSVGRect.cpp:116
> > > +        context->strokeRect(m_boundingBox, strokeWidth());
> > 
> > Don't we have the same problem with -webkit-svg-shadow here?
> 
> Hey Krit! Actuaully the logic is flawed here. In other words, there is an ugly hack here. Would be a good idea to fix it someday.
> 
> According to the GraphicsContext object of WebKit the shadow is enabled, but the higher level Mac CG object knows the other way, its shadow property is disabled. So you actually draw a shadowed object without shadow >:]
> 
> That happens with lines.
> 
> However, the FillRect has a custom shadow drawing method with ShadowBlur. It detects whether the shadow must be drawn depending on the GraphicsContext, not the Mac specific CG object, so it thinks a shadow must be drawn here! (By the way, is it possible to query the shadow status from the CG object? We checked the help of XCode and we found many setters, but no getters. It seems pretty lame to me.)

Hm, I'm not a fan of hacks. Now that you know what happens, how difficult is it to fix it before landing this bug?

Anyway, if we really need this hack and it is Mac specific, surround it with ifdefs, add a FIXME and point to the bug report. Please also add a note to the bug report itself after landing. I'd still use GraphicsContextStateSaver for consistency.

[Renata Hodovan]

Created attachment 111604 [details]
Proposed patch

> Hm, I'm not a fan of hacks. Now that you know what happens, how difficult is it to fix it before landing this bug?
It's a bigger task, so it should be solved in a follow-up patch.

The other suggestions of you are fulfilled.

[WebKit Review Bot]

Attachment 111604 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 111604 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111604&action=review

Great patch reni! r=me.

> Source/WebCore/rendering/svg/RenderSVGRect.h:3
> + * Copyright (C) 2011 Renata Hodovan ([e-mail])

Braces still wrong.

[Renata Hodovan]

Committed r97863: <http://trac.webkit.org/changeset/97863>

[Dimitri Glazkov (Google)]

(In reply to comment #84)
> Committed r97863: <http://trac.webkit.org/changeset/97863>

This patch is causing ASSERTs in Chromium:

	0x2aaee8766af0
	WebCore::RenderSVGShape::createShape() [0x19c7d0f]
	WebCore::RenderSVGShape::layout() [0x19c84d5]
	WebCore::SVGRenderSupport::layoutChildren() [0x18b4e45]
	WebCore::RenderSVGRoot::layout() [0x19c5b9f]
	WebCore::RenderBlock::layoutBlockChild() [0x14fbe64]
	WebCore::RenderBlock::layoutBlockChildren() [0x14fba86]
	WebCore::RenderBlock::layoutBlock() [0x14f88ae]
	WebCore::RenderBlock::layout() [0x14f80aa]
	WebCore::RenderView::layout() [0x162f0e6]
	WebCore::FrameView::layout() [0x127a22d]
	WebCore::Document::implicitClose() [0xc3bae3]
	WebCore::FrameLoader::checkCallImplicitClose() [0x11cf68f]
	WebCore::FrameLoader::checkCompleted() [0x11cf462]
	WebCore::FrameLoader::finishedParsing() [0x11cf1bb]
	WebCore::Document::finishedParsing() [0xc44422]
	WebCore::XMLDocumentParser::end() [0x1346304]
	WebCore::XMLDocumentParser::finish() [0x134633e]
	WebCore::DocumentWriter::endIfNotLoadingMainResource() [0x11c51ef]
	WebCore::DocumentWriter::end() [0x11c50f7]
	WebCore::DocumentLoader::finishedLoading() [0x11b6585]
	WebCore::FrameLoader::finishedLoading() [0x11d5f1b]
	WebCore::MainResourceLoader::didFinishLoading() [0x11eadf1]
	WebCore::ResourceLoader::didFinishLoading() [0x11fdbd9]
	WebCore::ResourceHandleInternal::didFinishLoading() [0x4faa14]
	webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest() [0x1a47c54]
	(anonymous namespace)::RequestProxy::NotifyCompletedRequest() [0x1b04559]
	base::internal::Invoker4<>::DoInvoke() [0x1b09a6a]
	base::Callback<>::Run() [0x67d9b3]
	MessageLoop::RunTask() [0x698986]
	MessageLoop::DeferOrRunPendingTask() [0x698ab1]
	MessageLoop::DoWork() [0x6992c7]
	base::MessagePumpGlib::HandleDispatch() [0x6f2715]
	(anonymous namespace)::WorkSourceDispatch() [0x6f1c67]
	0x2aaee2ffe8c2
	0x2aaee3002748
	0x2aaee30028fc
	base::MessagePumpGtk::RunOnce() [0x6f3f7d]
	base::MessagePumpGlib::RunWithDispatcher() [0x6f23c8]
	base::MessagePumpGlib::Run() [0x6f27f2]
	MessageLoop::RunInternal() [0x698611]
	MessageLoop::RunHandler() [0x6984c4]
	MessageLoop::Run() [0x697e67]
	webkit_support::RunMessageLoop() [0x60f2af]
	TestShell::waitTestFinished() [0x463e89]
	TestShell::runFileTest() [0x45cbce]
	runTest() [0x430fc8]
	main [0x431a20]
	0x2aaee8751c4d
	0x41df09

I bet you'll see it in other ports too.

[Dirk Schulze]

Reverted r97863 for reason:

Rollout

Committed r97870: <http://trac.webkit.org/changeset/97870>

[Renata Hodovan]

Created attachment 112678 [details]
Proposed patch

The assertions in the previous version was caused by some missing clearing method in fallback cases. They are added already, so we really don't have any regressions neither in release nor in debug version.

As we discussed on IRC, RenderSVGRect lies about itself being a "RenderSVGPath" and this way we don't need layouttest updates (because DRT produces the same output as before).

[WebKit Review Bot]

Comment on attachment 112678 [details]
Proposed patch

Attachment 112678 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10238161

New failing tests:
svg/W3C-SVG-1.1/animate-elem-64-t.svg
svg/W3C-SVG-1.1/animate-elem-63-t.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/W3C-SVG-1.1/animate-elem-60-t.svg
svg/W3C-SVG-1.1/animate-elem-23-t.svg
svg/W3C-SVG-1.1/animate-elem-40-t.svg
svg/W3C-SVG-1.1-SE/types-dom-05-b.svg
svg/W3C-SVG-1.1-SE/styling-pres-02-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-04-f.svg
svg/W3C-SVG-1.1/animate-elem-62-t.svg
svg/W3C-SVG-1.1/animate-elem-61-t.svg
svg/W3C-SVG-1.1/animate-elem-30-t.svg
svg/W3C-SVG-1.1-SE/styling-css-04-f.svg
svg/W3C-SVG-1.1/animate-elem-46-t.svg
svg/W3C-SVG-1.1/animate-elem-41-t.svg

[Renata Hodovan]

Created attachment 115805 [details]
Proposed patch

This new version contains the following:
 * Adapting it to the relevant modifications what are commited from the previous version (e.g.: the removed toPathData(), etc.)
 * Few refactoring, mainly related to the calculating of the boundingboxes.
 * Merge inflateWithMarkerBounds() and stroke plumping code snippet into one common inflateWithStrokeandMarkerBounds() function.
 * The previous version contained a solution for avoiding the shadow painting problem on mac. Since Chromium uses the same painting method like mac we needed to extend the #if PLATFORM(MAC) condition with (PLATFORM(CHROMIUM) && OS(MAC_OS_X)).
 * Pixel expecteds of the following tests are updated:
       * svg/W3C-SVG-1.1/coords-trans-01-b.svg
       * svg/custom/percentage-rect.svg
   Their results were one pixel slimmer than it was expected. The problematic rects had transformations and its sizes were defined in percentages.
   I dumped a mass of datas and got the following: all the bounding boxes and transformations paramters remained the same like in the original RenderSVGPath's implementation. Additionally, I tried to use fillPath() in my patch instead of fillRect() and the tests were succeeded again. That's in RenderSVGRect::fillShape() :

       context->fillRect(m_boundingBox);

       was changed to:

       Path tmp;
       tmp.addRect(m_boundingBox);
       context->fillPath(tmp);

    I suppose it means that just the difference of precision of the two methods was resposible for the fail. Any other idea?

[WebKit Review Bot]

Comment on attachment 115805 [details]
Proposed patch

Attachment 115805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10393157

[Gyuyoung Kim]

Comment on attachment 115805 [details]
Proposed patch

Attachment 115805 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10495123

[Renata Hodovan]

Created attachment 115816 [details]
Proposed patch

Sorry for the previous. After an alphabetical sorting style error one include was accidentally deleted.

[Renata Hodovan]

I have some dummy quesitons...
I checked the dump of ews and it was not really clear to me what i saw.
E.g.: here is one of the result pages created by mac ews:
http://queues.webkit.org/results/10559080
what contains a table:

Found:  27268 tests
Expect: 17002 passes   (17002 now,    0 wontfix)
Expect:   788 failures (  760 now,   28 wontfix)
Expect:  1305 flaky    ( 1150 now,  155 wontfix)
Expect:  8173 skipped  (  403 now, 7770 wontfix)

The first line told me that we expected 17002 pass what we also got according to the second column. Am I wrong? If not then why is ews still yellow and why are running tests again and again?

The second question is: why does start the mac ews dump with:
"Failed to run "['/mnt/git/webkit-chromium-ews/Tools/Scripts/webkit-patch'..."
The *webkit-chromium-ews* part makes me confused... 

I would be happy to fix all regressions at last but unless knowing the problems it's pretty hard :(
Any comment would be welcomed!

[WebKit Review Bot]

Comment on attachment 115816 [details]
Proposed patch

Attachment 115816 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10607295

New failing tests:
css2.1/20110323/abspos-containing-block-initial-004b.htm
fast/replaced/width100percent-textarea.html
css2.1/20110323/background-intrinsic-004.htm
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/W3C-SVG-1.1/animate-elem-60-t.svg
svg/W3C-SVG-1.1/animate-elem-23-t.svg
css2.1/20110323/background-intrinsic-005.htm
svg/W3C-SVG-1.1-SE/styling-pres-02-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-04-f.svg
svg/W3C-SVG-1.1/animate-elem-30-t.svg
svg/W3C-SVG-1.1-SE/styling-css-04-f.svg
svg/W3C-SVG-1.1/animate-elem-40-t.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
svg/W3C-SVG-1.1/animate-elem-46-t.svg
svg/W3C-SVG-1.1/animate-elem-41-t.svg

[Nikolas Zimmermann]

Comment on attachment 115816 [details]
Proposed patch

Looks good to me, but I fear it now needs an update due the recent changes in ToT, still r=me.
Please delay landing until the gtk/qt/win/mac rebaselines are done, that would be great.

[Vincent Scheib]

This patch caused a pretty large workload for chromium rebaselines, and caused some disruption due to the large number of test failures making it hard to see what other tests were having issues. 

The early warning system was giving indication that this would happen. It would have been good to address before landing. Probably by adjusting test_expectations to suppress failures until new images were generated on bots.

I see that Renata asked about this, and can suggest that the question should have been repeated in #webkit until someone came up with an answer. ;)

[Renata Hodovan]

(In reply to comment #96)
> This patch caused a pretty large workload for chromium rebaselines, and caused some disruption due to the large number of test failures making it hard to see what other tests were having issues. 
> 
> The early warning system was giving indication that this would happen. It would have been good to address before landing. Probably by adjusting test_expectations to suppress failures until new images were generated on bots.
> 
> I see that Renata asked about this, and can suggest that the question should have been repeated in #webkit until someone came up with an answer. ;)

You are right. I will do this way next time. Thanks again for your help :)

[Renata Hodovan]

Patch is landed in <http://trac.webkit.org/changeset/101517>

[Nikolas Zimmermann]

(In reply to comment #96)
> This patch caused a pretty large workload for chromium rebaselines, and caused some disruption due to the large number of test failures making it hard to see what other tests were having issues. 

Are you sure about that? Renatas patch should only have caused in the order of 20 changes for you.
My patch impacted chromium much more requiring 500+ rebaselines. My patch was given r+ last Thursday, and I waited until after the thanksgiving weekend on Tuesday to land it, and I discussed it before with zmo, the WebKit gardener at the team, who said I should go ahead and land it.
He didn't finish the SVG rebaselines, as he told me on IRC, as he had to leave.

Anyhow, I don't think Renatas patch caused problems, but rather mine.

[Renata Hodovan]

Committed r103407: <http://trac.webkit.org/changeset/103407>