Add a couple useful SVG constructors.

Add a couple useful SVG constructors.

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

[Jacob, 2011-12-13 01:49:22]

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart
File client/html/src/SVGElement.dart (right):

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart...
client/html/src/SVGElement.dart:7: SVGElement.tag(String tag);
Add a comment clarifying when you should use this instead of new Element.tag

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGSVGElementWr...
File client/html/src/SVGSVGElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGSVGElementWr...
client/html/src/SVGSVGElementWrappingImplementation.dart:8: factory
SVGSVGElementWrappingImplementation() {
Should we instead hide SVGSVGElement completely? I'm guessing it is the same
sort of useless element as HTMLHtmlElement.

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGSVGElementWr...
client/html/src/SVGSVGElementWrappingImplementation.dart:11:
el.attributes['version'] = 1.1;
At least comment why you want to hard code this version and ns.

[nweiz, 2011-12-13 19:08:06]

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart
File client/html/src/SVGElement.dart (right):

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart...
client/html/src/SVGElement.dart:7: SVGElement.tag(String tag);
On 2011/12/13 01:49:22, Jacob wrote:
> Add a comment clarifying when you should use this instead of new Element.tag

It seems clear from context that this one creates SVG elements, as opposed to
HTML...

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGSVGElementWr...
File client/html/src/SVGSVGElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGSVGElementWr...
client/html/src/SVGSVGElementWrappingImplementation.dart:8: factory
SVGSVGElementWrappingImplementation() {
On 2011/12/13 01:49:22, Jacob wrote:
> Should we instead hide SVGSVGElement completely? I'm guessing it is the same
> sort of useless element as HTMLHtmlElement.

It's more useful in that you want to create an SVGSVGElement programatically
reasonably often; it's not like HTMLHtmlElement which is always in one-to-one
correspondence with the HTMLDocument. Embedded SVG in an HTML document doesn't
actually have an SVGDocument at all, as far as I can tell.

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGSVGElementWr...
client/html/src/SVGSVGElementWrappingImplementation.dart:11:
el.attributes['version'] = 1.1;
On 2011/12/13 01:49:22, Jacob wrote:
> At least comment why you want to hard code this version and ns.

Done.

[Jacob, 2011-12-13 19:21:54]

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart
File client/html/src/SVGElement.dart (right):

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart...
client/html/src/SVGElement.dart:7: SVGElement.tag(String tag);
On 2011/12/13 19:08:06, nweiz wrote:
> On 2011/12/13 01:49:22, Jacob wrote:
> > Add a comment clarifying when you should use this instead of new Element.tag
> 
> It seems clear from context that this one creates SVG elements, as opposed to
> HTML...

What worries me is it isn't really clear from 
new Element.tag
that it should only work for HTML elements and not SVG elements. Perhaps I'm
paranoid.

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
File client/html/src/SVGSVGElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
client/html/src/SVGSVGElementWrappingImplementation.dart:11: // and set
innerSVG; set version to 1.1 because that's required by the SVG
innerSVG? You mean so that we can simulate innerHTML?

Place the version comment on the line before setting the version rather than
using ";"

Avoid repeating text in code and comments.
Rather than saying "set version to 1.1 because..."
you could write something like
"Version required by the latest SVG spec"

[nweiz, 2011-12-13 19:27:57]

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart
File client/html/src/SVGElement.dart (right):

http://codereview.chromium.org/8919029/diff/1/client/html/src/SVGElement.dart...
client/html/src/SVGElement.dart:7: SVGElement.tag(String tag);
On 2011/12/13 19:21:54, Jacob wrote:
> On 2011/12/13 19:08:06, nweiz wrote:
> > On 2011/12/13 01:49:22, Jacob wrote:
> > > Add a comment clarifying when you should use this instead of new
Element.tag
> > 
> > It seems clear from context that this one creates SVG elements, as opposed
to
> > HTML...
> 
> What worries me is it isn't really clear from 
> new Element.tag
> that it should only work for HTML elements and not SVG elements. Perhaps I'm
> paranoid.

I think it's reasonable to expect users to know that the normal APIs all have to
do with HTML. People using SVG should also have some understanding that it's a
Different Thing.

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
File client/html/src/SVGSVGElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
client/html/src/SVGSVGElementWrappingImplementation.dart:11: // and set
innerSVG; set version to 1.1 because that's required by the SVG
On 2011/12/13 19:21:54, Jacob wrote:
> innerSVG? You mean so that we can simulate innerHTML?
> 
> Place the version comment on the line before setting the version rather than
> using ";"
> 
> Avoid repeating text in code and comments.
> Rather than saying "set version to 1.1 because..."
> you could write something like
> "Version required by the latest SVG spec"

Done. It turns out (I think) that the implementation of innerSVG/innerHTML won't
care about the namespace anyway.

[Jacob, 2011-12-13 19:29:26]

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
File client/html/src/SVGSVGElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
client/html/src/SVGSVGElementWrappingImplementation.dart:11: // and set
innerSVG; set version to 1.1 because that's required by the SVG
On 2011/12/13 19:27:57, nweiz wrote:
> On 2011/12/13 19:21:54, Jacob wrote:
> > innerSVG? You mean so that we can simulate innerHTML?
> > 
> > Place the version comment on the line before setting the version rather than
> > using ";"
> > 
> > Avoid repeating text in code and comments.
> > Rather than saying "set version to 1.1 because..."
> > you could write something like
> > "Version required by the latest SVG spec"
> 
> Done. It turns out (I think) that the implementation of innerSVG/innerHTML
won't
> care about the namespace anyway.

If we can leave out the namespace and still have the code work on modern
browsers, we should. I'd rather avoid magic hard coded dependencies to specific
spec versions.

[nweiz, 2011-12-13 19:41:23]

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
File client/html/src/SVGSVGElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8919029/diff/6001/client/html/src/SVGSVGElemen...
client/html/src/SVGSVGElementWrappingImplementation.dart:11: // and set
innerSVG; set version to 1.1 because that's required by the SVG
On 2011/12/13 19:29:27, Jacob wrote:
> On 2011/12/13 19:27:57, nweiz wrote:
> > On 2011/12/13 19:21:54, Jacob wrote:
> > > innerSVG? You mean so that we can simulate innerHTML?
> > > 
> > > Place the version comment on the line before setting the version rather
than
> > > using ";"
> > > 
> > > Avoid repeating text in code and comments.
> > > Rather than saying "set version to 1.1 because..."
> > > you could write something like
> > > "Version required by the latest SVG spec"
> > 
> > Done. It turns out (I think) that the implementation of innerSVG/innerHTML
> won't
> > care about the namespace anyway.
> 
> If we can leave out the namespace and still have the code work on modern
> browsers, we should. I'd rather avoid magic hard coded dependencies to
specific
> spec versions.

I don't fully understand all the ramifications of XML namespacing and I suspect
that the XMLNS makes certain edge cases work. That said, I don't know for sure
that that's the case, and erring on the side of less code seems like a good
idea, so I'll remove it for now.

[Jacob, 2011-12-13 19:50:32]

lgtm