Implement addPath() method for Path object

Implement addPath() method for Path object

Intent to implement on blink-dev:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/addPath/blink-dev/YY-FmD8Efhc/y4Ag3pGF-toJ

BUG=159839
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168561

[pals, 2014-02-18 14:40:09]

Please review.

[rwlbuis, 2014-02-18 15:59:59]

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
File Source/core/html/canvas/DOMPath.h (right):

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
Source/core/html/canvas/DOMPath.h:55: m_path = path->path();
You can put this in the initialization list, it is a bit more efficient.

[jbroman, 2014-02-18 16:06:46]

https://codereview.chromium.org/170503002/diff/1/Source/platform/graphics/Pat...
File Source/platform/graphics/Path.cpp (right):

https://codereview.chromium.org/170503002/diff/1/Source/platform/graphics/Pat...
Source/platform/graphics/Path.cpp:442: }
drive-by nit: newline below this method, please.

[Justin Novosad, 2014-02-18 16:09:29]

Was there an intent to implement thread for this on blink-dev?

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
File LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js (right):

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:11: var m =
svgElement.createSVGMatrix();
simpler way to create an SVGMatrix: 
var m = ctx.currentTransform;

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:24: debug("Test
addPath() with transform as translate(100, -100).")
Please add an additional test case where the transform is a non-invertible
matrix.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:26: var p1 = new
Path();
re-declaration of existing variable.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:30: var svgElement =
document.createElementNS("http://www.w3.org/2000/svg", "svg");
No need to re-create this

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:51:
shouldThrow("p1.addPath(p2, null)");
According to the spec, the transform is a nullable argument.  If transform is
null, we must just leaving the path untransformed (same as if transform was an
identity matrix).

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
File Source/core/html/canvas/DOMPath.h (right):

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
Source/core/html/canvas/DOMPath.h:51: DOMPath(DOMPath* path)
This is weird. This is kind of like a public copy constructor, but not quite,
yet this class has WTF_MAKE_NONCOPYABLE(DOMPath).  It looks too much like
cheating.  Perhaps we should have a clone() method instead?

[pals, 2014-02-18 16:35:45]

On 2014/02/18 16:09:29, junov wrote:
> Was there an intent to implement thread for this on blink-dev?
> 
I could not find any intent to implement on blink-dev for this api. This will be
hidden behind the experimental canvas flag. Do I need to send an "Intent to
implement" on blink-dev?

[Justin Novosad, 2014-02-18 16:42:46]

On 2014/02/18 16:35:45, sanjoy.pal wrote:
> On 2014/02/18 16:09:29, junov wrote:
> > Was there an intent to implement thread for this on blink-dev?
> > 
> I could not find any intent to implement on blink-dev for this api. This will
be
> hidden behind the experimental canvas flag. Do I need to send an "Intent to
> implement" on blink-dev?

Yes, implementing a feature that is not exposed by default is exactly what
"intent to implement" is for. Once you want to expose it to the world, you send
an "intent to ship" message.
You will find the e-mail template and the instructions here:
http://www.chromium.org/blink.

[Rik, 2014-02-18 17:00:26]

On 2014/02/18 14:40:09, sanjoy.pal wrote:
> Please review.

Some people have expressed unease with have the SVGMatrix object in the canvas
API.
For SVG 2.0, there are plans to deprecate SVGMatrix and replace it with
DOMMatrix (or something equivalent)

You could implement this, but you might get pushback if you try to ship it.

[pals, 2014-02-19 11:47:21]

Fixed the review comments. I shall send an Intent to implement on blink-dev as
well.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
File LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js (right):

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:11: var m =
svgElement.createSVGMatrix();
On 2014/02/18 16:09:29, junov wrote:
> simpler way to create an SVGMatrix: 
> var m = ctx.currentTransform;

Done.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:24: debug("Test
addPath() with transform as translate(100, -100).")
On 2014/02/18 16:09:29, junov wrote:
> Please add an additional test case where the transform is a non-invertible
> matrix.

Done.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:26: var p1 = new
Path();
On 2014/02/18 16:09:29, junov wrote:
> re-declaration of existing variable.

Done.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:30: var svgElement =
document.createElementNS("http://www.w3.org/2000/svg", "svg");
On 2014/02/18 16:09:29, junov wrote:
> No need to re-create this

Done.

https://codereview.chromium.org/170503002/diff/1/LayoutTests/fast/canvas/scri...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:51:
shouldThrow("p1.addPath(p2, null)");
On 2014/02/18 16:09:29, junov wrote:
> According to the spec, the transform is a nullable argument.  If transform is
> null, we must just leaving the path untransformed (same as if transform was an
> identity matrix).

The addPath idl changes generates following code

static void addPathMethod(const v8::FunctionCallbackInfo<v8::Value>& info)
{
    if (UNLIKELY(info.Length() < 2)) {
        throwTypeError(ExceptionMessages::failedToExecute("addPath", "Path",
ExceptionMessages::notEnoughArguments(2, info.Length())), info.GetIsolate());
        return;
    }
    DOMPath* imp = V8Path::toNative(info.Holder());
    V8TRYCATCH_VOID(DOMPath*, path, V8Path::hasInstance(info[0],
info.GetIsolate()) ? V8Path::toNative(v8::Handle<v8::Object>::Cast(info[0])) :
0);
    V8TRYCATCH_VOID(RefPtr<SVGMatrixTearOff >, transform,
V8SVGMatrix::hasInstance(info[1], info.GetIsolate()) ?
V8SVGMatrix::toNative(v8::Handle<v8::Object>::Cast(info[1])) : 0);
    if (!transform) {
        throwTypeError(ExceptionMessages::failedToExecute("addPath", "Path",
"parameter 2 is not of type 'SVGMatrix'."), info.GetIsolate());
        return;
    }
    imp->addPath(path, transform->propertyReference());
}

So, we never get transform as NULL.

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
File Source/core/html/canvas/DOMPath.h (right):

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
Source/core/html/canvas/DOMPath.h:51: DOMPath(DOMPath* path)
On 2014/02/18 16:09:29, junov wrote:
> This is weird. This is kind of like a public copy constructor, but not quite,
> yet this class has WTF_MAKE_NONCOPYABLE(DOMPath).  It looks too much like
> cheating.  Perhaps we should have a clone() method instead?

Done. Reverted this change.

https://codereview.chromium.org/170503002/diff/1/Source/core/html/canvas/DOMP...
Source/core/html/canvas/DOMPath.h:55: m_path = path->path();
On 2014/02/18 15:59:59, rwlbuis wrote:
> You can put this in the initialization list, it is a bit more efficient.

Done.

https://codereview.chromium.org/170503002/diff/1/Source/platform/graphics/Pat...
File Source/platform/graphics/Path.cpp (right):

https://codereview.chromium.org/170503002/diff/1/Source/platform/graphics/Pat...
Source/platform/graphics/Path.cpp:442: }
On 2014/02/18 16:06:47, jbroman wrote:
> drive-by nit: newline below this method, please.

Done.

[Justin Novosad, 2014-02-19 16:38:28]

lgtm with minor corrections.

Please give people on blink-dev time to respond. Wait one week after sending the
intent to implement message before committing this change.

https://codereview.chromium.org/170503002/diff/170001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js (right):

https://codereview.chromium.org/170503002/diff/170001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:36: var imageData =
ctx.getImageData(100, 0, 100, 100);
no need for 'var' here. Same thing for following line.

https://codereview.chromium.org/170503002/diff/170001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:57: var imageData =
ctx.getImageData(100, 100, 100, 100);
Same here.

[pals, 2014-03-03 06:27:38]

Its more than a week. The intent-to-implement haven't received any LGTM's from
API owners. Should I commit this change?

https://codereview.chromium.org/170503002/diff/170001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js (right):

https://codereview.chromium.org/170503002/diff/170001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:36: var imageData =
ctx.getImageData(100, 0, 100, 100);
On 2014/02/19 16:38:28, junov wrote:
> no need for 'var' here. Same thing for following line.

Done.

https://codereview.chromium.org/170503002/diff/170001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-path-addpath.js:57: var imageData =
ctx.getImageData(100, 100, 100, 100);
On 2014/02/19 16:38:28, junov wrote:
> Same here.

Done.

[Justin Novosad, 2014-03-03 15:49:06]

On 2014/03/03 06:27:38, sanjoy.pal wrote:
> Its more than a week. The intent-to-implement haven't received any LGTM's from
> API owners. Should I commit this change?

That is fine. Only "intent to ship" is formally required to receive lgtm's from
the blink API owners.  Feel free to commit when you are ready.
The concern raised by Rik can easily be fixed when the time comes, before
shipping this API.

[pals, 2014-03-04 08:14:30]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2014-03-04 08:15:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/170503002/270001

[commit-bot: I haz the power, 2014-03-04 09:01:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 09:01:25]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[pals, 2014-03-04 10:38:47]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2014-03-04 10:38:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/170503002/270001

[commit-bot: I haz the power, 2014-03-04 11:06:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 11:06:43]

Retried try job too often on mac_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[pals, 2014-03-04 14:42:57]

On 2014/03/04 11:06:43, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_blink_rel for step(s) blink_heap_unittests,
> blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
> webkit_unit_tests, wtf_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

The new IDL compiler is not not doing the null check for path and transform in
the generated code. 
I made few changes related to that. Please review one more time.

[Justin Novosad, 2014-03-04 16:37:43]

lgtm

[pals, 2014-03-05 05:48:56]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2014-03-05 05:49:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/170503002/300001

[commit-bot: I haz the power, 2014-03-05 20:55:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/170503002/300001

[commit-bot: I haz the power, 2014-03-06 03:51:32]

Change committed as 168561