Issue 10827428: Dispatch with conversion hooks

Issue 10827428: Dispatch with conversion hooks (Closed)

Created:
8 years, 4 months ago by sra1

Modified:
8 years, 4 months ago

Reviewers:
vsm

CC:
reviews_dartlang.org, Anton Muhin

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Conversions between Dart code and DOM code. Arguments passed to and values returned by JavaScript code sometimes need to be converted. dart:html for dart2js specifies these conversions in a table indexed by type and, optionally, operation. If an operation has any overloads that require conversion, a dispatch is generated to identify the values needing conversion, and to ensure the native method is called with the same number of arguments as the original user call site. Committed: https://code.google.com/p/dart/source/detail?r=11193

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1156 lines, -56 lines)			Patch
M	lib/dom/scripts/generator.py	View	1 2 3	3 chunks	+67 lines, -0 lines	0 comments	Download
M	lib/dom/scripts/systemhtml.py	View	1 2 3	7 chunks	+213 lines, -16 lines	0 comments	Download
M	lib/dom/templates/html/dart2js/html_dart2js.darttemplate	View	1 2 3	2 chunks	+8 lines, -0 lines	0 comments	Download
M	lib/html/dart2js/html_dart2js.dart	View		19 chunks	+761 lines, -40 lines	0 comments	Download
A	lib/html/src/dart2js_Conversions.dart	View	1 2 3	1 chunk	+106 lines, -0 lines	0 comments	Download
M	tests/html/html.status	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 1 (0 generated)

Expand Messages | Collapse Messages

vsm

8 years, 4 months ago (2012-08-22 18:49:36 UTC) #1

lgtm!

I did notice some redundant checks (marked below) though.

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/dom/scripts/sys...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:906: # BUGBUG: _FindShadowedAttribute does not
take into account the html
s/BUGBUG/TODO(sra)/

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:976: #return_type = conversion.output_type
Delete commented code.

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:1048: output_conversion =
self._OutputConversion(info.type_name, info.declared_name)
line length

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:1164: # foo(x, y).
line len in this comment block

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/dart2js/ht...
File lib/html/dart2js/html_dart2js.dart (right):

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/dart2js/ht...
lib/html/dart2js/html_dart2js.dart:7934: +    if (_default != key_OR_range) {
Isn't this trivially true given the first check?

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/dart2js/ht...
lib/html/dart2js/html_dart2js.dart:7996: +    }
Isn't this redundant / unreachable with _openCursor_1 above?

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/dart2js/ht...
lib/html/dart2js/html_dart2js.dart:8046: +        _default == direction) {
Another redundant case.

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/src/dart2j...
File lib/html/src/dart2js_Conversions.dart (right):

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/src/dart2j...
lib/html/src/dart2js_Conversions.dart:58: class FakeImageData implements
ImageData {
Nit: I'd use a different name than 'Fake'.  E.g., 'Firefox' or 'Typed'.  Up to
you.

https://chromiumcodereview.appspot.com/10827428/diff/4001/lib/html/src/dart2j...
lib/html/src/dart2js_Conversions.dart:106: Object
_ensureStringArray(List<String> input) {
Rename to convertDartToNative for consistency?

Expand Messages | Collapse Messages