Issue 10977017: Adding constructor to ContentElement.

Issue 10977017: Adding constructor to ContentElement. (Closed)

Created:
8 years, 2 months ago by blois

Modified:
8 years, 2 months ago

Reviewers:
vsm, Emily Fortuna

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Adding constructor to ContentElement. This is the same CL as http://code.google.com/p/dart/source/detail?r=12852, but changing the test status to pass/fail on Chrome since the test works on very new builds, but fails on the version on the bots. BUG=3870 Committed: https://code.google.com/p/dart/source/detail?r=12888

Patch Set 1 #

Patch Set 2 : Fixing the test. #

Total comments: 5

Patch Set 3 : Incorporating review feedback. #

Created: 8 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+25 lines, -8 lines)			Patch
M	lib/html/dart2js/html_dart2js.dart	View		2 chunks	+7 lines, -0 lines	0 comments	Download
M	lib/html/dartium/html_dartium.dart	View		2 chunks	+7 lines, -0 lines	0 comments	Download
M	lib/html/scripts/systemhtml.py	View		1 chunk	+1 line, -0 lines	0 comments	Download
A +	tests/html/contentelement_test.dart	View	1 2	2 chunks	+5 lines, -8 lines	0 comments	Download
M	tests/html/html.status	View	1 2	3 chunks	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Emily Fortuna

lgtm with comments https://codereview.chromium.org/10977017/diff/2001/tests/html/contentelement_test.dart File tests/html/contentelement_test.dart (right): https://codereview.chromium.org/10977017/diff/2001/tests/html/contentelement_test.dart#newcode15 tests/html/contentelement_test.dart:15: Expect.isTrue(e is ContentElement); Is there anything ...

8 years, 2 months ago (2012-09-25 17:39:22 UTC) #3

vsm

https://codereview.chromium.org/10977017/diff/2001/tests/html/contentelement_test.dart File tests/html/contentelement_test.dart (right): https://codereview.chromium.org/10977017/diff/2001/tests/html/contentelement_test.dart#newcode15 tests/html/contentelement_test.dart:15: Expect.isTrue(e is ContentElement); Good point. Doesn't this work? expect(e ...

8 years, 2 months ago (2012-09-25 17:40:37 UTC) #4

blois

8 years, 2 months ago (2012-09-25 22:19:56 UTC) #5

https://codereview.chromium.org/10977017/diff/2001/tests/html/contentelement_...
File tests/html/contentelement_test.dart (right):

https://codereview.chromium.org/10977017/diff/2001/tests/html/contentelement_...
tests/html/contentelement_test.dart:15: Expect.isTrue(e is ContentElement);
On 2012/09/25 17:40:37, vsm wrote:
> Good point.  Doesn't this work?
> 
> expect(e is ContentElement);
> 
> I think that's the new style for this.
> 
> On 2012/09/25 17:39:22, Emily Fortuna wrote:
> > Is there anything our test framework needs to write better tests for this?
> 

Done.

https://codereview.chromium.org/10977017/diff/2001/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/10977017/diff/2001/tests/html/html.status#new...
tests/html/html.status:13: contentelement_test: Fail, Pass   # Issue 5445: not
supported on chrome stable.
On 2012/09/25 17:39:22, Emily Fortuna wrote:
> I'd suggest putting this as strictly Pass or Fail, whichever it's doing
> currently, and put a comment. We can look to see if we can give more specifics
> on listing particular versions of chrome in the status file.

Done.

Expand Messages | Collapse Messages