Bug 113849

Created attachment 198021 [details] proposed patch

Created attachment 209418 [details] patch

Comment on attachment 209418 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=209418&action=review > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:157 > + FloatPoint point1 = FloatPoint(x + 2.0 / 3.0 * (x1 - x), y + 2.0 / 3.0 * (y1 - y)); FloatPoint point1(x + 2.0 / 3.0 * (x1 - x), y + 2.0 / 3.0 * (y1 - y)); > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:158 > + FloatPoint point2 = FloatPoint(x2 + 2.0 / 3.0 * (x1 - x2), y2 + 2.0 / 3.0 * (y1 - y2)); Ditto. > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:159 > + FloatPoint point3 = FloatPoint(x2, y2); Ditto. > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:365 > + for (size_t i = 0; i < m_path->pathElements().size(); i++) ++i > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41 > + const Vector<OwnPtr<PlatformPathElement> >& elements = other.pathElements(); no need for the space between > >, C++11 is used. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:46 > +void CairoPath::appendPathElement(const PassOwnPtr<PlatformPathElement>& element) We don't usually pass PassOwnPtrs by const reference > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:55 > + m_pathElements.append(adoptPtr(new PlatformPathElementMoveToPoint(pathElement.points[0]))); By having static factory functions (called "create") in those classes returning a PassOwnPtr, you would not have to handle raw pointers from outside the class. This is a pattern commonly used in WebKit. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:74 > + for (size_t i = 0; i < m_pathElements.size(); i++) ++i > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:78 > +void CairoPath::transform(const AffineTransform& trans) no abbreviations in WebKit: "trans" > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:80 > + for (size_t i = 0; i < m_pathElements.size(); i++) ++i > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:35 > + virtual ~PlatformPathElement() { } Would be nice to have a constructor to initialize m_pathElement. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:37 > + const PathElement& pathElement() { return m_pathElement; } getter should be const. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:45 > +class PlatformPathElementMoveToPoint : public PlatformPathElement { The class can be FINAL if not subclassed. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:47 > + PlatformPathElementMoveToPoint(const FloatPoint& point) explicit would be nice > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:49 > + m_pathElementPoint = point; This initialization should be done in the initialiazer list. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:50 > + m_pathElement.type = PathElementMoveToPoint; m_pathElement is in the parent so I feel it would be cleaner to call the parent's constructor and let it do the init. e.g. PlatformPathElement(PathElementMoveToPoint, &m_pathElementPoint) > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:54 > + virtual void transform(const AffineTransform& trans) missing OVERRIDE no abbreviations. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:59 > + virtual void translate(const FloatSize& p) Missing OVERRIDE No abbreviations > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:68 > +class PlatformPathElementAddLineToPoint : public PlatformPathElement { The class can be FINAL if not subclassed. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:70 > + PlatformPathElementAddLineToPoint(const FloatPoint& point) explicit > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:72 > + m_pathElementPoint = point; initializer list > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:77 > + virtual void transform(const AffineTransform& trans) OVERRIDE + abbrev. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:82 > + virtual void translate(const FloatSize& p) OVERRIDE + abbrev. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:91 > +class PlatformPathElementAddCurveToPoint : public PlatformPathElement { The class can be FINAL if not subclassed. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:95 > + m_pathElementPoints[0] = point1; init list. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:98 > + m_pathElement.type = PathElementAddCurveToPoint; parent constructor > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:102 > + virtual void transform(const AffineTransform& trans) OVERRIDE + abbrev. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:109 > + virtual void translate(const FloatSize& p) OVERRIDE + abbrev. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:120 > +class PlatformPathElementCloseSubpath : public PlatformPathElement { The class can be FINAL if not subclassed. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:128 > + virtual void transform(const AffineTransform& trans) OVERRIDE + abbrev. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:133 > + virtual void translate(const FloatSize& p) OVERRIDE + abbrev. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:150 > + const Vector<OwnPtr<PlatformPathElement> >& pathElements() const { return m_pathElements; } No need for the extra space in C++11. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:152 > + void appendPathElement(const PassOwnPtr<PlatformPathElement>&); no const ref > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:160 > + Vector<OwnPtr<PlatformPathElement> > m_pathElements; No need for extra space

Created attachment 209444 [details] Patch

Attachment 209444 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1 Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165: Missing spaces around >> [whitespace/operators] [3] Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41: Missing spaces around >> [whitespace/operators] [3] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #5) > Attachment 209444 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1 > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165: Missing spaces around >> [whitespace/operators] [3] > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41: Missing spaces around >> [whitespace/operators] [3] > Total errors found: 2 in 7 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Looks like a style script bug.

Comment on attachment 209444 [details] Patch The patch looks clean technically and style wise. However, I don't know much about this area so I'll defer review to someone who does. Martin maybe? :)

Created attachment 209445 [details] Patch

(In reply to comment #6) > (In reply to comment #5) > > Attachment 209444 [details] [details] did not pass style-queue: > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1 > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165: Missing spaces around >> [whitespace/operators] [3] > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41: Missing spaces around >> [whitespace/operators] [3] > > Total errors found: 2 in 7 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > Looks like a style script bug. Sorry. I did not read this and uploaded patch again for fixing style error, but I'm not sure it is right.

(In reply to comment #9) > (In reply to comment #6) > > (In reply to comment #5) > > > Attachment 209444 [details] [details] [details] did not pass style-queue: > > > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1 > > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165: Missing spaces around >> [whitespace/operators] [3] > > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41: Missing spaces around >> [whitespace/operators] [3] > > > Total errors found: 2 in 7 files > > > > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > > > Looks like a style script bug. > Sorry. I did not read this and uploaded patch again for fixing style error, but I'm not sure it is right. It is a bug in the style script, it is confused by '>>' which is now allowed in C++11. I wouldn't change anything to make the style script happy. We should file a bug against the style script instead since I believe it was already announced on the mailing list that it was OK not to add the space anymore.

Created attachment 209457 [details] fixed patch

Attachment 209457 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1 Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165: Missing spaces around >> [whitespace/operators] [3] Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41: Missing spaces around >> [whitespace/operators] [3] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #6) > > > (In reply to comment #5) > > > > Attachment 209444 [details] [details] [details] [details] did not pass style-queue: > > > > > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.png', u'LayoutTests/platform/efl/svg/custom/circular-marker-reference-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp', u'Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h']" exit_code: 1 > > > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:165: Missing spaces around >> [whitespace/operators] [3] > > > > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.cpp:41: Missing spaces around >> [whitespace/operators] [3] > > > > Total errors found: 2 in 7 files > > > > > > > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > > > > > Looks like a style script bug. > > Sorry. I did not read this and uploaded patch again for fixing style error, but I'm not sure it is right. > > It is a bug in the style script, it is confused by '>>' which is now allowed in C++11. I wouldn't change anything to make the style script happy. We should file a bug against the style script instead since I believe it was already announced on the mailing list that it was OK not to add the space anymore. Thanks for your explanation. I uploaded previous patch again.

Comment on attachment 209457 [details] fixed patch Attachment 209457 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1555332 New failing tests: webaudio/javascriptaudionode.html

Created attachment 209507 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Reassigning some EFL bugs that are likely shared with GTK/WPE to the GTK component.

Created attachment 375220 [details] WIP patch Rebased onto ToT.

Created attachment 375221 [details] WIP patch

Comment on attachment 375221 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review Hey Fujii, I also started to look at this when you added me to the CC. I think this could be a lot cleaner if all the logig is moved to CairoPath instead. I see another big problem of this patch, cairo_arc calls are not saved at all in element paths, so they are missing in apply(). It's not easy to add, either, because cairo generates several line_to() command from them. Maybe this is ok if apply() is never called for paths containing arcs, but we need to ensure that's the case. I see other problems, I'll comment in the patch. > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56 > + *this = other; I don't think this is correct. CairoPath contains a RefPtr<cairo_context_t>, so we will end up with two Paths having the same cairo context. > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:140 > cairo_t* cr = ensurePlatformPath()->context(); > + m_path->translate(p); > cairo_translate(cr, -p.width(), -p.height()); As I said, here, and everywhere else, I think it would be cleaner if CairoPath also calls cairo_translate, since the context is there. This calls would become like a wrapper around the CairoPath one. > Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:155 > +}; I think a single class would be simpler, we just need a switch in transform and translate to decide whether to use 1, 2, or 3 points.

Comment on attachment 375221 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:238 > - cairo_line_to(cr, p1.x(), p1.y()); > + addLineTo(p1); I don't think it makes sense to register this line if we are not registering the other lines of the arc. > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:250 > - cairo_line_to(cr, p1.x(), p1.y()); > + addLineTo(p1); Ditto. > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:298 > - cairo_line_to(cr, t_p1p0.x(), t_p1p0.y()); > + addLineTo(t_p1p0); Ditto.

(In reply to Carlos Garcia Campos from comment #19) > Hey Fujii, I also started to look at this when you added me to the CC. Thank you for feedback, KaL! > I think this could be a lot cleaner if all the logig is moved to CairoPath > instead. I'm also thinking about the same idea. It's like a pimpl idiom. However, I'm hesitating to take the approach because 1. I need to a lot of logic from PathCairo.cpp to CairoPath. 2. I'm not confident the pimpl idiom is needed in this case

Comment on attachment 375221 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review >> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56 >> + *this = other; > > I don't think this is correct. CairoPath contains a RefPtr<cairo_context_t>, so we will end up with two Paths having the same cairo context. This calls Path::operator=. It does deep copy. >> Source/WebCore/platform/graphics/cairo/PlatformPathCairo.h:155 >> +}; > > I think a single class would be simpler, we just need a switch in transform and translate to decide whether to use 1, 2, or 3 points. Yeah, I think so.

Comment on attachment 375221 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=375221&action=review >>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56 >>> + *this = other; >> >> I don't think this is correct. CairoPath contains a RefPtr<cairo_context_t>, so we will end up with two Paths having the same cairo context. > > This calls Path::operator=. It does deep copy. Ah, right, this was duplicated indeed.

Created attachment 393240 [details] Updated patch

Comment on attachment 393240 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=393240&action=review > LayoutTests/platform/gtk/svg/W3C-SVG-1.1/painting-marker-03-f-expected.txt:18 > - RenderSVGContainer {g} at (57,77) size 366x86 [transform={m=((1.00,0.00)(0.00,1.00)) t=(50.00,20.00)}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] > - RenderSVGPath {path} at (57,77) size 86x86 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#FFD700] [fill rule=EVEN-ODD]}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] [data="M 10 60 C 63.3333 60 90 86.6667 90 140 C 36.6667 140 10 113.333 10 60 Z M 10 140 C 10 86.6667 36.6667 60 90 60 C 90 113.333 63.3333 140 10 140 Z M 50 70 L 80 100 L 50 130 L 20 100 Z"] > + RenderSVGContainer {g} at (47,17) size 376x146 [transform={m=((1.00,0.00)(0.00,1.00)) t=(50.00,20.00)}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] > + RenderSVGPath {path} at (47,17) size 96x146 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#FFD700] [fill rule=EVEN-ODD]}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] [data="M 10 60 C 63.3333 60 90 86.6667 90 140 C 36.6667 140 10 113.333 10 60 Z M 10 140 C 10 86.6667 36.6667 60 90 60 C 90 113.333 63.3333 140 10 140 Z M 50 70 L 80 100 L 50 130 L 20 100 Z"] > RenderSVGPath {polygon} at (147,77) size 86x86 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#FFD700] [fill rule=EVEN-ODD]}] [start marker=marker1] [middle marker=marker1] [end marker=marker1] [points="100 60 120 140 140 60 160 140 180 60 180 100 100 100"] Now that I see the pixel differences, it seems this tests is actually regressing. I'll investigate.

Created attachment 393366 [details] Patch

Created attachment 393368 [details] Patch

LGTM, but it seems that I'm not the right person to r+ (there is my name in the ChangeLog entry).

Committed r258492: <https://trac.webkit.org/changeset/258492>