Strong html

Strong html

BUG=
R=alanknight@google.com

Committed: https://github.com/dart-lang/sdk/commit/864b64fd5e7f55a154d67b73e03da8afbe1bfe5d

[Jacob, 2016-04-19 17:12:28]

jacobr@google.com changed reviewers:
+ alanknight@google.com

[Jacob, 2016-04-19 17:12:29]

https://codereview.chromium.org/1894713002/diff/40001/tools/dom/src/EventStre...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/1894713002/diff/40001/tools/dom/src/EventStre...
tools/dom/src/EventStreamProvider.dart:235: this._useCapture) : _onData =
_wrapZone/*<Event, dynamic>*/(onData) {
This is the most problematic of the dart:html changes.
I tried to change this to _wrapZone/*<Event, dynamic>*/((e) => onData(e as T))
but it broke way too many tests as it is too hard and sometimes impossible to
pass correct event subtypes to event listeners for tests.
Over 100 co19 tests broke because of this and I would expect many more could as
well.
The other option to fix this would be to make all event streams in the dom just
be EventStreamSubscription<Event> but that would be breaking change and lose
useful documentation information we currently provide.

[Jacob, 2016-04-19 17:13:31]

cced leafp to look at EventStreamProvider issue where we can't do the "right"
thing from a strong mode perspective without breaking 100 tests.

[Alan Knight, 2016-04-19 18:24:34]

https://codereview.chromium.org/1894713002/diff/60001/sdk/lib/html/html_commo...
File sdk/lib/html/html_common/css_class_set.dart (right):

https://codereview.chromium.org/1894713002/diff/60001/sdk/lib/html/html_commo...
sdk/lib/html/html_common/css_class_set.dart:59: Iterable/*<T>*/
map/*<T>*/(/*=T*/ f(String e)) =>
Where does T come from? Is it the <String> in CssClassSet? Would it make more
sense to hard-code these as String rather than a generic parameter that's fixed?

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/scripts/gos.sh
File tools/dom/scripts/gos.sh (right):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/scripts/gos.s...
tools/dom/scripts/gos.sh:2: git checkout
../../../sdk/lib/js/dartium/cached_patches.dart
Do we need this any more with Terry's change to the patch generation?

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/src/NodeValid...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/src/NodeValid...
tools/dom/src/NodeValidatorBuilder.dart:158: .map /*<String>*/ ((name) =>
'$tagNameUpper::${name.toLowerCase()}')
A null-aware operator to simplify this? I think it's just
  attributes?.map(...

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/templates/htm...
File tools/dom/templates/html/impl/impl_HTMLDocument.darttemplate (left):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/templates/htm...
tools/dom/templates/html/impl/impl_HTMLDocument.darttemplate:131: * Returns the
element, if any, that is currently displayed in fullscreen.
Are these removed? I don't see them being added back elsewhere.

[Jacob, 2016-04-19 21:50:46]

https://codereview.chromium.org/1894713002/diff/60001/sdk/lib/html/html_commo...
File sdk/lib/html/html_common/css_class_set.dart (right):

https://codereview.chromium.org/1894713002/diff/60001/sdk/lib/html/html_commo...
sdk/lib/html/html_common/css_class_set.dart:59: Iterable/*<T>*/
map/*<T>*/(/*=T*/ f(String e)) =>
On 2016/04/19 18:24:34, Alan Knight wrote:
> Where does T come from? Is it the <String> in CssClassSet? Would it make more
> sense to hard-code these as String rather than a generic parameter that's
fixed?

This is the correct form for the now generic map method.
CssClassSet doesn't specify this method... it is just inherited from Iterable.
Were you looking in the dartdocs or at the source code?

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/scripts/gos.sh
File tools/dom/scripts/gos.sh (right):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/scripts/gos.s...
tools/dom/scripts/gos.sh:2: git checkout
../../../sdk/lib/js/dartium/cached_patches.dart
On 2016/04/19 18:24:34, Alan Knight wrote:
> Do we need this any more with Terry's change to the patch generation?
Sorry didn't mean to include this script in the cl. Reverted.

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/src/NodeValid...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/src/NodeValid...
tools/dom/src/NodeValidatorBuilder.dart:158: .map /*<String>*/ ((name) =>
'$tagNameUpper::${name.toLowerCase()}')
On 2016/04/19 18:24:34, Alan Knight wrote:
> A null-aware operator to simplify this? I think it's just
>   attributes?.map(...
Much better... Done.

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/templates/htm...
File tools/dom/templates/html/impl/impl_HTMLDocument.darttemplate (left):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/templates/htm...
tools/dom/templates/html/impl/impl_HTMLDocument.darttemplate:131: * Returns the
element, if any, that is currently displayed in fullscreen.
On 2016/04/19 18:24:34, Alan Knight wrote:
> Are these removed? I don't see them being added back elsewhere.

These were also in Document... somewhat accidentally. I've added the backwards
compatibility support provided by Document.visibilityState to the document
template so we don't break IE 10.

[Alan Knight, 2016-04-19 22:04:08]

lgtm

https://codereview.chromium.org/1894713002/diff/60001/sdk/lib/html/html_commo...
File sdk/lib/html/html_common/css_class_set.dart (right):

https://codereview.chromium.org/1894713002/diff/60001/sdk/lib/html/html_commo...
sdk/lib/html/html_common/css_class_set.dart:59: Iterable/*<T>*/
map/*<T>*/(/*=T*/ f(String e)) =>
On 2016/04/19 21:50:45, Jacob wrote:
> On 2016/04/19 18:24:34, Alan Knight wrote:
> > Where does T come from? Is it the <String> in CssClassSet? Would it make
more
> > sense to hard-code these as String rather than a generic parameter that's
> fixed?
> 
> This is the correct form for the now generic map method.
> CssClassSet doesn't specify this method... it is just inherited from Iterable.
> Were you looking in the dartdocs or at the source code?

I was thinking that because we get Iterable through CssClassSet, which
implements Set<String> that T would always be String. But that's not true. The
input is always String, but we could map it to anything. Disregard.

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/templates/htm...
File tools/dom/templates/html/impl/impl_HTMLDocument.darttemplate (left):

https://codereview.chromium.org/1894713002/diff/60001/tools/dom/templates/htm...
tools/dom/templates/html/impl/impl_HTMLDocument.darttemplate:131: * Returns the
element, if any, that is currently displayed in fullscreen.
On 2016/04/19 21:50:46, Jacob wrote:
> On 2016/04/19 18:24:34, Alan Knight wrote:
> > Are these removed? I don't see them being added back elsewhere.
> 
> These were also in Document... somewhat accidentally. I've added the backwards
> compatibility support provided by Document.visibilityState to the document
> template so we don't break IE 10.

OK

[Jacob, 2016-04-19 22:08:49]

Description was changed from

==========
Strong html

BUG=
==========

to

==========
Strong html

BUG=
R=alanknight@google.com

Committed:
https://github.com/dart-lang/sdk/commit/864b64fd5e7f55a154d67b73e03da8afbe1bfe5d
==========

[Jacob, 2016-04-19 22:08:51]

Committed patchset #5 (id:80001) manually as
864b64fd5e7f55a154d67b73e03da8afbe1bfe5d (presubmit successful).

[kevmoo, 2016-04-19 22:30:55]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2016-04-19 22:30:56]

DBQ: we should be documenting API changes as they happen here.

Why is the error event stream no longer Stream<ErrorEvent> ?

Why the switch from double to num on some APIs?

Looks like a few things were hidden – any breaks here?

[Leaf, 2016-04-19 23:01:14]

leafp@google.com changed reviewers:
+ leafp@google.com

[Leaf, 2016-04-19 23:01:15]

https://codereview.chromium.org/1894713002/diff/40001/tools/dom/src/EventStre...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/1894713002/diff/40001/tools/dom/src/EventStre...
tools/dom/src/EventStreamProvider.dart:235: this._useCapture) : _onData =
_wrapZone/*<Event, dynamic>*/(onData) {
On 2016/04/19 17:12:29, Jacob wrote:
> This is the most problematic of the dart:html changes.
> I tried to change this to _wrapZone/*<Event, dynamic>*/((e) => onData(e as T))
> but it broke way too many tests as it is too hard and sometimes impossible to
> pass correct event subtypes to event listeners for tests.
> Over 100 co19 tests broke because of this and I would expect many more could
as
> well.
> The other option to fix this would be to make all event streams in the dom
just
> be EventStreamSubscription<Event> but that would be breaking change and lose
> useful documentation information we currently provide.

Do tests that do this screwy stuff pass in an onData that is dynamic -> void? 
If so, you could possibly force a dcall here so that you only fail if the even
is the wrong type and onData doesn't accept dynamic?  If I'm understanding the
problem correctly.

[Jacob, 2016-04-19 23:03:26]

On 2016/04/19 22:30:56, kevmoo wrote:
> DBQ: we should be documenting API changes as they happen here.
Where would you like the API fixes to be documented?
> 
> Why is the error event stream no longer Stream<ErrorEvent> ?
It was an error for the api to ever be Stream<ErrorEvent> as far as I can tell.
Note some other error event streams were already just Stream<Event>.

> 
> Why the switch from double to num on some APIs?
Which APIs changed? Please add comments inline with any changes that concern
you.

> 
> Looks like a few things were hidden –&nbsp;any breaks here?
Nothing was hidden. Previously we had some accidental duplicates which strong
mode caught due to the prohibition of overriding final fields.