Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
237743007:
Handle the DOMString case of CRC2D.fillStyle/strokeStyle correctly (Closed)
|
Created:
6 years, 8 months ago by fs Modified:
6 years, 8 months ago CC:
blink-reviews, Nils Barth (inactive), arv+blink, jsbell+bindings_chromium.org, sof, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, Inactive Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionHandle the DOMString case of CRC2D.fillStyle/strokeStyle correctly
Non-CanvasGradient/CanvasPattern objects would not be ToString(v)'d and
have the result assigned/parsed as a color-string.
Implement this by checking for CanvasGradient/CanvasPattern first,
and then invole ToString() on the value.
BUG=364008
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171852
Patch Set 1 #
Total comments: 6
Patch Set 2 : Simplify. #Patch Set 3 : Add tests for numeric values; Simplify tests. #Patch Set 4 : Add tests for objects without (explicit) toString. #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } I'm not sure if we need separate branches for IsString() and IsObject(). Won't the following code work? if (RefPtr<CanvasStyle> canvasStyle = toCanvasStyle(value, info.GetIsolate())) { impl->setStrokeStyle(canvasStyle); } else { TOSTRING_VOID(V8StringResource<>, colorString, value); impl->setStrokeColor(colorString); } ? https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:103: } Ditto.
https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } On 2014/04/16 14:18:27, haraken wrote: > > I'm not sure if we need separate branches for IsString() and IsObject(). Won't > the following code work? > > if (RefPtr<CanvasStyle> canvasStyle = toCanvasStyle(value, info.GetIsolate())) { > impl->setStrokeStyle(canvasStyle); > } else { > TOSTRING_VOID(V8StringResource<>, colorString, value); > impl->setStrokeColor(colorString); > } > > ? Yes, that should work. I wasn't sure if the way it's currently was due to some (micro)optimization or not (there seemed to be quite few in this area). Maybe Justin knows something? Personally, I'd happily switch to the suggested version (which I hope is close to what the union implementation would yield).
https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } On 2014/04/16 14:29:57, fs wrote: > On 2014/04/16 14:18:27, haraken wrote: > > > > I'm not sure if we need separate branches for IsString() and IsObject(). Won't > > the following code work? > > > > if (RefPtr<CanvasStyle> canvasStyle = toCanvasStyle(value, info.GetIsolate())) > { > > impl->setStrokeStyle(canvasStyle); > > } else { > > TOSTRING_VOID(V8StringResource<>, colorString, value); > > impl->setStrokeColor(colorString); > > } > > > > ? > > Yes, that should work. I wasn't sure if the way it's currently was due to some > (micro)optimization or not (there seemed to be quite few in this area). Maybe > Justin knows something? Personally, I'd happily switch to the suggested version > (which I hope is close to what the union implementation would yield). Uploaded that as PS2.
https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } V8StringResource::prepareBase starts by checking whether the cast is from another string and it exits immediately in that case, so there is not much to be micro-optimized other than a redundant isEmpty() check.
On 2014/04/16 15:16:18, junov wrote: > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): > > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } > V8StringResource::prepareBase starts by checking whether the cast is from > another string and it exits immediately in that case, so there is not much to be > micro-optimized other than a redundant isEmpty() check. I believe this change will also allow an implicit conversion from a number to a string. I am not sure whether the Bison parser will consistently accept or reject numbers converted to strings as color values. We may need to handle that case explicitly in the custom binding. We definitely need to add that case to the layout test. I'll do some more digging to figure this out...
https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } On 2014/04/16 15:16:18, junov wrote: > V8StringResource::prepareBase starts by checking whether the cast is from > another string and it exits immediately in that case, so there is not much to be > micro-optimized other than a redundant isEmpty() check. Before that you have the two toNativeWithTypeCheck() calls, and the calls through CanvasStyle::createFrom... though.
On 2014/04/16 15:24:10, junov wrote: > On 2014/04/16 15:16:18, junov wrote: > > > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > > File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): > > > > > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > > Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } > > V8StringResource::prepareBase starts by checking whether the cast is from > > another string and it exits immediately in that case, so there is not much to > be > > micro-optimized other than a redundant isEmpty() check. > > I believe this change will also allow an implicit conversion from a number to a > string. I am not sure whether the Bison parser will consistently accept or > reject numbers converted to strings as color values. We may need to handle that > case explicitly in the custom binding. We definitely need to add that case to > the layout test. I'll do some more digging to figure this out... Theoretically that's the same as something like: data:text/html,<script>c = document.createElement('canvas'); (ctx = c.getContext('2d')).fillStyle = '800000'; document.write(ctx.fillStyle);</script> though. (But it does end up stringifying to a "valid" color - i.e. quirks mode.)
On 2014/04/16 15:43:20, fs wrote: ... > though. (But it does end up stringifying to a "valid" color - i.e. quirks mode.) (In the case of 800000 being assigned as a number that is.)
On 2014/04/16 15:30:19, fs wrote: > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): > > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } > On 2014/04/16 15:16:18, junov wrote: > > V8StringResource::prepareBase starts by checking whether the cast is from > > another string and it exits immediately in that case, so there is not much to > be > > micro-optimized other than a redundant isEmpty() check. > > Before that you have the two toNativeWithTypeCheck() calls, and the calls > through CanvasStyle::createFrom... though. Right. And we probably use color strings more often that patterns and gradients... If we really want to care about that, we could try converting to a string first: TOSTRING_VOID(V8StringResource<>, colorString, value); if (!colorString.isEmpty()) { impl->setFillColor(toCoreString(value.As<v8::String>())); } else if (RefPtr<CanvasStyle> canvasStyle = toCanvasStyle(value, info.GetIsolate())) { impl->setFillStyle(canvasStyle); } But I think we are over-optimizing now. About the implicit conversion from numbers, I think we are OK. No number will converted to a string will match a parsable CSS color format. But you should still add that to the test. You should also try testing objects that do not implement toString, and verify that the style stays set to its previous value without throwing any exceptions.
On 2014/04/16 15:49:09, junov wrote: > On 2014/04/16 15:30:19, fs wrote: > > > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > > File Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp (right): > > > > > https://codereview.chromium.org/237743007/diff/1/Source/bindings/v8/custom/V8... > > Source/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:84: } > > On 2014/04/16 15:16:18, junov wrote: > > > V8StringResource::prepareBase starts by checking whether the cast is from > > > another string and it exits immediately in that case, so there is not much > to > > be > > > micro-optimized other than a redundant isEmpty() check. > > > > Before that you have the two toNativeWithTypeCheck() calls, and the calls > > through CanvasStyle::createFrom... though. > > Right. And we probably use color strings more often that patterns and > gradients... If we really want to care about that, we could try converting to a > string first: > > TOSTRING_VOID(V8StringResource<>, colorString, value); If value is a CanvasGradient though you'll get "[object CanvasGradient]" (or something like that) after this step though, so that doesn't work. > But I think we are over-optimizing now. Yes. > About the implicit conversion from numbers, I think we are OK. No number will > converted to a string will match a parsable CSS color format. But you should > still add that to the test. You should also try testing objects that do not > implement toString, and verify that the style stays set to its previous value > without throwing any exceptions. Actually, '800000' (or similar) does end up parsing as a color value, because: static bool parseColor(RGBA32& color, const String&, bool strict = false); and because strict==false, the quirks form is accepted. (Try the TC/data URI in my previous message.)
On 2014/04/16 15:57:44, fs wrote: > On 2014/04/16 15:49:09, junov wrote: > > On 2014/04/16 15:30:19, fs wrote: .... > > About the implicit conversion from numbers, I think we are OK. No number will > > converted to a string will match a parsable CSS color format. But you should > > still add that to the test. You should also try testing objects that do not > > implement toString, and verify that the style stays set to its previous value > > without throwing any exceptions. > > Actually, '800000' (or similar) does end up parsing as a color value, because: > > static bool parseColor(RGBA32& color, const String&, bool strict = false); > > and because strict==false, the quirks form is accepted. (Try the TC/data URI in > my previous message.) ...and I didn't mean to say that your statement was incorrect, but rather that the current behavior is to accept string like the one above. (Which for instance Gecko doesn't)
On 2014/04/16 15:59:27, fs wrote: > On 2014/04/16 15:57:44, fs wrote: > > On 2014/04/16 15:49:09, junov wrote: > > > On 2014/04/16 15:30:19, fs wrote: > .... > > > About the implicit conversion from numbers, I think we are OK. No number > will > > > converted to a string will match a parsable CSS color format. But you > should > > > still add that to the test. You should also try testing objects that do not > > > implement toString, and verify that the style stays set to its previous > value > > > without throwing any exceptions. > > > > Actually, '800000' (or similar) does end up parsing as a color value, because: > > > > static bool parseColor(RGBA32& color, const String&, bool strict = false); > > > > and because strict==false, the quirks form is accepted. (Try the TC/data URI > in > > my previous message.) > > ...and I didn't mean to say that your statement was incorrect, but rather that > the current behavior is to accept string like the one above. (Which for instance > Gecko doesn't) Uploaded https://codereview.chromium.org/237743009 which should make color parsing strict.
On 2014/04/16 16:16:14, fs wrote: > On 2014/04/16 15:59:27, fs wrote: > > On 2014/04/16 15:57:44, fs wrote: > > > On 2014/04/16 15:49:09, junov wrote: > > > > On 2014/04/16 15:30:19, fs wrote: > > .... > > > > About the implicit conversion from numbers, I think we are OK. No number > > will > > > > converted to a string will match a parsable CSS color format. But you > > should > > > > still add that to the test. You should also try testing objects that do > not > > > > implement toString, and verify that the style stays set to its previous > > value > > > > without throwing any exceptions. > > > > > > Actually, '800000' (or similar) does end up parsing as a color value, > because: > > > > > > static bool parseColor(RGBA32& color, const String&, bool strict = > false); > > > > > > and because strict==false, the quirks form is accepted. (Try the TC/data URI > > in > > > my previous message.) > > > > ...and I didn't mean to say that your statement was incorrect, but rather that > > the current behavior is to accept string like the one above. (Which for > instance > > Gecko doesn't) > > Uploaded https://codereview.chromium.org/237743009 which should make color > parsing strict. Great! lgtm
On 2014/04/16 15:49:09, junov wrote: > On 2014/04/16 15:30:19, fs wrote: ... > About the implicit conversion from numbers, I think we are OK. No number will > converted to a string will match a parsable CSS color format. But you should > still add that to the test. You should also try testing objects that do not > implement toString, and verify that the style stays set to its previous value > without throwing any exceptions. Added test for number and object w/o (explicit) toString.
On 2014/04/16 16:27:37, junov wrote: > On 2014/04/16 16:16:14, fs wrote: > > On 2014/04/16 15:59:27, fs wrote: > > > On 2014/04/16 15:57:44, fs wrote: > > > > On 2014/04/16 15:49:09, junov wrote: > > > > > On 2014/04/16 15:30:19, fs wrote: > > > .... > > > > > About the implicit conversion from numbers, I think we are OK. No number > > > will > > > > > converted to a string will match a parsable CSS color format. But you > > > should > > > > > still add that to the test. You should also try testing objects that do > > not > > > > > implement toString, and verify that the style stays set to its previous > > > value > > > > > without throwing any exceptions. > > > > > > > > Actually, '800000' (or similar) does end up parsing as a color value, > > because: > > > > > > > > static bool parseColor(RGBA32& color, const String&, bool strict = > > false); > > > > > > > > and because strict==false, the quirks form is accepted. (Try the TC/data > URI > > > in > > > > my previous message.) > > > > > > ...and I didn't mean to say that your statement was incorrect, but rather > that > > > the current behavior is to accept string like the one above. (Which for > > instance > > > Gecko doesn't) > > > > Uploaded https://codereview.chromium.org/237743009 which should make color > > parsing strict. > > Great! lgtm Wait, add the -expected.txt file
On 2014/04/16 16:28:42, junov wrote: > On 2014/04/16 16:27:37, junov wrote: > > On 2014/04/16 16:16:14, fs wrote: > > > On 2014/04/16 15:59:27, fs wrote: ... > > > Uploaded https://codereview.chromium.org/237743009 which should make color > > > parsing strict. > > > > Great! lgtm > > Wait, add the -expected.txt file It's not needed (for testharness tests, if all the tests pass - which they will in this case when https://codereview.chromium.org/237743009/ lands.) If you prefer to have it I can add it though.
On 2014/04/16 16:39:15, fs wrote: > On 2014/04/16 16:28:42, junov wrote: > > On 2014/04/16 16:27:37, junov wrote: > > > On 2014/04/16 16:16:14, fs wrote: > > > > On 2014/04/16 15:59:27, fs wrote: > ... > > > > Uploaded https://codereview.chromium.org/237743009 which should make color > > > > parsing strict. > > > > > > Great! lgtm > > > > Wait, add the -expected.txt file > > It's not needed (for testharness tests, if all the tests pass - which they will > in this case when https://codereview.chromium.org/237743009/ lands.) If you > prefer to have it I can add it though. Nah, its fine lgtm.
On 2014/04/16 16:55:46, junov wrote: > On 2014/04/16 16:39:15, fs wrote: ... > > It's not needed (for testharness tests, if all the tests pass - which they > will > > in this case when https://codereview.chromium.org/237743009/ lands.) If you > > prefer to have it I can add it though. > > Nah, its fine lgtm. Ok. I hope it's fine w/ haraken too now (needed to approve bindings/).
LGTM!
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/237743007/50001
Message was sent while issue was closed.
Change committed as 171852 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
