Bug 82560

Created attachment 134468 [details] Patch

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.

Created attachment 134471 [details] Patch

Created attachment 134477 [details] Patch

Created attachment 134505 [details] Patch

Created attachment 134707 [details] Patch

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

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

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

(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

<rdar://problem/11159527>

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 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.

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 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

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

(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.

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

Created attachment 162089 [details] Patch

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 on attachment 162089 [details] Patch Attachment 162089 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13744646

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

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

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

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

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

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

Created attachment 162258 [details] Patch

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 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.

(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.

Created attachment 162267 [details] Patch

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 on attachment 162267 [details] Patch Attachment 162267 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13744998

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

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

Created attachment 162291 [details] Patch

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

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.

Created attachment 162301 [details] Patch

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.

> > 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.

(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?

(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 :)

(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.

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".

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!") }

(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.

(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 on attachment 162301 [details] Patch r- to make darin's change (and add conversion operations to the valid/invalid tests)

Created attachment 162762 [details] Patch

(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 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?

(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?

(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.

Created attachment 162846 [details] Patch

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

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

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

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

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

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

Created attachment 163120 [details] Patch

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.

Created attachment 163135 [details] Patch for landing

Comment on attachment 163135 [details] Patch for landing Verified that Darin's suggestion was updated. Setting cq.

Comment on attachment 163135 [details] Patch for landing Clearing flags on attachment: 163135 Committed r128116: <http://trac.webkit.org/changeset/128116>

All reviewed patches have been landed. Closing bug.

This test fails on Qt bots. I created a bug report: #96360. Could you check it please?