Issue 9930008: Fix issue http://code.google.com/p/dart/issues/detail?id=1877 without degrading performance. Impro…

Issue 9930008: Fix issue http://code.google.com/p/dart/issues/detail?id=1877 without degrading performance. Impro… (Closed)

Created:
8 years, 8 months ago by Jacob

Modified:
8 years, 8 months ago

Reviewers:
nweiz

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Fix issue http://code.google.com/p/dart/issues/detail?id=1877 without degrading performance. Improved performance by removing harmful caching logic. BUG=1877 TEST= Committed: https://code.google.com/p/dart/source/detail?r=6015

Patch Set 2 : Code review fixes #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+695 lines, -784 lines)			Patch
M	client/tests/client/html/ElementTests.dart	View	1	1 chunk	+41 lines, -31 lines	0 comments	Download
M	lib/dom/templates/html/impl/impl_Element.darttemplate	View	1	6 chunks	+206 lines, -28 lines	0 comments	Download
M	lib/dom/templates/html/interface/interface_Element.darttemplate	View		4 chunks	+11 lines, -222 lines	0 comments	Download
M	lib/html/dartium/html_dartium.dart	View		10 chunks	+217 lines, -250 lines	0 comments	Download
M	lib/html/frog/html_frog.dart	View	1	10 chunks	+217 lines, -250 lines	0 comments	Download
M	lib/html/src/DataAttributeMap.dart	View		2 chunks	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

nweiz

https://chromiumcodereview.appspot.com/9930008/diff/1/client/tests/client/html/ElementTests.dart File client/tests/client/html/ElementTests.dart (right): https://chromiumcodereview.appspot.com/9930008/diff/1/client/tests/client/html/ElementTests.dart#newcode450 client/tests/client/html/ElementTests.dart:450: test('coersion', () { coercion https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/impl/impl_Element.darttemplate File lib/dom/templates/html/impl/impl_Element.darttemplate (right): https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/impl/impl_Element.darttemplate#newcode420 ...

8 years, 8 months ago (2012-03-30 00:06:23 UTC) #2

Jacob

8 years, 8 months ago (2012-03-30 00:40:53 UTC) #3

https://chromiumcodereview.appspot.com/9930008/diff/1/client/tests/client/htm...
File client/tests/client/html/ElementTests.dart (right):

https://chromiumcodereview.appspot.com/9930008/diff/1/client/tests/client/htm...
client/tests/client/html/ElementTests.dart:450: test('coersion', () {
On 2012/03/30 00:06:23, nweiz wrote:
> coercion

Done.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
File lib/dom/templates/html/impl/impl_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:420: if
(!containsKey(key)) {
On 2012/03/30 00:06:23, nweiz wrote:
> Can't this just call $dom_attributes.putIfAbsent?

Yep. Done. FYI this code isn't new I just moved it because it was in the wrong
file before.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:429: // Needs to operate
on a snapshot since we are mutatiting the collection.
On 2012/03/30 00:06:23, nweiz wrote:
> mutating

Done.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:445:
$dom_attributes.forEach((String key, String value) {
On 2012/03/30 00:06:23, nweiz wrote:
> This would be a little cleaner if you used this class's forEach, or
> $dom_attributes.getKeys().filter(...)
Reasonable idea but I'd rather leave it unchanged. This code didn't really
change with this CL it just moved files. If you want to tweak it to use a more
functional style you can submit a separate CL for that.  The alternate style
would probably be a bit slower but I'm alright with it as it would also be
likely have smaller code size.
I suspect there is a significantly simpler solution for all of this on modern
browsers that support the .data property directly.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:455:
$dom_attributes.forEach((String key, String value) {
On 2012/03/30 00:06:23, nweiz wrote:
> As above, a little cleaner to use this class's forEach.

feel free to send an alternate CL.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:481: return
_formatSet(_read());
On 2012/03/30 00:06:23, nweiz wrote:
> =>, here and below.

Done.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:533: bool result =
s.remove(value);
On 2012/03/30 00:06:23, nweiz wrote:
> Couldn't you use _modify here if you changed it to pass through the return
value
> of the function?
i don't follow. if you want to send a cleanup cl feel free. once again i didn't
modify this code or write it in the first place. it was just moved to the
correct file.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:599: String
$dom_className() => _element.$dom_className;
On 2012/03/30 00:06:23, nweiz wrote:
> Why does this need to be implemented here?

Good catch... my regexp adding $dom to the names of things went a bit too far
and nobody noticed when they reviewed my last CL.

https://chromiumcodereview.appspot.com/9930008/diff/1/lib/dom/templates/html/...
lib/dom/templates/html/impl/impl_Element.darttemplate:715: return new
_DataAttributeMap(attributes);
On 2012/03/30 00:06:23, nweiz wrote:
> =>

Done.

lgtm

Expand Messages | Collapse Messages