Bug 82560 – Implement canvas v5 line dash feature

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Lu Guanqun]

The new canvas v5 api is released by Ian. Adapt the old webkitLineDash to the new setLineDash() interface.

[Lu Guanqun]

Created attachment 134468 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


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

[Lu Guanqun]

Created attachment 134471 [details]
Patch

[Lu Guanqun]

Created attachment 134477 [details]
Patch

[Lu Guanqun]

Created attachment 134505 [details]
Patch

[Lu Guanqun]

Created attachment 134707 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134707 [details]
Patch

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

New failing tests:
fast/canvas/canvas-lineDash-invalid.html
platform/chromium/virtual/gpu/fast/canvas/canvas-lineDash-invalid.html

[WebKit Review Bot]

Created attachment 134719 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Igor Trindade Oliveira]

this patch can be a problem because a browser can already be using the old behavior.

[Lu Guanqun]

(In reply to comment #9)
> this patch can be a problem because a browser can already be using the old behavior.

Hi Igor,

Thanks for your comments. So what's the best practices for doing things like this(add new interfaces while deprecating old ones)?

I can add the new interface while keeping the old ones intact. Is that acceptable?

Thanks!
--guanqun

[Dean Jackson]

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-setlinedash

[Radar WebKit Bug Importer]

<rdar://problem/11159527>

[Tim Horton]

Comment on attachment 134707 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +

Can you explain here what's different from the old behavior?

Also, I believe that keeping the old behavior (and implementing the new one under the unprefixed name) is the right thing to do.

> Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:106
> +JSValue JSCanvasRenderingContext2D::setLineDash(ExecState* exec)

Why does this return a value that is always jsUndefined now?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:491
> +    if ((dash.size() % 2) == 1)

I don't think the == 1 is necessary.

> LayoutTests/ChangeLog:8
> +

I'm not positive, but I think you should test both the old prefixed behavior and the new behavior. Is there a test of the new odd-length-dash-array doubling behavior?

> LayoutTests/fast/canvas/canvas-lineDash-expected.txt:12
> +PASS lineDash[2] is 15

I guess so.

[Dean Jackson]

Comment on attachment 134707 [details]
Patch

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

+1 to Tim's comments

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:-80
> -        attribute [Custom] Array webkitLineDash;
> -        attribute float webkitLineDashOffset;

We shouldn't remove the existing API yet.

[Justin Novosad]

This implementation does not conform to the spec.  The IDL in the spec uses sequence<unrestricted-double> as the type for the dash array.

[Justin Novosad]

Comment on attachment 134707 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:123
> +            return throwError(TYPE_MISMATCH_ERR);

We usually throw INDEX_SIZE_ERR in this type of situation.
Spec needs clarification: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18754

[Justin Novosad]

Filed another issue against the spec for this feature to clarify whether the argument should be an array or a sequence:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=18758

[Justin Novosad]

(In reply to comment #17)
Already got answers back from Hixie:
1. No exceptions are to be thrown when a value in the dash array is negative or non-finite.
2. The dash array is a sequence, not an array.

[Justin Novosad]

Contacted Guanqun Lu, he is working on different projects now.
I'll finish it up.

[Justin Novosad]

Created attachment 162089 [details]
Patch

[Justin Novosad]

New patch:
* Leaves the old webkitLineDash behavior intact and tested.
* Does not use custom bindings
* Fixes spec compliance issues


(In reply to comment #20)
> Created an attachment (id=162089) [details]
> Patch

[Build Bot]

Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13744646

[Early Warning System Bot]

Comment on attachment 162089 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 162089 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 162089 [details]
Patch

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

[Build Bot]

Comment on attachment 162089 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 162089 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 162089 [details]
Patch

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

[Justin Novosad]

Created attachment 162258 [details]
Patch

[Justin Novosad]

New patch fixes build errors in ports that use JavaScriptCore.  Turns out the auto-generated bindings did not support the sequence<double> IDL type.  The implementation was just missing a couple template specializations.

[Darin Adler]

Comment on attachment 162258 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:395
> +            if (!jsValue.isNumber())
> +                return false;
> +
> +            indexedValue = jsValue.toNumber(exec);
> +            if (exec->hadException())
> +                return false;

This is not the right way to write it, and is unnecessarily inefficient. The toNumber() function is the general purpose way to convert any value to a number, and should not be used if the code has already checked isNumber. If this code is really supposed to reject anything that’s not a number, then it should just call asNumber() after isNumber(). If it’s supposed to work for other types of objects, then toNumber should be used without an isNumber check.

The existing conversion to unsigned long above has a similar mistake, but it’s a little harder to fix correctly. The toUInt32() function does too much, but there’s no obvious alternative that does exactly the right thing.

[Justin Novosad]

(In reply to comment #31)
> (From update of attachment 162258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162258&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:395
> > +            if (!jsValue.isNumber())
> > +                return false;
> > +
> > +            indexedValue = jsValue.toNumber(exec);
> > +            if (exec->hadException())
> > +                return false;
> 
> This is not the right way to write it, and is unnecessarily inefficient. The toNumber() function is the general purpose way to convert any value to a number, and should not be used if the code has already checked isNumber. If this code is really supposed to reject anything that’s not a number, then it should just call asNumber() after isNumber(). If it’s supposed to work for other types of objects, then toNumber should be used without an isNumber check.
> 
> The existing conversion to unsigned long above has a similar mistake, but it’s a little harder to fix correctly. The toUInt32() function does too much, but there’s no obvious alternative that does exactly the right thing.

To figure-out the right thing to do, I looked up the WebIDL spec: http://www.w3.org/TR/WebIDL/#idl-types
Found this: "If the conversion cannot be performed, then the operation will not run or the attribute will not be updated. In some language bindings, type conversions could result in an exception being thrown."

So I guess isNumber + asNumber would be the way to go.

[Justin Novosad]

Created attachment 162267 [details]
Patch

[Ian 'Hixie' Hickson]

Justin, please don't refer to specs with "/TR/" in the URL, they are by definition out of date. For Web IDL, you want: http://dev.w3.org/2006/webapi/WebIDL/

[Early Warning System Bot]

Comment on attachment 162267 [details]
Patch

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

[Build Bot]

Comment on attachment 162267 [details]
Patch

Attachment 162267 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13761613

[Gyuyoung Kim]

Comment on attachment 162267 [details]
Patch

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

[Justin Novosad]

Created attachment 162291 [details]
Patch

[Build Bot]

Comment on attachment 162291 [details]
Patch

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

[Dean Jackson]

Comment on attachment 162291 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:388
> +        static inline bool arrayNativeValue(JSC::ExecState* exec, JSC::JSValue jsValue, double& indexedValue)

Remove the "exec" to avoid the unused param error.

[Justin Novosad]

Created attachment 162301 [details]
Patch

[Dean Jackson]

Comment on attachment 162301 [details]
Patch

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

It's slightly weird that the lineDashOffset methods call from the webkit prefix into the unprefixed, but the setLineDash method does it the other way around. But that's not a big deal.

It's also a shame we can't copy the vector directly thanks to the CGFloat -> double cast :(

I wonder if we should have image tests for this? It seems important.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:523
> +    int copyCount = 1 + dash.size() % 2;

I think this is more clear as int copyCount = (dash.size() % 2) ? 2 : 1; (but I'm not asking to change)

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:533
> +    for (size_t i = 0; i < dash.size(); i++) {
> +        if (!isfinite(dash[i]) || dash[i] < 0)
> +            return;
> +        convertedDashArray[i] = static_cast<DashArrayEntry>(dash[i]);
> +    }
> +    if (copyCount == 2) {
> +        for (size_t src = 0, dst = dash.size(); src < dash.size(); ++src, ++dst)
> +            convertedDashArray[dst] = convertedDashArray[src];
> +    }

I'm not sure it would actually be more readable, but you could merge the content of the second loop into the first.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:547
> +    if (!isfinite(offset))
> +        return;
> +    if (state().m_lineDashOffset == offset)
>          return;

Might as well combine these into a single test.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:79
> +        void setLineDash(in sequence<double> dash);
> +        sequence<double> getLineDash();
> +        attribute float lineDashOffset;

I wonder why the spec says the dash array is a list of double when the offset is a float?

> LayoutTests/fast/canvas/script-tests/canvas-lineDash-invalid.js:10
> +    ctx.setLineDash([1.5, 2.5]);

Could you move all these lines into a resetLineDash() method? It could probably debug() when it runs too, because I was looking at the expected output and couldn't work out why these numbers were coming up.

> LayoutTests/fast/canvas/script-tests/canvas-lineDash-invalid.js:33
> +shouldBe("trySettingLineDash([1, 2, 3])", "[1, 2, 3, 1, 2, 3]");

Technically this one isn't invalid, yet it is in the invalid test.

[Justin Novosad]

> 
> I wonder why the spec says the dash array is a list of double when the offset is a float?
> 

My bad. Spec says it is a double. Good catch.

[Justin Novosad]

(In reply to comment #43)
> > 
> > I wonder why the spec says the dash array is a list of double when the offset is a float?
> > 
> 
> My bad. Spec says it is a double. Good catch.

Internally, the GraphicsContext interface uses float for the lineDash offset, even for the Cairo port, which uses double for the elements of DashArray.  Given that we have no use for double precision internally, how important is it for the JS interface to expose doubles rather than floats?

[Dean Jackson]

(In reply to comment #43)
> > 
> > I wonder why the spec says the dash array is a list of double when the offset is a float?
> > 
> 
> My bad. Spec says it is a double. Good catch.

It just shows I didn't read the spec :)

[Dean Jackson]

(In reply to comment #44)

> Internally, the GraphicsContext interface uses float for the lineDash offset, even for the Cairo port, which uses double for the elements of DashArray.  Given that we have no use for double precision internally, how important is it for the JS interface to expose doubles rather than floats?

I'm not sure - sounds like we wouldn't lose much.

[Ian 'Hixie' Hickson]

The JS interface actually exposes "Number"s, and Web IDL says that "double" is a better match for that than "float", and basically discourages the specs from using "float" at all. The HTML spec never uses "float".

[Justin Novosad]

After some thinking... Even though it is useless for webkit, I think we need the interface to be double, and the context state to store the value as a double, otherwise we risk getting burned by rounding in compliance tests.

ctx.lineDashOffset = someDoublePrecisionValue;
if (ctx.lineDashOffset != someDoublePrecisionValue) {
    print("Look at that, this UA is busted!")
}

[Darin Adler]

(In reply to comment #32)
> To figure-out the right thing to do, I looked up the WebIDL spec: http://www.w3.org/TR/WebIDL/#idl-types
> Found this: "If the conversion cannot be performed, then the operation will not run or the attribute will not be updated. In some language bindings, type conversions could result in an exception being thrown."
> 
> So I guess isNumber + asNumber would be the way to go.

I don’t agree.

Given that description, it sounds like toNumber is the way to go. That’s the function that does the conversion. Calling isNumber prevents conversion and creates failure if the value isn’t already a number. Conversion is exactly what toNumber is for.

We need test cases that cover this behavior.

[Dean Jackson]

(In reply to comment #49)
> (In reply to comment #32)
> > To figure-out the right thing to do, I looked up the WebIDL spec: http://www.w3.org/TR/WebIDL/#idl-types
> > Found this: "If the conversion cannot be performed, then the operation will not run or the attribute will not be updated. In some language bindings, type conversions could result in an exception being thrown."
> > 
> > So I guess isNumber + asNumber would be the way to go.
> 
> I don’t agree.
> 
> Given that description, it sounds like toNumber is the way to go. That’s the function that does the conversion. Calling isNumber prevents conversion and creates failure if the value isn’t already a number. Conversion is exactly what toNumber is for.
> 
> We need test cases that cover this behavior.

So you're supposed to be able to do ctx.lineDashOffset = "10"; ? That makes sense.
(Similarly lineDash = ["10", 20, "30.0"];)

[Dean Jackson]

Comment on attachment 162301 [details]
Patch

r- to make darin's change (and add conversion operations to the valid/invalid tests)

[Justin Novosad]

Created attachment 162762 [details]
Patch

[Justin Novosad]

(In reply to comment #52)
> Created an attachment (id=162762) [details]
> Patch

Improvements in the latest patch:
* CanvasRenderingContext2D now uses double-precision for line dash state and interface
* Code reorg in CanvasRenderingContext2D
* Now using toNumber to convert double values from JS to native
* Layout test validates conversion from string to number for lineDash element.
* Layout test verifies rendering behavior (text-only baseline)

[Darin Adler]

Comment on attachment 162762 [details]
Patch

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

This patch seems great and almost ready to go. But I am not comfortable with the mix of float and double in our canvas element implementation without any rationale or plan for moving forward. I see how it’s handy to use doubles for this new feature, but we need to look at this in context, and not in isolation.

> Source/WebCore/bindings/js/JSDOMBinding.h:394
> +            if (exec->hadException())
> +                return false;
> +
> +            return true;

I think this reads better as:

    return !exec->hadException();

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:516
> +static bool validateLineDash(const Vector<double>& dash)

I would name this something more like:

    lineDashSequenceIsValid

Using a verb phrase to name a function that returns a boolean with no side effects raises the question of whether there are side effects, and also makes it unclear what the boolean result of the function means.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:533
> +    if (dash.size() % 2)
> +        modifiableState().m_lineDash.append(dash);

This mysterious code needs a brief "why" comment. You shouldn’t assume that the person reading the code has read the specification and so understands the need for this peculiar rule.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:581
> +    DashArray convertedLineDash;
> +    convertedLineDash.resize(state().m_lineDash.size());

The correct way to allocate a vector with an initial size is to pass it into the constructor. If you make a separate function call it’s unnecessarily less efficient.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:257
> +        Vector<double> m_lineDash;
> +        double m_lineDashOffset;

This is not good. After this patch the canvas implementation now has a mix of double and float and there is no rationale for which items are float and which are double. Until this patch it was all float.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:-77
> -        // FIXME: These attributes should also be implemented for V8.

Why did you remove this FIXME? Is the legacy WebKit line dash feature something we’ll never do in V8-based builds of WebKit? If so, maybe a "why" comment is in order.

> Source/WebCore/platform/graphics/DashArray.h:32
> +typedef CGFloat DashArrayEntry;

I think perhaps the word Element rather than Entry would be better?

[Justin Novosad]

(In reply to comment #54)

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:257
> > +        Vector<double> m_lineDash;
> > +        double m_lineDashOffset;
> 
> This is not good. After this patch the canvas implementation now has a mix of double and float and there is no rationale for which items are float and which are double. Until this patch it was all float.

I am unsure about what to do here. Is it acceptable for a number to loose precision from a round trip through the API?

[Justin Novosad]

(In reply to comment #55)

> I am unsure about what to do here. Is it acceptable for a number to loose precision from a round trip through the API?

I guess this question was settled long before I came along. Just realized that all the other number attributes and args are float in CanvasRenderingContext2D.idl.  I'll just conform to that.

[Justin Novosad]

Created attachment 162846 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 162846 [details]
Patch

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

[Build Bot]

Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13771915

[Early Warning System Bot]

Comment on attachment 162846 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 162846 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 162846 [details]
Patch

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

[Build Bot]

Comment on attachment 162846 [details]
Patch

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

[Justin Novosad]

Created attachment 163120 [details]
Patch

[Darin Adler]

Comment on attachment 163120 [details]
Patch

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

r=me, assuming you fix that one toDouble that should be toFloat

> Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:114
> -        float elem = valueArray->getIndex(i).toFloat(exec);
> +        float elem = valueArray->getIndex(i).toNumber(exec);

This change no longer makes sense. Should be left alone.

[Justin Novosad]

Created attachment 163135 [details]
Patch for landing

[Dean Jackson]

Comment on attachment 163135 [details]
Patch for landing

Verified that Darin's suggestion was updated. Setting cq.

[WebKit Review Bot]

Comment on attachment 163135 [details]
Patch for landing

Clearing flags on attachment: 163135

Committed r128116: <http://trac.webkit.org/changeset/128116>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Szilard Ledan]

This test fails on Qt bots.

I created a bug report: #96360.

Could you check it please?