Splitting SVG types out of dart:html.

Splitting SVG types out of dart:html.

Beyond this CL I need to remove the SVG prefix for most of the classes, but this is an additional chunk of work.

Committed: https://code.google.com/p/dart/source/detail?r=14808

[blois, 2012-11-09 01:15:56]

This CL moves all of the SVG types out of dart:html to dart:svg.

With the associated Dartium change everything should be working in Dart2JS and
Dartium. I've also validated that the docs have been updated properly.

[Anton Muhin, 2012-11-09 13:13:23]

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:493: // dart:html needs
access to convertDartClosureToJS.
what about dart:svg?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:5328: +    return new
FixedSizeListIterator<DOMMimeType>(this);
why all those changes?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/dart...
File sdk/lib/html/scripts/dartdomgenerator.py (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/dart...
sdk/lib/html/scripts/dartdomgenerator.py:78: emitters, None, dart_output_dir,
dart_libraries)
None is hard to read, can we make it optional?  or pass named constant?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/dart...
sdk/lib/html/scripts/dartdomgenerator.py:189: _logger.info('Copy html_dart2js to
dart2js/')
please, update here and in then clause

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
File sdk/lib/html/scripts/systemhtml.py (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
sdk/lib/html/scripts/systemhtml.py:295:
LIBRARYNAME=self._renamer.GetLibraryName(self._interface),
LIBRARYNAME=self._library_name?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
sdk/lib/html/scripts/systemhtml.py:924: def __init__(self, template_loader,
library_type, output_dir):
it might be more natural to use a dictionary here: self._libraries['svg'] = etc.
 You can wrap it into a syntactic sugar if you want.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
File sdk/lib/html/scripts/systemnative.py (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
sdk/lib/html/scripts/systemnative.py:235: DART_IMPLEMENTATION_LIBRARY='dart:%s'
% self._renamer.GetLibraryName(self._interface))
does it have access to self._library_name?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
File sdk/lib/html/templates/html/impl/impl_DocumentFragment.darttemplate (left):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
sdk/lib/html/templates/html/impl/impl_DocumentFragment.darttemplate:7: class
_FilteredElementList implements List {
do all those changes belong here?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
File sdk/lib/html/templates/html/impl/impl_SVGElement.darttemplate (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
sdk/lib/html/templates/html/impl/impl_SVGElement.darttemplate:7: class
_AttributeClassSet extends CssClassSet {
why it's now parameterized by library name?  cannot it be just part of svg?

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
File sdk/lib/html/templates/html/impl/impl_SVGSVGElement.darttemplate (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
sdk/lib/html/templates/html/impl/impl_SVGSVGElement.darttemplate:5: part of
$LIBRARYNAME;
ditto

https://codereview.chromium.org/11398002/diff/39018/tests/html/node_test.dart
File tests/html/node_test.dart (right):

https://codereview.chromium.org/11398002/diff/39018/tests/html/node_test.dart...
tests/html/node_test.dart:9: import 'dart:svg';
do we need this import here?

https://codereview.chromium.org/11398002/diff/39018/tests/html/svgelement2_te...
File tests/html/svgelement2_test.dart (right):

https://codereview.chromium.org/11398002/diff/39018/tests/html/svgelement2_te...
tests/html/svgelement2_test.dart:9: import 'dart:svg';
do we need an import here?  same to dart:html

https://codereview.chromium.org/11398002/diff/39018/utils/apidoc/html_diff.dart
File utils/apidoc/html_diff.dart (right):

https://codereview.chromium.org/11398002/diff/39018/utils/apidoc/html_diff.da...
utils/apidoc/html_diff.dart:112: void _processLibrary(LibraryMirror library) {
why a helper method?  can it be inline or a closure?

[blois, 2012-11-09 18:21:55]

Thanks for the feedback Anton!

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:493: // dart:html needs
access to convertDartClosureToJS.
On 2012/11/09 13:13:23, Anton Muhin wrote:
> what about dart:svg?

I'll have to ask around- I initially had it, but then removed it and everything
seemed to work.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:5328: +    return new
FixedSizeListIterator<DOMMimeType>(this);
On 2012/11/09 13:13:23, Anton Muhin wrote:
> why all those changes?

These are used by the SVG types as well. So I could either duplicate the code
into the two libs or make it public, I decided to make it public.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/dart...
File sdk/lib/html/scripts/dartdomgenerator.py (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/dart...
sdk/lib/html/scripts/dartdomgenerator.py:78: emitters, None, dart_output_dir,
dart_libraries)
On 2012/11/09 13:13:23, Anton Muhin wrote:
> None is hard to read, can we make it optional?  or pass named constant?

This was cruft, removed completely.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/dart...
sdk/lib/html/scripts/dartdomgenerator.py:189: _logger.info('Copy html_dart2js to
dart2js/')
On 2012/11/09 13:13:23, Anton Muhin wrote:
> please, update here and in then clause

Done.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
File sdk/lib/html/scripts/systemhtml.py (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
sdk/lib/html/scripts/systemhtml.py:295:
LIBRARYNAME=self._renamer.GetLibraryName(self._interface),
On 2012/11/09 13:13:23, Anton Muhin wrote:
> LIBRARYNAME=self._library_name?

Done.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
sdk/lib/html/scripts/systemhtml.py:924: def __init__(self, template_loader,
library_type, output_dir):
On 2012/11/09 13:13:23, Anton Muhin wrote:
> it might be more natural to use a dictionary here: self._libraries['svg'] =
etc.
>  You can wrap it into a syntactic sugar if you want.

Good point, I had more logic here where the members helped, but no more.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
File sdk/lib/html/scripts/systemnative.py (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/scripts/syst...
sdk/lib/html/scripts/systemnative.py:235: DART_IMPLEMENTATION_LIBRARY='dart:%s'
% self._renamer.GetLibraryName(self._interface))
On 2012/11/09 13:13:23, Anton Muhin wrote:
> does it have access to self._library_name?
It does not.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
File sdk/lib/html/templates/html/impl/impl_DocumentFragment.darttemplate (left):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
sdk/lib/html/templates/html/impl/impl_DocumentFragment.darttemplate:7: class
_FilteredElementList implements List {
On 2012/11/09 13:13:23, Anton Muhin wrote:
> do all those changes belong here?

I made this public since it's being used by SVG as well. It's quite odd to have
FilteredElementList declared here since it's used by a bunch of types, so I
moved it out into the src directory as well.

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
File sdk/lib/html/templates/html/impl/impl_SVGElement.darttemplate (right):

https://codereview.chromium.org/11398002/diff/39018/sdk/lib/html/templates/ht...
sdk/lib/html/templates/html/impl/impl_SVGElement.darttemplate:7: class
_AttributeClassSet extends CssClassSet {
On 2012/11/09 13:13:23, Anton Muhin wrote:
> why it's now parameterized by library name?  cannot it be just part of svg?


Changed. I had it that way since it's easier to modify if they happen to move,
probably the same reasoning behind using $CLASSNAME here as well.

https://codereview.chromium.org/11398002/diff/39018/tests/html/node_test.dart
File tests/html/node_test.dart (right):

https://codereview.chromium.org/11398002/diff/39018/tests/html/node_test.dart...
tests/html/node_test.dart:9: import 'dart:svg';
On 2012/11/09 13:13:23, Anton Muhin wrote:
> do we need this import here?
It does, it references SVGElement.

https://codereview.chromium.org/11398002/diff/39018/tests/html/svgelement2_te...
File tests/html/svgelement2_test.dart (right):

https://codereview.chromium.org/11398002/diff/39018/tests/html/svgelement2_te...
tests/html/svgelement2_test.dart:9: import 'dart:svg';
On 2012/11/09 13:13:23, Anton Muhin wrote:
> do we need an import here?  same to dart:html

Neither are needed, good catch.

https://codereview.chromium.org/11398002/diff/39018/utils/apidoc/html_diff.dart
File utils/apidoc/html_diff.dart (right):

https://codereview.chromium.org/11398002/diff/39018/utils/apidoc/html_diff.da...
utils/apidoc/html_diff.dart:112: void _processLibrary(LibraryMirror library) {
On 2012/11/09 13:13:23, Anton Muhin wrote:
> why a helper method?  can it be inline or a closure?

Done.

[Anton Muhin, 2012-11-12 18:37:31]

lgtm

nice work, Pete

[sra1, 2012-11-12 20:21:24]

lgtm

[ahe, 2012-11-16 09:12:50]

Next time you make modifications to sdk/lib/_internal/compiler please make sure
to wait for LGTM from a reviewer from the dart2js team in AAR.  It is hard for
us to keep track of what is going on when you don't.

The changes to sdk/lib/_internal/compiler, LGTM.