Bug 38845

Created attachment 55555 [details] Proposed patch, first stab

Attachment 55555 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/html/canvas/CanvasStyle.cpp:242: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CanvasStyle.h:30: Alphabetical sorting problem. [build/include_order] [4] WebCore/html/canvas/CanvasStyle.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 3 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 55557 [details] Proposed patch, first stab + stylefixes

(In reply to comment #3) > Created an attachment (id=55557) [details] > Proposed patch, first stab + stylefixes I miss a test, where you set a color like: fillStyle = "rgba(255,0,0,1.0)"; If I read the Spec correctly, we should get back "#ff0000".

(In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=55557) [details] [details] > > Proposed patch, first stab + stylefixes > > I miss a test, where you set a color like: fillStyle = "rgba(255,0,0,1.0)"; > If I read the Spec correctly, we should get back "#ff0000". Also your're violating this part: "a U+0030 DIGIT ZERO, a U+002E FULL STOP (representing the decimal point), one or more digits in the range 0-9 (U+0030 to U+0039) representing the fractional part of the alpha value" shouldBe("ctx.shadowColor", "'rgba(1, 2, 3, 0.40000)'"); is wrong here, should be "rgba(1, 2, 3, 0.4)"

Comment on attachment 55557 [details] Proposed patch, first stab + stylefixes I'm also a bit unhappy about the parsing code. We have some bugs about the performance issues on color parsing. So it's not a good idea to parse the color multiple times: in setFill(/Stroke/Shadow)Color and again in CanvasStyle::applyFill(/Stroke/Shadow)Color().

Created attachment 55725 [details] Proposed patch v2 Updated patch with all comments except #5 addressed. @Dirk, are you sure returning 0.40000 instead of 0.4 is in violation of the spec?

Attachment 55725 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/html/canvas/CanvasStyle.cpp:1: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/html/canvas/CanvasStyle.cpp:126: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/html/canvas/CanvasStyle.cpp:169: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #7) > Created an attachment (id=55725) [details] > Proposed patch v2 > > Updated patch with all comments except #5 addressed. > @Dirk, are you sure returning 0.40000 instead of 0.4 is in violation of the spec? I pasted the part of the spec. And the spec talks about one digit after the point. That doesn't mean that I agree to the spec, but if I get it right, we give back a string. And the user has to parse it on his own. So a defined output with limited digits after the point makes sense some how.

Created attachment 55794 [details] Proposed patch v3 Style fixed + CanvasStyle slightly slimmed. Dirk, I don't think I'm violating the spec here, it reads to me as if this format should be acceptable. If nothing else, it's what Gecko does. (Weak argument, I know.) That said, I would personally also prefer returning "0.4" instead of "0.40000". Is there a simple way to do this?

Created attachment 55797 [details] Proposed patch v4 Oops! I was only running the fast/canvas tests at first and missed the fact that fast/dom/canvasContext2d-element-attribute-js-null.html checks the shadowColor property.

Comment on attachment 55797 [details] Proposed patch v4 The color parsing code looks better now. > +String serializedCanvasColor(const Color& color) > +{ > + // FIXME: What should we do with invalid colors? > + if (!color.isValid()) > + return String("rgba(0, 0, 0, 0.0)"); > + > + if (color.hasAlpha()) { > + // Match Gecko (0.0 for zero, 5 decimals for anything else) > + if (!color.alpha()) > + return String::format("rgba(%u, %u, %u, 0.0)", color.red(), color.green(), color.blue()); > + return String::format("rgba(%u, %u, %u, %.5f)", color.red(), color.green(), color.blue(), color.alpha() / 255.0f); > } > + > + return String::format("#%02x%02x%02x", color.red(), color.green(), color.blue()); I'm sorry, I missread the spec. The spec talks about 1 _or more digits_ for fractional part. Nevertheless, if alpha is zero, you just use one digit after the point, if alpha is >0 but <1 you use 5? Shouldn't we have the same count of digits? What is FF and Opera doing here? Maybe we should aline to them here. In my opinion, the fractional part should allways have the same count of digits, this makes the parsing easier for the user. If you want, you can write a mail to whatwg@lists.whatwg.org for clarification. Another point is String::format(). This is not language aware on some posix-systems (point or comma on fractional numbers). See bug 18994. You can also ask Evan Martin for a better solution here. > +String serializedCanvasColor(const Color& color); > + > class CanvasGradient; > class CanvasPattern; > class GraphicsContext; > > class CanvasStyle : public RefCounted<CanvasStyle> { > public: > + static PassRefPtr<CanvasStyle> create(const Color& color) { return adoptRef(new CanvasStyle(color)); } > static PassRefPtr<CanvasStyle> create(const String& color) { return adoptRef(new CanvasStyle(color)); } > static PassRefPtr<CanvasStyle> create(float grayLevel) { return adoptRef(new CanvasStyle(grayLevel)); } > static PassRefPtr<CanvasStyle> create(const String& color, float alpha) { return adoptRef(new CanvasStyle(color, alpha)); } > @@ -45,14 +50,17 @@ namespace WebCore { > static PassRefPtr<CanvasStyle> create(PassRefPtr<CanvasGradient> gradient) { return adoptRef(new CanvasStyle(gradient)); } > static PassRefPtr<CanvasStyle> create(PassRefPtr<CanvasPattern> pattern) { return adoptRef(new CanvasStyle(pattern)); } > > - String color() const { return m_color; } > + String color() const { return serializedCanvasColor(m_color); } > CanvasGradient* canvasGradient() const { return m_gradient.get(); } > CanvasPattern* canvasPattern() const { return m_pattern.get(); } > > void applyFillColor(GraphicsContext*); > void applyStrokeColor(GraphicsContext*); > > + bool isValid() const; ... the complete class shouldn't be indented, please also put serializedCanvasColor after the class definition. The code looks much better. I would like to see Oliver to give some comments on this bug.

(In reply to comment #12) > I'm sorry, I missread the spec. The spec talks about 1 _or more digits_ for fractional part. Nevertheless, if alpha is zero, you just use one digit after the point, if alpha is >0 but <1 you use 5? Shouldn't we have the same count of digits? What is FF and Opera doing here? Maybe we should aline to them here. > In my opinion, the fractional part should allways have the same count of digits, this makes the parsing easier for the user. If you want, you can write a mail to whatwg@lists.whatwg.org for clarification. I copied FF's logic here: "0.0" for transparent, 5 digits otherwise. Just tested on Opera and they return the least possible amount of digits. The other reason I didn't want to use 5 digits for transparent is that some of the HTML5 Canvas tests check for "rgba(0, 0, 0, 0.0)" ;-) > Another point is String::format(). This is not language aware on some posix-systems (point or comma on fractional numbers). See bug 18994. You can also ask Evan Martin for a better solution here. So this will actually break on systems where %f is printed with a comma? Not good. > ... > the complete class shouldn't be indented, please also put serializedCanvasColor after the class definition. I didn't want to mess up svn/git blame, but sure why not? > The code looks much better. I would like to see Oliver to give some comments on this bug. That'd be nice indeed.

Comment on attachment 55797 [details] Proposed patch v4 I see quite a few code changes here without corresponding tests that exercise those functions. Are there already tests covering all these code paths? Which tests? > + * fast/dom/canvasContext2d-element-attribute-js-null-expected.txt: Updated baseline. > + * fast/dom/canvasContext2d-element-attribute-js-null.html: We should move these tests into fast/canvas. Doesn't need to be part of this patch, though. > +PASS ctx.strokeStyle is '#000000' > +PASS ctx.fillStyle is '#000000' > +PASS ctx.shadowColor is 'rgba(0, 0, 0, 0.0)' > +PASS ctx.strokeStyle is '#ff0000' > +PASS ctx.strokeStyle is '#ffffff' > +PASS ctx.strokeStyle is '#ffffff' > +PASS ctx.fillStyle is '#ff0000' > +PASS ctx.fillStyle is '#ffffff' > +PASS ctx.fillStyle is '#ffffff' > +PASS ctx.fillStyle is '#00ff00' > +PASS ctx.shadowColor is '#ff0000' > +PASS ctx.shadowColor is '#ffffff' > +PASS ctx.shadowColor is '#ffffff' > +PASS ctx.shadowColor is 'rgba(1, 2, 3, 0.40000)' The way this test is written does not make very good use of the shouldBe macros. It's always a danger sign when the test output shows only the results and not what is being tested. It's easy to do better by doing this: shouldBe("ctx.strokeStyle = 'red'; ctx.strokeStyle", "'#ff0000'"); Or you can make helper functions. Either way, the test output is a lot clearer when the expression includes the code being tested, not just something that fetches the test result. > + state().m_shadowColor = Color(rgba); There should be no need to write "Color(rgba)" here instead of just "rgba". > - state().m_shadowColor = color; > + state().m_shadowColor = Color(color); Why do some call sites parse colors with CSSParser::parseColor, but this one uses the Color constructor? Could you add test cases to show the different behavior is correct? > + // Default to transparent black > + RGBA32 rgba = state().m_shadowColor.isValid() ? state().m_shadowColor.rgb() : 0; > c->setShadow(IntSize(width, -height), state().m_shadowBlur, Color(colorWithOverrideAlpha(rgba, alpha)), DeviceColorSpace); Color's rgb() function already returns 0 for invalid colors, so there's no need for the extra code here. > RGBA32 rgba = 0; // default is transparent black > - if (!state().m_shadowColor.isEmpty()) > - CSSParser::parseColor(rgba, state().m_shadowColor); > + if (state().m_shadowColor.isValid()) > + rgba = state().m_shadowColor.rgb(); Color already does this 0 default, so we don't need an if statement. > - String m_shadowColor; > + Color m_shadowColor; The real reason to use Color instead of RGBA32 is that Color has a distinct "invalid" value that can be distinguished from transparent black. If we don't need that, then we may wnat to consider using RGBA32 here instead of Color to save storage. > +CanvasStyle::CanvasStyle(const Color& color) We could consider taking an RGBA32 here instead for the same reasons mentioned above. > + // We're only supporting 255 levels of gray here. Since this isn't > + // even part of HTML5, I don't expect anyone will care. If they do > + // we'll make a fancier Color abstraction. I don't think this comment is helpful. It's correct, but doesn't seem to add anything. For one thing, we also only support 255 levels of R, G, B, and A in all the other constructors yet they have no comment. On the other hand, changing the behavior of the existing code to always round colors to 255 gray levels and 255 levels of R, G, B, and A will likely change the behavior on Mac OS X, removing a feature from the canvas on that platform that some applications may depend on. I wonder what the best way to find this out is? > + case RGBA: > + if (m_color.isValid()) > + context->setStrokeColor(m_color.rgb(), DeviceColorSpace); > + break; I don't understand the handling for m_color's invalid state. When would m_color be invalid? Is there a test case that covers this code path? > +bool CanvasStyle::isValid() const > +{ > + switch (m_type) { > + case RGBA: > + return m_color.isValid(); > + default: > + return true; > + } > +} Is this really needed? > + // FIXME: What should we do with invalid colors? > + if (!color.isValid()) > + return String("rgba(0, 0, 0, 0.0)"); And why do we even have the concept of an invalid color in this class? > namespace WebCore { > > +String serializedCanvasColor(const Color& color); > + > class CanvasGradient; > class CanvasPattern; > class GraphicsContext; The function should be indented as the rest of the namespace contents is, and should be after the forward declarations, not before them.

Created attachment 56075 [details] Proposed patch v5 Updated patch: Prettier tests, CanvasStyle now stores an RGBA32 internally.

Comment on attachment 56075 [details] Proposed patch v5 This is looking good. Still needs some work, though. > - , m_shadowColor("black") > + , m_shadowColor(Color::transparent) Is this change correct? Does it behave like this in the other browsers? What does HTML5 say about it? > @@ -814,15 +815,18 @@ void CanvasRenderingContext2D::setShadow(float width, float height, float blur) > { > state().m_shadowOffset = FloatSize(width, height); > state().m_shadowBlur = blur; > - state().m_shadowColor = ""; > + state().m_shadowColor = Color::transparent; > applyShadow(); > } Where's the test case for this? How does it behave in the other browsers? What does HTML5 say about it? I have the same question about many of the setShadow overloads and about clearShadow. I don't see tests that use them and then check the value of the shadowColor property, yet that's the code we're changing. > + , m_valid(true) For boolean members, the WebKit style has been to complete the phrase "style <xxx>". So this would be m_isValid rather than m_valid. > + if ((m_valid = CSSParser::parseColor(m_rgba, color))) > + m_rgba = colorWithOverrideAlpha(m_rgba, alpha); I think this code would be slightly clearer if it used a local variable for the result of parseColor instead of setting m_rgba first to the wrong value and then the right on. I don't think we need to complicate things by adding in the notion of an invalid canvas style. Instead the color parsing from the CanvasStyle constructors that take color strings should move into the CanvasStyle::create functions. Those functions will return 0 if the color cannot be parsed and create a CanvasStyle if the color can be parsed. The CanvasStyle constructors that take color strings would be removed. > + CMYKAValues* m_cmyka; I suggest using an OwnPtr for this instead of writing the delete explicitly. But I also don't see the need to heap allocate this. There are a lot of changes here without test cases -- I'd like to see either new or existing test cases that cover all the modified functions. review- mainly because we need better test coverage

(In reply to comment #16) > > - , m_shadowColor("black") > > + , m_shadowColor(Color::transparent) > > Is this change correct? Does it behave like this in the other browsers? What does HTML5 say about it? From HTML5 section 4.8.11.1.6: "When the context is created, the shadowColor attribute initially must be fully-transparent black."

Created attachment 56129 [details] Proposed patch v6 Updated patch with comments addressed.

Created attachment 56131 [details] Proposed patch v7 Updated patch - v6 missed one instance of "...would be slightly clearer if it used a local variable for the result of parseColor instead of setting m_rgba first to the wrong value and then the right one..."

(In reply to comment #17) > (In reply to comment #16) > > > - , m_shadowColor("black") > > > + , m_shadowColor(Color::transparent) > > > > Is this change correct? Does it behave like this in the other browsers? What does HTML5 say about it? > > From HTML5 section 4.8.11.1.6: > "When the context is created, the shadowColor attribute initially must be fully-transparent black." Did you check against other browsers -- this change seems odd, as it would mean shadow colour needs to be set before you get shadows and i'm 95% sure that isn't needed in browsers currently.

(In reply to comment #20) > > From HTML5 section 4.8.11.1.6: > > "When the context is created, the shadowColor attribute initially must be fully-transparent black." > > Did you check against other browsers -- this change seems odd, as it would mean shadow colour needs to be set before you get shadows and i'm 95% sure that isn't needed in browsers currently. Yes, both Opera and Firefox return "rgba(0, 0, 0, 0.0)" for "document.createElement('canvas').getContext('2d').shadowColor"

*** Bug 39161 has been marked as a duplicate of this bug. ***

Created attachment 56334 [details] Proposed patch v8 Moved color serialization into the Color class - String Color::serialized() const; (I liked that in the patch for bug 39161) Other than that, this is the same patch as earlier.

(In reply to comment #23) > Moved color serialization into the Color class - String Color::serialized() const; Is this the one and only style of color serialization? Generally speaking I don't think the platform layer files, like Color, should have code that is specific to a particular part of the HTML5 specification. If the serialization rules are specific to HTML then I prefer the old way this code was divided between the source files.

(In reply to comment #24) > (In reply to comment #23) > > Moved color serialization into the Color class - String Color::serialized() const; > > Is this the one and only style of color serialization? Generally speaking I don't think the platform layer files, like Color, should have code that is specific to a particular part of the HTML5 specification. If the serialization rules are specific to HTML then I prefer the old way this code was divided between the source files. On the other hand, if the Color.h file already has the parsing that is the flip side of this serialization code, then I suppose it's good to have both in the same place. We should keep them together, whether we leave them in the platform code or move them into the HTML code.

(In reply to comment #25) > On the other hand, if the Color.h file already has the parsing that is the flip side of this serialization code, then I suppose it's good to have both in the same place. We should keep them together, whether we leave them in the platform code or move them into the HTML code. I agree. Any other thoughts on the patch? :-)

Comment on attachment 56334 [details] Proposed patch v8 > CanvasStyle::CanvasStyle(PassRefPtr<CanvasGradient> gradient) > - : m_type(gradient ? Gradient : ColorString) > + : m_type(gradient ? Gradient : RGBA) > , m_gradient(gradient) > { > } This will create a style with an uninitialized m_rgba with type RGBA. That's not good. The old code would have a null color string, not an uninitialized color string. Instead, I suggest we put the null check in the CanvasStyle::create function and return 0 in that case. Also, I'd like to see test cases for this. > CanvasStyle::CanvasStyle(PassRefPtr<CanvasPattern> pattern) > - : m_type(pattern ? ImagePattern : ColorString) > + : m_type(pattern ? ImagePattern : RGBA) > , m_pattern(pattern) > { > } Same thing. Everything else looks fine to me.

Created attachment 56486 [details] Proposed patch v9 Updated patch with null checks moved to create(). AFAICT there is no way to cause the CanvasStyle(Gradient/Pattern) constructors to be called with invalid arguments, so I don't know how to make a test for that.

Comment on attachment 56486 [details] Proposed patch v9 If we don't need to allow null arguments to the CanvasStyle create functions, we could remove the "return 0" code and just have an assertion instead. I can't think of a good reason to have dead code for non-testable code paths.

I will help to commit the patch manually.

Comment on attachment 56486 [details] Proposed patch v9 committed manually, r59768. remove flags.

http://trac.webkit.org/changeset/59768 might have broken Qt Linux ARMv5 Release

(In reply to comment #32) > http://trac.webkit.org/changeset/59768 might have broken Qt Linux ARMv5 Release No, it isn't. It was a false positive alarm because of slave lost. Sorry.