Bug 82560

Summary: Implement canvas v5 line dash feature
Product: WebKit Reporter: Lu Guanqun <guanqun.lu>
Component: CanvasAssignee: Justin Novosad <junov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, dglazkov, dino, dongseong.hwang, eoconnor, gtk-ews, gustavo, haraken, ian, igor.oliveira, japhet, joybro201, junov, ojan, peter, philn, simon.fraser, szledan, thorton, webkit-bug-importer, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96360    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Lu Guanqun 2012-03-28 18:40:22 PDT
The new canvas v5 api is released by Ian. Adapt the old webkitLineDash to the new setLineDash() interface.
Comment 1 Lu Guanqun 2012-03-28 18:50:49 PDT
Created attachment 134468 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-28 18:54:07 PDT
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.
Comment 3 Lu Guanqun 2012-03-28 18:59:28 PDT
Created attachment 134471 [details]
Patch
Comment 4 Lu Guanqun 2012-03-28 19:06:07 PDT
Created attachment 134477 [details]
Patch
Comment 5 Lu Guanqun 2012-03-28 23:29:05 PDT
Created attachment 134505 [details]
Patch
Comment 6 Lu Guanqun 2012-03-29 18:11:48 PDT
Created attachment 134707 [details]
Patch
Comment 7 WebKit Review Bot 2012-03-29 20:59:03 PDT
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
Comment 8 WebKit Review Bot 2012-03-29 20:59:11 PDT
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
Comment 9 Igor Trindade Oliveira 2012-03-29 21:12:13 PDT
this patch can be a problem because a browser can already be using the old behavior.
Comment 10 Lu Guanqun 2012-03-29 21:38:22 PDT
(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
Comment 12 Radar WebKit Bug Importer 2012-03-30 16:23:46 PDT
<rdar://problem/11159527>
Comment 13 Tim Horton 2012-06-24 08:00:51 PDT
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.
Comment 14 Dean Jackson 2012-06-25 14:13:06 PDT
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.
Comment 15 Justin Novosad 2012-08-31 13:52:14 PDT
This implementation does not conform to the spec.  The IDL in the spec uses sequence<unrestricted-double> as the type for the dash array.
Comment 16 Justin Novosad 2012-08-31 13:58:43 PDT
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
Comment 17 Justin Novosad 2012-08-31 14:09:27 PDT
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
Comment 18 Justin Novosad 2012-08-31 20:46:26 PDT
(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.
Comment 19 Justin Novosad 2012-09-04 05:52:58 PDT
Contacted Guanqun Lu, he is working on different projects now.
I'll finish it up.
Comment 20 Justin Novosad 2012-09-04 13:15:07 PDT
Created attachment 162089 [details]
Patch
Comment 21 Justin Novosad 2012-09-04 13:20:00 PDT
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
Comment 22 Build Bot 2012-09-04 13:56:18 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13744646
Comment 23 Early Warning System Bot 2012-09-04 14:14:48 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13757339
Comment 24 Early Warning System Bot 2012-09-04 14:23:49 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13740668
Comment 25 kov's GTK+ EWS bot 2012-09-04 14:42:46 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13743630
Comment 26 Build Bot 2012-09-04 14:51:48 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13741651
Comment 27 Gyuyoung Kim 2012-09-04 15:01:05 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13746607
Comment 28 Early Warning System Bot 2012-09-04 15:15:43 PDT
Comment on attachment 162089 [details]
Patch

Attachment 162089 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13740686
Comment 29 Justin Novosad 2012-09-05 08:43:54 PDT
Created attachment 162258 [details]
Patch
Comment 30 Justin Novosad 2012-09-05 08:47:42 PDT
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.
Comment 31 Darin Adler 2012-09-05 08:56:10 PDT
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.
Comment 32 Justin Novosad 2012-09-05 09:25:51 PDT
(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.
Comment 33 Justin Novosad 2012-09-05 09:30:01 PDT
Created attachment 162267 [details]
Patch
Comment 34 Ian 'Hixie' Hickson 2012-09-05 09:38:19 PDT
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/
Comment 35 Early Warning System Bot 2012-09-05 10:34:51 PDT
Comment on attachment 162267 [details]
Patch

Attachment 162267 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13744998
Comment 36 Build Bot 2012-09-05 10:54:47 PDT
Comment on attachment 162267 [details]
Patch

Attachment 162267 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13761613
Comment 37 Gyuyoung Kim 2012-09-05 11:00:39 PDT
Comment on attachment 162267 [details]
Patch

Attachment 162267 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13754871
Comment 38 Justin Novosad 2012-09-05 11:10:57 PDT
Created attachment 162291 [details]
Patch
Comment 39 Build Bot 2012-09-05 11:45:17 PDT
Comment on attachment 162291 [details]
Patch

Attachment 162291 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13768018
Comment 40 Dean Jackson 2012-09-05 11:57:41 PDT
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.
Comment 41 Justin Novosad 2012-09-05 12:10:58 PDT
Created attachment 162301 [details]
Patch
Comment 42 Dean Jackson 2012-09-05 13:12:36 PDT
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.
Comment 43 Justin Novosad 2012-09-05 13:19:51 PDT
> 
> 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.
Comment 44 Justin Novosad 2012-09-05 13:41:07 PDT
(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?
Comment 45 Dean Jackson 2012-09-05 13:42:56 PDT
(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 :)
Comment 46 Dean Jackson 2012-09-05 13:44:33 PDT
(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.
Comment 47 Ian 'Hixie' Hickson 2012-09-05 13:51:23 PDT
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".
Comment 48 Justin Novosad 2012-09-05 14:00:00 PDT
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!")
}
Comment 49 Darin Adler 2012-09-05 14:39:33 PDT
(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.
Comment 50 Dean Jackson 2012-09-05 16:23:11 PDT
(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"];)
Comment 51 Dean Jackson 2012-09-05 16:27:07 PDT
Comment on attachment 162301 [details]
Patch

r- to make darin's change (and add conversion operations to the valid/invalid tests)
Comment 52 Justin Novosad 2012-09-07 07:13:35 PDT
Created attachment 162762 [details]
Patch
Comment 53 Justin Novosad 2012-09-07 07:19:28 PDT
(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)
Comment 54 Darin Adler 2012-09-07 09:39:32 PDT
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?
Comment 55 Justin Novosad 2012-09-07 11:14:53 PDT
(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?
Comment 56 Justin Novosad 2012-09-07 12:18:14 PDT
(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.
Comment 57 Justin Novosad 2012-09-07 12:47:24 PDT
Created attachment 162846 [details]
Patch
Comment 58 kov's GTK+ EWS bot 2012-09-07 13:45:10 PDT
Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13776828
Comment 59 Build Bot 2012-09-07 13:49:32 PDT
Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13771915
Comment 60 Early Warning System Bot 2012-09-07 14:38:38 PDT
Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13777764
Comment 61 Gyuyoung Kim 2012-09-07 14:58:12 PDT
Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13792004
Comment 62 Early Warning System Bot 2012-09-07 14:59:23 PDT
Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13769969
Comment 63 Build Bot 2012-09-07 16:39:29 PDT
Comment on attachment 162846 [details]
Patch

Attachment 162846 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13777807
Comment 64 Justin Novosad 2012-09-10 07:09:09 PDT
Created attachment 163120 [details]
Patch
Comment 65 Darin Adler 2012-09-10 07:49:13 PDT
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.
Comment 66 Justin Novosad 2012-09-10 08:16:57 PDT
Created attachment 163135 [details]
Patch for landing
Comment 67 Dean Jackson 2012-09-10 10:41:51 PDT
Comment on attachment 163135 [details]
Patch for landing

Verified that Darin's suggestion was updated. Setting cq.
Comment 68 WebKit Review Bot 2012-09-10 15:21:28 PDT
Comment on attachment 163135 [details]
Patch for landing

Clearing flags on attachment: 163135

Committed r128116: <http://trac.webkit.org/changeset/128116>
Comment 69 WebKit Review Bot 2012-09-10 15:21:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 70 Szilard Ledan 2012-09-11 00:33:35 PDT
This test fails on Qt bots.

I created a bug report: #96360.

Could you check it please?