Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15138002:
Added tests to previously broken functionality and added null checks. (Closed)
|
Created:
7 years, 7 months ago by Andrei Mouravski Modified:
7 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdded tests to previously broken functionality and added null checks.
R=antonm@google.com
Committed: https://code.google.com/p/dart/source/detail?r=23123
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 8
Patch Set 10 : Correctly check for position. #
Total comments: 2
Patch Set 11 : Contradiction! #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : I think this is good now. #
Total comments: 9
Patch Set 15 : #
Total comments: 1
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/15138002/diff/2001/sdk/lib/html/dart2js/html_... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/2001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:20733: if (protocol_OR_protocols is List<String> && protocol_OR_protocols != null && url is String) { why are the "url is String" checks here- it's typed and it's not optional. https://codereview.chromium.org/15138002/diff/2001/sdk/lib/html/dartium/html_... File sdk/lib/html/dartium/html_dartium.dart (left): https://codereview.chromium.org/15138002/diff/2001/sdk/lib/html/dartium/html_... sdk/lib/html/dartium/html_dartium.dart:1413: - if ((canvas_OR_image is CanvasElement || canvas_OR_image == null) && (repetitionType is String || repetitionType == null)) { This was allowing canvas_OR_image to be null, now it throws an exception if it is null. Why? https://codereview.chromium.org/15138002/diff/2001/sdk/lib/indexed_db/dart2js... File sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/2001/sdk/lib/indexed_db/dart2js... sdk/lib/indexed_db/dart2js/indexed_db_dart2js.dart:907: if (keyPath is List<String> && keyPath != null) { keyPath is List<String> && keyPath != null is overly redundant. https://codereview.chromium.org/15138002/diff/2001/sdk/lib/web_audio/dartium/... File sdk/lib/web_audio/dartium/web_audio_dartium.dart (right): https://codereview.chromium.org/15138002/diff/2001/sdk/lib/web_audio/dartium/... sdk/lib/web_audio/dartium/web_audio_dartium.dart:199: if (when is num && !?grainOffset && !?grainDuration) { when is when not a num? https://codereview.chromium.org/15138002/diff/2001/sdk/lib/web_gl/dart2js/web... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/2001/sdk/lib/web_gl/dart2js/web... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2408: void texImage2D(int target, int level, int internalformat, int format_OR_width, int height_OR_type, border_OR_canvas_OR_image_OR_pixels_OR_video, [int format, int type, TypedData pixels]) { Which reminds me, would be great to fix https://code.google.com/p/dart/issues/detail?id=8489. These are just senseless overloads.
Okay, this is much smaller and should be better. https://codereview.chromium.org/15138002/diff/2001/sdk/lib/html/dart2js/html_... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/2001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:20733: if (protocol_OR_protocols is List<String> && protocol_OR_protocols != null && url is String) { On 2013/05/13 21:09:16, blois wrote: > why are the "url is String" checks here- it's typed and it's not optional. Because that's what the code does? It was being checked before, too.
https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2409: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && pixels != null) { IDL has pixels as: ArrayBufferView? pixels Is it not nullable? (asking, I don't know what the ? means).
https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2409: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && pixels != null) { On 2013/05/14 01:47:56, blois wrote: > IDL has pixels as: > ArrayBufferView? pixels > > Is it not nullable? (asking, I don't know what the ? means). No idea, that was put in because pixels is optional.
https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2409: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && pixels != null) { On 2013/05/14 01:58:56, Andrei Mouravski wrote: > On 2013/05/14 01:47:56, blois wrote: > > IDL has pixels as: > > ArrayBufferView? pixels > > > > Is it not nullable? (asking, I don't know what the ? means). > > No idea, that was put in because pixels is optional. I have seen some Blink IDL that has nullable parameters where w3 does not. Someone should ping someone in Blink to ask why.
https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2409: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && pixels != null) { ? means nullable that is null is acceptable value. We really need to check if passing null and not passing an argument here is the same. On 2013/05/14 01:58:56, Andrei Mouravski wrote: > On 2013/05/14 01:47:56, blois wrote: > > IDL has pixels as: > > ArrayBufferView? pixels > > > > Is it not nullable? (asking, I don't know what the ? means). > > No idea, that was put in because pixels is optional. https://codereview.chromium.org/15138002/diff/11001/tests/html/canvasrenderin... File tests/html/canvasrenderingcontext2d_test.dart (right): https://codereview.chromium.org/15138002/diff/11001/tests/html/canvasrenderin... tests/html/canvasrenderingcontext2d_test.dart:184: drawnData.data[0] = 25; I might be overvaluing cascade syntax, but something like drawnData.data ..[0] = ... ..[1] = ... might be an interesting alternative. Up to you though. https://codereview.chromium.org/15138002/diff/11001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/11001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:250: if can_omit_type_check(test_type, i): why if, looks like it should be rather else:, see elif at line 246 in patched version. https://codereview.chromium.org/15138002/diff/11001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:252: if argument not in signature: this check looks incorrect: you most probably want to check that given argument is not present at the given position. Overall, why this check wasn't needed before and is needed now?
PTAL. I just want to be done with this stupid CL. :[ No solution has been perfect, and to get things perfect is going to take a bunch of refactoring, which is totally doable, but let's get rid of ? first. https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/11001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2409: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && pixels != null) { On 2013/05/14 01:47:56, blois wrote: > IDL has pixels as: > ArrayBufferView? pixels > > Is it not nullable? (asking, I don't know what the ? means). Done. https://codereview.chromium.org/15138002/diff/11001/tests/html/canvasrenderin... File tests/html/canvasrenderingcontext2d_test.dart (right): https://codereview.chromium.org/15138002/diff/11001/tests/html/canvasrenderin... tests/html/canvasrenderingcontext2d_test.dart:184: drawnData.data[0] = 25; Not going to worry about it for now. On 2013/05/14 06:17:34, Anton Muhin wrote: > I might be overvaluing cascade syntax, but something like > > drawnData.data > ..[0] = ... > ..[1] = ... > > might be an interesting alternative. Up to you though. https://codereview.chromium.org/15138002/diff/11001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/11001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:252: if argument not in signature: Done. This check wasn't here before because... nobody thought to check this case? On 2013/05/14 06:17:34, Anton Muhin wrote: > this check looks incorrect: you most probably want to check that given argument > is not present at the given position. > > Overall, why this check wasn't needed before and is needed now?
https://codereview.chromium.org/15138002/diff/27001/sdk/lib/indexed_db/dartiu... File sdk/lib/indexed_db/dartium/indexed_db_dartium.dart (right): https://codereview.chromium.org/15138002/diff/27001/sdk/lib/indexed_db/dartiu... sdk/lib/indexed_db/dartium/indexed_db_dartium.dart:488: if (?key && key != null) { why there are both checks? https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:243: nullable = False or argument.type.nullable False or? https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:252: if nullable is False: nit: if not nullable https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:254: if (argument not in signature or (len(signature) > i and why still argument not in signagture check
PTAL. https://codereview.chromium.org/15138002/diff/27001/sdk/lib/indexed_db/dartiu... File sdk/lib/indexed_db/dartium/indexed_db_dartium.dart (right): https://codereview.chromium.org/15138002/diff/27001/sdk/lib/indexed_db/dartiu... sdk/lib/indexed_db/dartium/indexed_db_dartium.dart:488: if (?key && key != null) { Because removing the question mark is in the next CL. This currently is not a problem since the other case is true when key is null. On 2013/05/21 13:24:08, Anton Muhin wrote: > why there are both checks? https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:243: nullable = False or argument.type.nullable I am not a smart person... :-) On 2013/05/21 13:24:08, Anton Muhin wrote: > False or? https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:252: if nullable is False: On 2013/05/21 13:24:08, Anton Muhin wrote: > nit: if not nullable Done. https://codereview.chromium.org/15138002/diff/27001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:254: if (argument not in signature or (len(signature) > i and On 2013/05/21 13:24:08, Anton Muhin wrote: > why still argument not in signagture check Done.
https://codereview.chromium.org/15138002/diff/32001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/32001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2405: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && format != null && type != null) { why do format_OR_width, height_OR_type need to be checked?
Okay, this should for reals be good. https://codereview.chromium.org/15138002/diff/32001/sdk/lib/web_gl/dart2js/we... File sdk/lib/web_gl/dart2js/web_gl_dart2js.dart (right): https://codereview.chromium.org/15138002/diff/32001/sdk/lib/web_gl/dart2js/we... sdk/lib/web_gl/dart2js/web_gl_dart2js.dart:2405: if (format_OR_width != null && height_OR_type != null && (border_OR_canvas_OR_image_OR_pixels_OR_video is int || border_OR_canvas_OR_image_OR_pixels_OR_video == null) && format != null && type != null) { They don't. I just couldn't figure out why they were like that. :[ On 2013/05/21 23:53:07, blois wrote: > why do format_OR_width, height_OR_type need to be checked?
https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:257: if (len(signature) <= i or signature[i].id not in parameter_name): in parameter_name or == parameter_name? https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:259: checks.append('%s != null' % parameter_name) still, I do not understand why this check is needed.
https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:257: if (len(signature) <= i or signature[i].id not in parameter_name): This is to handle the case of merged types. On 2013/05/22 12:27:33, Anton Muhin wrote: > in parameter_name or == parameter_name? https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:259: checks.append('%s != null' % parameter_name) Because other logic does some dumb things and if this isnt here then the "incorrect arguments error" is never thrown. On 2013/05/22 12:27:33, Anton Muhin wrote: > still, I do not understand why this check is needed.
https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:257: if (len(signature) <= i or signature[i].id not in parameter_name): I am really concerned with the cases like (hypothetic): foo(a) foo(data) On 2013/05/22 12:37:42, Andrei Mouravski wrote: > This is to handle the case of merged types. > > On 2013/05/22 12:27:33, Anton Muhin wrote: > > in parameter_name or == parameter_name? > https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:259: checks.append('%s != null' % parameter_name) May you, please, provide an exact example: which case of overloading this logic resolves? Just to elaborate, it looks like logic under this if actually belongs to block at ln. 249 of the current snapshot. On 2013/05/22 12:37:42, Andrei Mouravski wrote: > Because other logic does some dumb things and if this isnt here then the > "incorrect arguments error" is never thrown. > > On 2013/05/22 12:27:33, Anton Muhin wrote: > > still, I do not understand why this check is needed. >
https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:257: if (len(signature) <= i or signature[i].id not in parameter_name): Agreed. I'll run a regex instead or else split on _OR_ On 2013/05/22 12:50:56, Anton Muhin wrote: > I am really concerned with the cases like (hypothetic): > > foo(a) > foo(data) > > On 2013/05/22 12:37:42, Andrei Mouravski wrote: > > This is to handle the case of merged types. > > > > On 2013/05/22 12:27:33, Anton Muhin wrote: > > > in parameter_name or == parameter_name? > > > https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:259: checks.append('%s != null' % parameter_name) Please take a look at the tests added in this CL. You can even prove it to yourself by patching in JUST the tests and watching them fail. The location of this code is not great, but this whole method is a little wonky. I will try to redactor today. On 2013/05/22 12:50:56, Anton Muhin wrote: > May you, please, provide an exact example: which case of overloading this logic > resolves? > > Just to elaborate, it looks like logic under this if actually belongs to block > at ln. 249 of the current snapshot. > > On 2013/05/22 12:37:42, Andrei Mouravski wrote: > > Because other logic does some dumb things and if this isnt here then the > > "incorrect arguments error" is never thrown. > > > > On 2013/05/22 12:27:33, Anton Muhin wrote: > > > still, I do not understand why this check is needed. > > >
https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/50001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:257: if (len(signature) <= i or signature[i].id not in parameter_name): On 2013/05/22 14:40:10, Andrei Mouravski wrote: > Agreed. I'll run a regex instead or else split on _OR_ > > On 2013/05/22 12:50:56, Anton Muhin wrote: > > I am really concerned with the cases like (hypothetic): > > > > foo(a) > > foo(data) > > > > On 2013/05/22 12:37:42, Andrei Mouravski wrote: > > > This is to handle the case of merged types. > > > > > > On 2013/05/22 12:27:33, Anton Muhin wrote: > > > > in parameter_name or == parameter_name? > > > > > > Done.
https://codereview.chromium.org/15138002/diff/58001/tools/dom/scripts/htmldar... File tools/dom/scripts/htmldartgenerator.py (right): https://codereview.chromium.org/15138002/diff/58001/tools/dom/scripts/htmldar... tools/dom/scripts/htmldartgenerator.py:237: def IsDartTypeNullable(type_signature): Andrei, thanks a lot, now I understand what is the problem. However, my feeling is it should be addressed somewhat differently. First of all, I'd kindly ask you to use ?-operator for presence checks: nullability is rather tricky thing and should be addressed for all ?-operators in one go and in completely separate CL. Secondly, if my understanding is correct, all you want is to insert presence check if argument is optional in Dart declaration, but required by the given signature, correct? Example: foo(var x) foo(var x, var y, var z) We'd like to have something like: if (?x && !?y && !?z) { ... } if (?x && ?y && ?z) { ... } as Dart signature would be foo(var x, [var y, var z]). I'd suggest to implement this logic directly by passing an number of required arguments in Dart, see something like https://codereview.chromium.org/15885002/ (better approach would be to pass operation info instead) What do you all think about it?
lgtm Pete, Stephen, just a bit of context: we discussed this CL with Andrei and decided to unblock him to submit it the way it is right now, and I'll try to bring the changes I proposed. That's purely tactical and if you have any additional concerns, please, let us know.
Message was sent while issue was closed.
Committed patchset #15 manually as r23123 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
