Bug 42189 – Failing 2d.path.stroke.prune.line philip canvas test

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Matthew Delaney]

Safari 5 fails 2d.path.stroke.prune.line canvas test.

[Matthew Delaney]

Created attachment 61461 [details]
Patch

[Adele Peterson]

Comment on attachment 61461 [details]
Patch

Looks good.  The local variable should get a more meaningful name - like "path".  There are a bunch of "p"s and "cp"s in this file, so you don't have to fix those, but since you're adding a new one, you might as well give it a good name.

r=me!  Do you want to use the commit-queue for this?

[Adele Peterson]

Comment on attachment 61461 [details]
Patch

Actually, I think you don't need the local variable at all.  You can just write it like this:

    if (p == CGPathGetCurrentPoint(m_path))

[Matthew Delaney]

Created attachment 61545 [details]
Updated with Adele's suggestions.

Updated with Adele's suggestions.

[WebKit Commit Bot]

Comment on attachment 61545 [details]
Updated with Adele's suggestions.

Rejecting patch 61545 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[Darin Adler]

Comment on attachment 61545 [details]
Updated with Adele's suggestions.

>  void Path::addLineTo(const FloatPoint& p)
> -{
> +{   

Should not add trailing spaces here.

Is it right to have the check be in the CG version of Path::addLineTo? Maybe the caller should do the check in platform independent code. I think it would be good to follow up and investigate that.

[Matthew Delaney]

@Darin: I wanted to put the checking higher up in CanvasRenderingContext2D.cpp so that it would be platform independent, but there's currently no way to expose the current point in in the path, so I'm adding that in at the moment. Though, I'm worried that there may be a good reason why that isn't exposed. I can't find one yet since it seems fine as long as the current point in the path isn't exposed at the canvas api layer we should be all good.

[WebKit Commit Bot]

Comment on attachment 61545 [details]
Updated with Adele's suggestions.

Rejecting patch 61545 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20689 test cases.
svg/custom/marker-overflow-clip.svg -> failed

Exiting early after 1 failures. 18057 tests run.
335.97s total testing time

18056 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
26 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3401413

[Andreas Kling]

Comment on attachment 61545 [details]
Updated with Adele's suggestions.

Since Path has currentPoint() now, this can and should be done in cross-platform code.

[Matthew Delaney]

True. I actually rolled that into the stroke.prune.arc fix, so I should obsolete this bug. Doing that now...

[Matthew Delaney]

Fixed in https://bugs.webkit.org/show_bug.cgi?id=42188, fix committed r63599