Fix XML tests on Safari and IE.

Fix XML tests on Safari and IE.

This involves fixing $typeNameOf when dealing with XML documents, as well as
ensuring that nodes are removed from their owner documents before being added to
new documents.

Fixes issue 1884.

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

[Jacob, 2012-02-29 00:01:37]

lgtm for the html library changes

[sra1, 2012-02-29 01:28:55]

LGTM - these are just suggestions.

We should cc Kasper/Florian/Nicolas on any frog changes.

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart
File frog/corejs.dart (right):

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart#newcod...
frog/corejs.dart:404: if (name && typeof(name) == 'string' && name != 'Object')
return name;
Nit: I'd put typeof first (assuming name && is protecting against empty string)

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart#newcod...
frog/corejs.dart:412: if (typeof(navigator) != 'object') {
Nit: typeof is an operator and does not need parens.
There are some others here that could lose the parens too.

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart#newcod...
frog/corejs.dart:418: return function() { return this.constructor.name; };
This function occurs twice.
Which makes me think all the 'methods' could be defined as local functions, then
they can be returned compactly.

function chrome_$typeNameOf() { return this.constructor.name; }
function firefox_$typeNameOf() {
...}


if (...) return chrome_$typeNameOf;
if (/Firefox/...) return firefox_$typeNameOf;
if (/MSIE/...) return msie_$typeNameOf;

[nweiz, 2012-02-29 01:54:27]

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart
File frog/corejs.dart (right):

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart#newcod...
frog/corejs.dart:404: if (name && typeof(name) == 'string' && name != 'Object')
return name;
On 2012/02/29 01:28:55, sra1 wrote:
> Nit: I'd put typeof first (assuming name && is protecting against empty
string)

Done.

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart#newcod...
frog/corejs.dart:412: if (typeof(navigator) != 'object') {
On 2012/02/29 01:28:55, sra1 wrote:
> Nit: typeof is an operator and does not need parens.
> There are some others here that could lose the parens too.

I believe the style is to use parens with typeof.

https://chromiumcodereview.appspot.com/9513015/diff/1/frog/corejs.dart#newcod...
frog/corejs.dart:418: return function() { return this.constructor.name; };
On 2012/02/29 01:28:55, sra1 wrote:
> This function occurs twice.
> Which makes me think all the 'methods' could be defined as local functions,
then
> they can be returned compactly.
> 
> function chrome_$typeNameOf() { return this.constructor.name; }
> function firefox_$typeNameOf() {
> ...}
> 
> 
> if (...) return chrome_$typeNameOf;
> if (/Firefox/...) return firefox_$typeNameOf;
> if (/MSIE/...) return msie_$typeNameOf;

Done.

[kasperl, 2012-02-29 05:59:58]

LGTM! Thanks for sending this my way.

https://chromiumcodereview.appspot.com/9513015/diff/5001/client/html/src/XMLE...
File client/html/src/XMLElementWrappingImplementation.dart (right):

https://chromiumcodereview.appspot.com/9513015/diff/5001/client/html/src/XMLE...
client/html/src/XMLElementWrappingImplementation.dart:55: // Safari requires
that the clone be removed from its owner document before
Is this another place where knowing which platform you run on could help
(performance)?

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart
File frog/corejs.dart (right):

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart#new...
frog/corejs.dart:410: function chrome_$typeNameOf() { return
this.constructor.name; }
I would put a newline after { for consistency.

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart#new...
frog/corejs.dart:412: function firefox_$typeNameOf() {
Why is there an underscore after firefox, chrome, and ie? The $ delimits things
nicely, no?

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart#new...
frog/corejs.dart:430: // If we're not in the browser, we're almost certainly
running on v8.
Long term, it would be great to factor this platform check out so we can use it
from multiple different places.

[dgrove, 2012-02-29 06:04:13]

(switching cc to correct Nicolas and to Florian's google account)

[nweiz, 2012-02-29 19:41:45]

https://chromiumcodereview.appspot.com/9513015/diff/5001/client/html/src/XMLE...
File client/html/src/XMLElementWrappingImplementation.dart (right):

https://chromiumcodereview.appspot.com/9513015/diff/5001/client/html/src/XMLE...
client/html/src/XMLElementWrappingImplementation.dart:55: // Safari requires
that the clone be removed from its owner document before
On 2012/02/29 05:59:58, kasperl wrote:
> Is this another place where knowing which platform you run on could help
> (performance)?

I believe #remove is very cheap, so I don't think a browser check here would be
too helpful.

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart
File frog/corejs.dart (right):

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart#new...
frog/corejs.dart:410: function chrome_$typeNameOf() { return
this.constructor.name; }
On 2012/02/29 05:59:58, kasperl wrote:
> I would put a newline after { for consistency.

Done.

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart#new...
frog/corejs.dart:412: function firefox_$typeNameOf() {
On 2012/02/29 05:59:58, kasperl wrote:
> Why is there an underscore after firefox, chrome, and ie? The $ delimits
things
> nicely, no?

I was thinking of "$typeNameOf" as an atomic identifier, rather than an
identifier with an initial delimiter, but I can change it to have no underscore.

https://chromiumcodereview.appspot.com/9513015/diff/5001/frog/corejs.dart#new...
frog/corejs.dart:430: // If we're not in the browser, we're almost certainly
running on v8.
On 2012/02/29 05:59:58, kasperl wrote:
> Long term, it would be great to factor this platform check out so we can use
it
> from multiple different places.

The best practice in most instances where you want functionality to vary between
browsers is to do feature detection, and choose the behavior based on what APIs
the browser supports rather than which browser you're using. That's not possible
here because we're not accessing a feature as such, but that's probably the
direction we should go for most compatibility issues.