Bug 108508

Summary: Enable CANVAS_PATH flag
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CanvasAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, benjamin, buildbot, cmarcelo, dbates, gyuyoung.kim, hausmann, kadam, laszlo.gombos, ojan.autocc, oliver, ossy, rakuco, rniwa, vestbo, webkit.review.bot, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 97333    
Attachments:
Description Flags
Test building
none
Second attempt
none
Patch
none
Patch
none
Patch hausmann: review+

Description Dirk Schulze 2013-01-31 11:12:54 PST
This is a placeholder bug for logging all tests that need to be unskipped after enabling the CANVAS_PATH flag.
Comment 1 Dirk Schulze 2013-02-19 21:21:47 PST
Created attachment 189232 [details]
Test building
Comment 2 Dirk Schulze 2013-02-19 21:50:47 PST
Created attachment 189236 [details]
Second attempt

Not for review.
Comment 3 Dirk Schulze 2013-02-19 22:32:08 PST
Created attachment 189245 [details]
Patch
Comment 4 Laszlo Gombos 2013-02-19 22:35:15 PST
Dirk, would you mind setting it to 1 as well in Source/WTF/wtf/FeatureDefines.h ?
Comment 5 Dirk Schulze 2013-02-19 22:38:12 PST
(In reply to comment #4)
> Dirk, would you mind setting it to 1 as well in Source/WTF/wtf/FeatureDefines.h ?

Yes of course.
Comment 6 Dirk Schulze 2013-02-19 22:39:34 PST
Created attachment 189247 [details]
Patch
Comment 7 Build Bot 2013-02-20 00:34:54 PST
Comment on attachment 189247 [details]
Patch

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

New failing tests:
fast/css/sticky/inline-sticky-abspos-child.html
canvas/philip/tests/2d.path.arc.scale.2.html
fast/canvas/canvas-composite-alpha.html
canvas/philip/tests/2d.path.stroke.skew.html
fast/canvas/webgl/read-pixels-test.html
fast/dom/constructed-objects-prototypes.html
canvas/philip/tests/2d.path.arcTo.scale.html
fast/css/sticky/sticky-both-sides.html
canvas/philip/tests/2d.path.arcTo.transformation.html
canvas/philip/tests/2d.path.stroke.scale1.html
fast/css/sticky/inline-sticky.html
Comment 8 Dirk Schulze 2013-02-20 08:23:38 PST
(In reply to comment #7)
> (From update of attachment 189247 [details])
> Attachment 189247 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16657131
> 
> New failing tests:
> fast/css/sticky/inline-sticky-abspos-child.html
> canvas/philip/tests/2d.path.arc.scale.2.html
> fast/canvas/canvas-composite-alpha.html
> canvas/philip/tests/2d.path.stroke.skew.html
> fast/canvas/webgl/read-pixels-test.html
> fast/dom/constructed-objects-prototypes.html
> canvas/philip/tests/2d.path.arcTo.scale.html
> fast/css/sticky/sticky-both-sides.html
> canvas/philip/tests/2d.path.arcTo.transformation.html
> canvas/philip/tests/2d.path.stroke.scale1.html
> fast/css/sticky/inline-sticky.html

I can not verify these problems. I run the tests multiple times on my mac build and all these tests passed every single time. I will watch the bots.
Comment 9 Dirk Schulze 2013-02-20 14:16:00 PST
Committed r143505: <http://trac.webkit.org/changeset/143505>
Comment 10 Csaba Osztrogonác 2013-02-20 16:01:49 PST
(In reply to comment #9)
> Committed r143505: <http://trac.webkit.org/changeset/143505>

It broke the Qt minimal build:
In file included from /ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:45:0:
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h: In static member function 'static WTF::PassRefPtr<WebCore::DOMPath> WebCore::DOMPath::create(const WTF::String&)':
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:45:101: error: no matching function for call to 'WebCore::DOMPath::DOMPath(const WTF::String&)'
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:45:101: note: candidates are:
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:60:5: note: WebCore::DOMPath::DOMPath(WebCore::DOMPath*)
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:60:5: note:   no known conversion for argument 1 from 'const WTF::String' to 'WebCore::DOMPath*'
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:55:5: note: WebCore::DOMPath::DOMPath(const WebCore::Path&)
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:55:5: note:   no known conversion for argument 1 from 'const WTF::String' to 'const WebCore::Path&'
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:54:5: note: WebCore::DOMPath::DOMPath()
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:54:5: note:   candidate expects 0 arguments, 1 provided
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:42:14: note: WebCore::DOMPath::DOMPath(const WebCore::DOMPath&)
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/html/canvas/DOMPath.h:42:14: note:   no known conversion for argument 1 from 'const WTF::String' to 'const WebCore::DOMPath&'


I cc Qt guys, maybe you can help fixing it.
Comment 11 Oliver Hunt 2013-02-20 17:20:52 PST
(In reply to comment #9)
> Committed r143505: <http://trac.webkit.org/changeset/143505>

Looks like a number of tests started failing after this -- presumably some interaction between the canvas path logic and regular path stuff?
Comment 12 Dirk Schulze 2013-02-20 17:51:19 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Committed r143505: <http://trac.webkit.org/changeset/143505>
> I cc Qt guys, maybe you can help fixing it.

Definitely. It would be interesting to know why this did not happen on the EWS bots. Both, WK1 and WK2 Qt passed.
Comment 13 Dirk Schulze 2013-02-20 18:00:47 PST
(In reply to comment #11)
> (In reply to comment #9)
> > Committed r143505: <http://trac.webkit.org/changeset/143505>
> 
> Looks like a number of tests started failing after this -- presumably some interaction between the canvas path logic and regular path stuff?

It actually does not touch anything of the current logic for drawing paths. If it is related to this patch, I wonder if one of the new tests causes flakiness. I run the test suite multiple times with the patch applied and never got any problems. I don't know how to investigate so.

Do you have the link to the results of the failing tests?
Comment 14 Dirk Schulze 2013-02-20 18:11:33 PST
(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Committed r143505: <http://trac.webkit.org/changeset/143505>
> > 
> > Looks like a number of tests started failing after this -- presumably some interaction between the canvas path logic and regular path stuff?
> 
> It actually does not touch anything of the current logic for drawing paths. If it is related to this patch, I wonder if one of the new tests causes flakiness. I run the test suite multiple times with the patch applied and never got any problems. I don't know how to investigate so.
> 
> Do you have the link to the results of the failing tests?

From the last build bots result I get the following crashing tests:

  fast/canvas/canvas-currentPath.html [ Failure ]
  fast/canvas/canvas-path-constructors.html [ Failure ]
  fast/dom/constructed-objects-prototypes.html [ Failure ]

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r143533%20(5686)/fast/canvas/canvas-currentPath-pretty-diff.html

From the results it looks like the attribute currentPath is not working on the bots properly. I am building from scratch again right now and investigate. The last test needs a rebaseline.

Do I miss more failing tests?
Comment 15 Dirk Schulze 2013-02-20 18:19:00 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Committed r143505: <http://trac.webkit.org/changeset/143505>
> I cc Qt guys, maybe you can help fixing it.

That build does not support SVG. I guarded everything but forgot one thing:

#if ENABLE(SVG)
    DOMPath(const String& pathData)
        : CanvasPathMethods()
    {
        buildPathFromString(pathData, m_path);
    }
#endif

should be

    DOMPath(const String& pathData)
        : CanvasPathMethods()
    {
#if ENABLE(SVG)
        buildPathFromString(pathData, m_path);
#else
        UNUSED_PARAM(pathData);
#endif
    }

I do not have a clean build right now. But will upload a patch ASAP.
Comment 16 Dirk Schulze 2013-02-20 19:24:26 PST
Committed r143554: <http://trac.webkit.org/changeset/143554>
Comment 17 Dirk Schulze 2013-02-20 19:27:27 PST
(In reply to comment #16)
> Committed r143554: <http://trac.webkit.org/changeset/143554>

Landed a fix for Qt and rebaselined the Ctor test.

The other two tests work absolutely fine for me. Maybe a completely clean build might fix this problem? It looks like the currentPath attribute on CanvasRenderingContext2D is not enabled on the bots.
Comment 18 Allan Sandfeld Jensen 2013-02-28 05:48:48 PST
Reopening to attach new patch.
Comment 19 Allan Sandfeld Jensen 2013-02-28 05:49:00 PST
Created attachment 190712 [details]
Patch
Comment 20 Tor Arne Vestbø 2013-02-28 05:58:30 PST
Comment on attachment 190712 [details]
Patch

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

> Tools/ChangeLog:4
> +        âhttps://bugs.webkit.org/show_bug.cgi?id=108508

?

> LayoutTests/ChangeLog:4
> +        âhttps://bugs.webkit.org/show_bug.cgi?id=108508

?
Comment 21 Allan Sandfeld Jensen 2013-02-28 06:00:28 PST
Landed in https://trac.webkit.org/r144300
Comment 22 Ádám Kallai 2013-03-01 07:39:22 PST
fast/canvas/canvas-currentPath.html test fails on Qt.
Diff is here:

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/canvas/canvas-currentPath-expected.txt
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/canvas/canvas-currentPath-actual.txt
@@ -62,7 +62,7 @@
 Transform CTM in the process of adding segments to context path. Check that currentPath's path object archive these transformations.
 PASS ctx.isPointInPath(49,49) is true
 PASS ctx.isPointInPath(99,99) is true
-PASS ctx.isPointInPath(149,149) is false
+FAIL ctx.isPointInPath(149,149) should be false. Was true.
 PASS ctx.isPointInPath(199,199) is true
 PASS ctx.isPointInPath(249,249) is true
 Clear current path on object and check that it is cleaned up.
@@ -75,7 +75,7 @@
 Apply path back to context path.
 PASS ctx.isPointInPath(49,49) is true
 PASS ctx.isPointInPath(99,99) is true
-PASS ctx.isPointInPath(149,149) is false
+FAIL ctx.isPointInPath(149,149) should be false. Was true.
 PASS ctx.isPointInPath(199,199) is true
 PASS ctx.isPointInPath(249,249) is true
 PASS successfullyParsed is true

Could you check it please?
Comment 23 Allan Sandfeld Jensen 2013-03-01 07:53:30 PST
(In reply to comment #22)
> fast/canvas/canvas-currentPath.html test fails on Qt.
<snip>
> Could you check it please?
It fails the same way here. Looks like a wrong rounding or something similar small. I would make the test as failing.