Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
938413002:
Work around Safari for-in bug. (Closed)
DescriptionWork around Safari for-in bug.
R=floitsch@google.com
Committed: https://code.google.com/p/dart/source/detail?r=43959
Patch Set 1 #
Total comments: 27
Patch Set 2 : Address Florian's comments. #
Total comments: 3
Patch Set 3 : Object.keys throws if given undefined :-( #Patch Set 4 : JS foreign function doesn't support named arguments. #Patch Set 5 : More issues discovered during testing. #
Total comments: 2
Patch Set 6 : Merged with r43958. #
Messages
Total messages: 18 (7 generated)
ahe@google.com changed reviewers: + floitsch@google.com
I could reproduce the issue easily with tests/html/element_test.dart, with these
changes I cannot.
Diff of JavaScript compilation of tests/html/element_test.dart:
--- broken.js 2015-02-20 14:10:07.000000000 +0100
+++ out.js 2015-02-20 14:17:55.000000000 +0100
@@ -147,7 +147,6 @@
var inheritFrom = function() {
function tmp() {
}
- var hasOwnProperty = Object.prototype.hasOwnProperty;
return function(constructor, superConstructor) {
if (superConstructor == null) {
var prototype = constructor.prototype;
@@ -158,10 +157,12 @@
tmp.prototype = superConstructor.prototype;
var object = new tmp();
var properties = constructor.prototype;
- for (var member in properties) {
- if (hasOwnProperty.call(properties, member)) {
- object[member] = properties[member];
- }
+ var members = Object.keys(properties);
+ var members_length = members.length;
+ var member;
+ for (var i = 0; i < members_length; ++i) {
+ member = members[i];
+ object[member] = properties[member];
}
object["$is" + constructor.name] = constructor;
object.constructor = constructor;
@@ -201,8 +202,12 @@
var mixin = allClasses[mixinClass];
var mixinPrototype = mixin.prototype;
var clsPrototype = allClasses[cls].prototype;
- for (var d in mixinPrototype) {
- if (hasOwnProperty.call(mixinPrototype, d) &&
!hasOwnProperty.call(clsPrototype, d))
+ var properties = Object.keys(mixinPrototype);
+ var properties_length = properties.length;
+ var d;
+ for (var i = 0; i < properties_length; ++i) {
+ d = properties[i];
+ if (!hasOwnProperty.call(clsPrototype, d))
clsPrototype[d] = mixinPrototype[d];
}
}
@@ -241,15 +246,19 @@
}
}
}
- for (var cls in processedClasses.pending)
- finishClass(cls);
+ var properties = Object.keys(processedClasses.pending);
+ var properties_length = properties.length;
+ for (var i = 0; i < properties_length; ++i)
+ finishClass(properties[i]);
}
function processClassData(cls, descriptor, processedClasses) {
var newDesc = {};
var previousProperty;
- for (var property in descriptor) {
- if (!hasOwnProperty.call(descriptor, property))
- continue;
+ var properties = Object.keys(descriptor);
+ var properties_length = properties.length;
+ var property;
+ for (var i = 0; i < properties_length; ++i) {
+ property = properties[i];
var firstChar = property.substring(0, 1);
if (property === "static") {
processStatics(init.statics[cls] = descriptor[property],
processedClasses);
@@ -297,9 +306,11 @@
classes.push(cls);
}
function processStatics(descriptor, processedClasses) {
- for (var property in descriptor) {
- if (!hasOwnProperty.call(descriptor, property))
- continue;
+ var properties = Object.keys(descriptor);
+ var properties_length = properties.length;
+ var property;
+ for (var i = 0; i < properties_length; ++i) {
+ property = properties[i];
if (property === "^")
continue;
var element = descriptor[property];
@@ -6745,14 +6756,7 @@
["dart._js_names", "dart:_js_names", , H, {
"^": "",
extractKeys: function(victim) {
- var t1 = H.setRuntimeTypeInfo(function(victim, hasOwnProperty) {
- var result = [];
- for (var key in victim) {
- if (hasOwnProperty.call(victim, key))
- result.push(key);
- }
- return result;
- }(victim, Object.prototype.hasOwnProperty), [null]);
+ var t1 = H.setRuntimeTypeInfo(Object.keys(victim), [null]);
t1.fixed$length = Array;
return t1;
}
@@ -22082,20 +22086,25 @@
Isolate.$finishIsolateConstructor = function(oldIsolate) {
var isolateProperties = oldIsolate.$isolateProperties;
function Isolate() {
- var hasOwnProperty = Object.prototype.hasOwnProperty;
- for (var staticName in isolateProperties)
- if (hasOwnProperty.call(isolateProperties, staticName))
- this[staticName] = isolateProperties[staticName];
+ var staticNames = Object.keys(isolateProperties);
+ var staticNames_length = staticNames.length;
+ var staticName;
+ for (var i = 0; i < staticNames_length; ++i) {
+ staticName = staticNames[i];
+ this[staticName] = isolateProperties[staticName];
+ }
var lazies = init.lazies;
- for (var lazyInit in lazies) {
- this[lazies[lazyInit]] = null;
+ var lazyInitializers = Object.keys(lazies);
+ var lazyInitializers_length = lazyInitializers.length;
+ for (var i = 0; i < lazyInitializers_length; ++i) {
+ this[lazies[lazyInitializers[i]]] = null;
}
function ForceEfficientMap() {
}
ForceEfficientMap.prototype = this;
new ForceEfficientMap();
- for (var lazyInit in lazies) {
- var lazyInitName = lazies[lazyInit];
+ for (var i = 0; i < lazyInitializers_length; ++i) {
+ var lazyInitName = lazies[lazyInitializers[i]];
this[lazyInitName] = isolateProperties[lazyInitName];
}
}
LGTM. just nits. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... File dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:438: var members_length = members.length; No need to do this. In fact Slava showed that this actually slows down JS VMs. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:440: for (var i = 0; i < members_length; ++i) { I prefer i++. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:441: member = members[i]; var member = members[i] (no need to hoist the "var declaration". The js-parser will move the "var"s into one line on the top of the function anyway. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:504: var properties_length = properties.length; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:505: var d; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1024: var staticNames_length = staticNames.length; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1025: var staticName; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1038: var lazyInitializers_length = lazyInitializers.length; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... File dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart (right): https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:61: var properties_length = properties.length; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:62: var property; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:142: var properties_length = properties.length; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:143: var property; ditto. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:417: var properties_length = properties.length; ditto.
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
Patchset #2 (id:20001) has been deleted
Still LGTM. thanks.
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
Patchset #2 (id:40001) has been deleted
Still LGTM.
Not sure if you actually reviewed the changes to js_mirrors.dart? https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... File dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:438: var members_length = members.length; On 2015/02/20 14:03:38, floitsch wrote: > No need to do this. In fact Slava showed that this actually slows down JS VMs. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:440: for (var i = 0; i < members_length; ++i) { On 2015/02/20 14:03:38, floitsch wrote: > I prefer i++. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:441: member = members[i]; On 2015/02/20 14:03:38, floitsch wrote: > var member = members[i] (no need to hoist the "var declaration". > > The js-parser will move the "var"s into one line on the top of the function > anyway. I actually put it there to make the hoisting explicit :-) https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:441: member = members[i]; On 2015/02/20 14:03:38, floitsch wrote: > var member = members[i] (no need to hoist the "var declaration". > > The js-parser will move the "var"s into one line on the top of the function > anyway. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:504: var properties_length = properties.length; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:505: var d; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1024: var staticNames_length = staticNames.length; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1025: var staticName; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1038: var lazyInitializers_length = lazyInitializers.length; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... File dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart (right): https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:61: var properties_length = properties.length; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:62: var property; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:142: var properties_length = properties.length; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:143: var property; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/1/dart/pkg/compiler/lib/src/js... dart/pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:417: var properties_length = properties.length; On 2015/02/20 14:03:38, floitsch wrote: > ditto. Done. https://codereview.chromium.org/938413002/diff/60001/dart/sdk/lib/_internal/c... File dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://codereview.chromium.org/938413002/diff/60001/dart/sdk/lib/_internal/c... dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:2239: var properties = Object.keys(reflectee); I'm pretty sure I was too lazy to use hasOwnProperty here.
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
https://codereview.chromium.org/938413002/diff/60001/dart/sdk/lib/_internal/c... File dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://codereview.chromium.org/938413002/diff/60001/dart/sdk/lib/_internal/c... dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:2239: var properties = Object.keys(reflectee); On 2015/02/20 14:32:16, ahe wrote: > I'm pretty sure I was too lazy to use hasOwnProperty here. Change LGTM. The call-properties are stored on the reflectee directly.
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
https://codereview.chromium.org/938413002/diff/60001/dart/sdk/lib/_internal/c... File dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://codereview.chromium.org/938413002/diff/60001/dart/sdk/lib/_internal/c... dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:2239: var properties = Object.keys(reflectee); On 2015/02/20 16:08:03, floitsch wrote: > On 2015/02/20 14:32:16, ahe wrote: > > I'm pretty sure I was too lazy to use hasOwnProperty here. > > Change LGTM. The call-properties are stored on the reflectee directly. Turns out that the property is stored on the prototype, and D8 can't find it with Object.getOwnPropertyNames :-( However, this works: Object.keys(reflectee.constructor.prototype) But now I'm scared about Chrome/V8 having issues with Object.getOwnPropertyNames. Good thing Object.keys seem to work across the board. Not looking forward to seeing how this works on IE D:
https://chromiumcodereview.appspot.com/938413002/diff/120001/dart/sdk/lib/_in... File dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://chromiumcodereview.appspot.com/938413002/diff/120001/dart/sdk/lib/_in... dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:2239: var properties = Object.keys(reflectee.constructor.prototype); Are you sure this is equivalent? Does the JsClosureMirror get invoked for classes that implement the 'call' method? If yes, then the call-property might be on a super. Safari's for-in can't be completely broken (otherwise every page would have this problem). I would let this one in, and hope that it isn't affected.
https://chromiumcodereview.appspot.com/938413002/diff/120001/dart/sdk/lib/_in... File dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://chromiumcodereview.appspot.com/938413002/diff/120001/dart/sdk/lib/_in... dart/sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:2239: var properties = Object.keys(reflectee.constructor.prototype); On 2015/02/23 14:14:26, floitsch wrote: > Does the JsClosureMirror get invoked for classes that implement the 'call' > method? It doesn't. It only gets invoked for actual closures that implements the Closure interface (which is private to dart2js' platform implementation libraries). That's probably a bug, but the solution is not to use for-in here. This class assumes that reflectee is an instance of Closure. > Safari's for-in can't be completely broken (otherwise every page would have this > problem). I would let this one in, and hope that it isn't affected. I don't know precisely how for-in is broken, but I don't see how this situation is different from the other for-ins. So I'd strongly prefer to not rely on for-in.
Message was sent while issue was closed.
Committed patchset #6 (id:140001) manually as 43959 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
