Issue 144463014: Add [StrictTypeChecking] to the SVGGeometryElement interface

Issue 144463014: Add [StrictTypeChecking] to the SVGGeometryElement interface (Closed)

Created:
6 years, 10 months ago by fs

Modified:
6 years, 10 months ago

Reviewers:
Nils Barth (inactive), pdr., f(malita), Stephen Chennney, kouhei (in TOK)

CC:
blink-reviews, krit, kojih, arv+blink, jsbell+bindings_chromium.org, sof, rwlbuis, fs, abarth-chromium, marja+watch_chromium.org, ed+blinkwatch_opera.com, pdr., f(malita), adamk+blink_chromium.org, gyuyoung.kim_webkit.org, Stephen Chennney, Nate Chapin, watchdog-blink-watchlist_google.com, Inactive

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Add [StrictTypeChecking] to the SVGGeometryElement interface Apply strict type checking to the SVGGeometryElement interface. Fix an issue with the [StrictTypeChecking] attribute, where 'undefined' would be accepted when it shouldn't have been. This made the explicit checking in SVGTransformListPropertyTearOff unnecessary, so remove (and update test with the new expected result.) BUG=341810 BUG=342741 BUG=341379 BUG=341620 BUG=345199 BUG=341030 BUG=341776 BUG=344474 BUG=345191 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167003

Patch Set 1 #

Total comments: 10

Patch Set 2 : Drop/change TC; Simplify generated code. #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+48 lines, -32 lines)			Patch
M	LayoutTests/svg/dom/SVGTransformList-expected.txt	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/svg/dom/undefined-null.html	View	1	5 chunks	+11 lines, -11 lines	0 comments	Download
M	LayoutTests/svg/dom/undefined-null-expected.txt	View	1	2 chunks	+11 lines, -0 lines	0 comments	Download
M	Source/bindings/scripts/code_generator_v8.pm	View	1	1 chunk	+15 lines, -8 lines	0 comments	Download
M	Source/bindings/templates/methods.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfacePython.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestObjectPython.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/tests/results/V8TestSVG.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGGeometryElement.idl	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/svg/SVGTransformList.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/properties/SVGTransformListPropertyTearOff.h	View		1 chunk	+1 line, -5 lines	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

Stephen Chennney

LGTM for SVG code and IDLs but I believe you'll also need an owner for ...

6 years, 10 months ago (2014-02-11 16:35:21 UTC) #2

haraken

On 2014/02/11 16:35:21, Stephen Chenney wrote: > LGTM for SVG code and IDLs but I ...

6 years, 10 months ago (2014-02-11 16:36:35 UTC) #3

Nils Barth (inactive)

LGTM for bindings - thanks for handling this! (Had a suggestion for comment, but code ...

6 years, 10 months ago (2014-02-12 01:15:22 UTC) #4

Nils Barth (inactive)

https://codereview.chromium.org/144463014/diff/1/Source/bindings/scripts/code_generator_v8.pm File Source/bindings/scripts/code_generator_v8.pm (right): https://codereview.chromium.org/144463014/diff/1/Source/bindings/scripts/code_generator_v8.pm#newcode2785 Source/bindings/scripts/code_generator_v8.pm:2785: # If the "StrictTypeChecking" extended attribute is present, and ...

6 years, 10 months ago (2014-02-12 01:15:44 UTC) #5

Nils Barth (inactive)

A few JS style points too. https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeometryElement.html File LayoutTests/svg/dom/SVGGeometryElement.html (right): https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeometryElement.html#newcode1 LayoutTests/svg/dom/SVGGeometryElement.html:1: <!doctype html> Style ...

6 years, 10 months ago (2014-02-12 03:29:51 UTC) #6

Nils Barth (inactive)

Hi Florin, As this CL fixes many bugs, I've first created a test case in ...

6 years, 10 months ago (2014-02-12 03:51:48 UTC) #7

Nils Barth (inactive)

On 2014/02/12 03:51:48, Nils Barth wrote: > Hi Florin, I mean: Hi Fredrik, (^_^;)

6 years, 10 months ago (2014-02-12 07:19:07 UTC) #8

Dropped my TC (after fixing the style issue naturally ;-)) in favor of the one ...

6 years, 10 months ago (2014-02-12 10:07:26 UTC) #9

Dropped my TC (after fixing the style issue naturally ;-)) in favor of the one
added by https://codereview.chromium.org/159973005/. Simplified the generated
code a tad (dropping the redundant part of the disjunction.)

https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeome...
File LayoutTests/svg/dom/SVGGeometryElement.html (right):

https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeome...
LayoutTests/svg/dom/SVGGeometryElement.html:1: <!doctype html>
On 2014/02/12 03:29:51, Nils Barth wrote:
> Style notes:
> http://www.w3.org/TR/html5/syntax.html
> http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml
> <!DOCTYPE html>
> (it's case insensitive, but conventionally uppercase.)

Done.

https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeome...
LayoutTests/svg/dom/SVGGeometryElement.html:3: <svg><rect/></svg>
On 2014/02/12 03:29:51, Nils Barth wrote:
> <rect></rect> not <rect/>
> (no self-closing tags in HTML5, so <rect/> is equivalent to <rect>,
> and <rect> should be closed, since it otherwise can have contents;
> I don't believe its end tag is optional.)

Done.

https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeome...
LayoutTests/svg/dom/SVGGeometryElement.html:4: <script
src=../../resources/js-test.js></script>
On 2014/02/12 03:29:51, Nils Barth wrote:
> We conventionally quote this, with double quotes.
> Also usually put this script in the head.

Done.

https://codereview.chromium.org/144463014/diff/1/LayoutTests/svg/dom/SVGGeome...
LayoutTests/svg/dom/SVGGeometryElement.html:7:
shouldThrow("rect.isPointInFill(null)");
On 2014/02/12 03:29:51, Nils Barth wrote:
> Single quotes in JavaScript.

Done.

https://codereview.chromium.org/144463014/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/144463014/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:2785: # If the "StrictTypeChecking"
extended attribute is present, and the argument's type is an
On 2014/02/12 01:15:45, Nils Barth wrote:
> Could you rewrite this comment to be explicit about what happens for undefined
> and null?
> 
> # If the [StrictTypeChecking] extended attribute is present, type
> # check interface type arguments for correct type and nullability.
> #
> # If the argument is passed, and is not |undefined| or |null|, then
> # it must implement the interface type, otherwise throw a TypeError
> # If the parameter is nullable, then both |undefined| and |null|
> # pass a NULL pointer to the C++ code, otherwise these also throw.
> # Without [StrictTypeChecking], in all these cases NULL is silently
> # passed to the C++ code.
> #
> # Per the Web IDL and ECMAScript specifications, incoming values
> # can always be converted to primitive types and strings (including
> # |undefined| and |null|), so do not throw TypeError for these.

Done.

Nils Barth (inactive)

Thanks for fixes! On 2014/02/12 10:07:26, fs wrote: > Simplified the generated code a tad ...

6 years, 10 months ago (2014-02-12 10:49:15 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/144463014/120001

6 years, 10 months ago (2014-02-12 11:48:13 UTC) #12

Nils Barth (inactive)

6 years, 10 months ago (2014-02-13 06:21:26 UTC) #14

Message was sent while issue was closed.

BTW, I've written documentation about |undefined| and |null| at:
https://sites.google.com/a/chromium.org/dev/blink/webidl#TOC-undefined-and-null

Expand Messages | Collapse Messages