Fix client/html_tests so they use the new framework.

Fix client/html_tests so they use the new framework.

TBR=jacobr

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

[Jacob, 2011-10-25 23:45:51]

http://codereview.chromium.org/8341027/diff/1/client/html/src/CSSStyleDeclara...
File client/html/src/CSSStyleDeclarationWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/CSSStyleDeclara...
client/html/src/CSSStyleDeclarationWrappingImplementation.dart:25: return new
CSSStyleDeclarationWrappingImplementation.css("");
"" --> ''

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
client/html/src/DocumentFragmentWrappingImplementation.dart:112: :
super._wrap(dom.document.createElement('div').style) {}
wouldn't 
super._wrap(null);

be sufficient?

also, omit {} for empty constructor bodies.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
File client/tests/client/html/CSSStyleDeclarationTests.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:6: getStyle() {
nit: rename to 
createTestStyle

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:20: //
Expect.isNull(style.parentRule);
remove or note why they are commented out

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:34: test('length is
wrapped', () {
i'm confused why these tests contain the text "is wrapped"
isn't that redundant as every test for this library should contain those words.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:59: test('getters and
setters are generated', () {
nit: replace "getters and setters are generated" with 
'css property getters and setters'

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/Docume...
File client/tests/client/html/DocumentFragmentTests.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/Docume...
client/tests/client/html/DocumentFragmentTests.dart:316: // test('const fields
are immutable', () {
If code is commented out, add a comment indicating why it is.

[nweiz, 2011-10-26 00:27:36]

http://codereview.chromium.org/8341027/diff/1/client/html/src/CSSStyleDeclara...
File client/html/src/CSSStyleDeclarationWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/CSSStyleDeclara...
client/html/src/CSSStyleDeclarationWrappingImplementation.dart:25: return new
CSSStyleDeclarationWrappingImplementation.css("");
On 2011/10/25 23:45:51, jacobr wrote:
> "" --> ''

Done.

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
client/html/src/DocumentFragmentWrappingImplementation.dart:112: :
super._wrap(dom.document.createElement('div').style) {}
On 2011/10/25 23:45:51, jacobr wrote:
> wouldn't 
> super._wrap(null);
> 
> be sufficient?
> 
> also, omit {} for empty constructor bodies.

DOMWrapperBase assumes _ptr is non-null.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
File client/tests/client/html/CSSStyleDeclarationTests.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:6: getStyle() {
On 2011/10/25 23:45:51, jacobr wrote:
> nit: rename to 
> createTestStyle

Done.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:20: //
Expect.isNull(style.parentRule);
On 2011/10/25 23:45:51, jacobr wrote:
> remove or note why they are commented out

Done.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:34: test('length is
wrapped', () {
On 2011/10/25 23:45:51, jacobr wrote:
> i'm confused why these tests contain the text "is wrapped"
> isn't that redundant as every test for this library should contain those
words.

The idea is that these tests just test that the property wrapper is hooked up
correctly; the wrapper isn't doing anything particularly interesting. In other
tests, the wrapper is doing more stuff, which is the bulk of what we're testing,
so the test descriptions reflect that.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/CSSSty...
client/tests/client/html/CSSStyleDeclarationTests.dart:59: test('getters and
setters are generated', () {
On 2011/10/25 23:45:51, jacobr wrote:
> nit: replace "getters and setters are generated" with 
> 'css property getters and setters'

Done.

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/Docume...
File client/tests/client/html/DocumentFragmentTests.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/tests/client/html/Docume...
client/tests/client/html/DocumentFragmentTests.dart:316: // test('const fields
are immutable', () {
On 2011/10/25 23:45:51, jacobr wrote:
> If code is commented out, add a comment indicating why it is.

Done. Sorry about that, I committed this a little prematurely.

[Jacob, 2011-10-26 01:03:13]

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
client/html/src/DocumentFragmentWrappingImplementation.dart:112: :
super._wrap(dom.document.createElement('div').style) {}
On 2011/10/26 00:27:37, nweiz wrote:
> On 2011/10/25 23:45:51, jacobr wrote:
> > wouldn't 
> > super._wrap(null);
> > 
> > be sufficient?
> > 
> > also, omit {} for empty constructor bodies.
> 
> DOMWrapperBase assumes _ptr is non-null.

I'd suggest adding an alternate 
_empty()
constructor  to DOMWrapperBase.
which sets _ptr to null.

[Anton Muhin, 2011-10-26 13:24:05]

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
client/html/src/DocumentFragmentWrappingImplementation.dart:112: :
super._wrap(dom.document.createElement('div').style) {}
Why it's needed?  I would really prefer not to have anything like that.

On 2011/10/26 01:03:13, jacobr wrote:
> On 2011/10/26 00:27:37, nweiz wrote:
> > On 2011/10/25 23:45:51, jacobr wrote:
> > > wouldn't 
> > > super._wrap(null);
> > > 
> > > be sufficient?
> > > 
> > > also, omit {} for empty constructor bodies.
> > 
> > DOMWrapperBase assumes _ptr is non-null.
> 
> I'd suggest adding an alternate 
> _empty()
> constructor  to DOMWrapperBase.
> which sets _ptr to null.

[nweiz, 2011-10-26 17:30:53]

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
client/html/src/DocumentFragmentWrappingImplementation.dart:112: :
super._wrap(dom.document.createElement('div').style) {}
On 2011/10/26 13:24:05, antonmuhin wrote:
> Why it's needed?  I would really prefer not to have anything like that.
> 
> On 2011/10/26 01:03:13, jacobr wrote:
> > On 2011/10/26 00:27:37, nweiz wrote:
> > > On 2011/10/25 23:45:51, jacobr wrote:
> > > > wouldn't 
> > > > super._wrap(null);
> > > > 
> > > > be sufficient?
> > > > 
> > > > also, omit {} for empty constructor bodies.
> > > 
> > > DOMWrapperBase assumes _ptr is non-null.
> > 
> > I'd suggest adding an alternate 
> > _empty()
> > constructor  to DOMWrapperBase.
> > which sets _ptr to null.
> 

I think it's useful to have this constructor wrap an empty style declaration
anyway; that was if new accessors are added EmptyStyleDeclaration doesn't break.

[Jacob, 2011-10-27 01:03:44]

Also, I should have noticed this in the previous review, but:
DocumentFragmentWrappingImplementation needs to cache the computed
EmptyStyleDeclaration so that
myElement.style === myElement.style

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8341027/diff/1/client/html/src/DocumentFragmen...
client/html/src/DocumentFragmentWrappingImplementation.dart:112: :
super._wrap(dom.document.createElement('div').style) {}
On 2011/10/26 17:30:53, nweiz wrote:
> On 2011/10/26 13:24:05, antonmuhin wrote:
> > Why it's needed?  I would really prefer not to have anything like that.
> > 
> > On 2011/10/26 01:03:13, jacobr wrote:
> > > On 2011/10/26 00:27:37, nweiz wrote:
> > > > On 2011/10/25 23:45:51, jacobr wrote:
> > > > > wouldn't 
> > > > > super._wrap(null);
> > > > > 
> > > > > be sufficient?
> > > > > 
> > > > > also, omit {} for empty constructor bodies.
> > > > 
> > > > DOMWrapperBase assumes _ptr is non-null.
> > > 
> > > I'd suggest adding an alternate 
> > > _empty()
> > > constructor  to DOMWrapperBase.
> > > which sets _ptr to null.
> > 
> 
> I think it's useful to have this constructor wrap an empty style declaration
> anyway; that was if new accessors are added EmptyStyleDeclaration doesn't
break.

I disagree with this reasoning because if the dummy style declaration created
here is ever used to set a property value (due to some hypothetical new
CSSStyleDeclaration method) then the behavior desired for this class will be
broken in surprising ways as now it would be possible to actually set styles
even though this class is supposed to be frozen.

Please create a new CL that fixes this issue.