Make 'crossOrigin' IDL attributes nullable

Make 'crossOrigin' IDL attributes nullable

This affects the 'crossOrigin' IDL attributes in the interfaces
HTMLImageElement, HTMLLinkElement, HTMLMediaElement and HTMLScriptElement,
and is done to match the corresponding specification change:
  http://html5.org/r/8727

The effect is that on setting the IDL attribute to null, the content
attribute is removed (if present) instead of set to the string "null", and
on getting null is returned instead of the empty string if the content
attribute is not present.

BUG=409524
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181728

[Jens Widell, 2014-09-02 10:33:22]

jl@opera.com changed reviewers:
+ arv@chromium.org, haraken@chromium.org, sigbjornf@opera.com

[Jens Widell, 2014-09-02 10:33:22]

PTAL

[haraken, 2014-09-02 10:37:04]

What's a behavior of other browsers?

[Jens Widell, 2014-09-02 10:38:21]

On 2014/09/02 10:37:04, haraken wrote:
> What's a behavior of other browsers?

I believe they all currently implement the old behavior.

[haraken, 2014-09-02 10:41:25]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org, tkent@chromium.org

[haraken, 2014-09-02 10:41:26]

On 2014/09/02 10:38:21, Jens Widell wrote:
> On 2014/09/02 10:37:04, haraken wrote:
> > What's a behavior of other browsers?
> 
> I believe they all currently implement the old behavior.

OK. Then I'd like to delegate the decision to API owners.

+jochen, +tkent

[sof, 2014-09-02 13:04:39]

lgtm wrt now-spec'ed behavior.

It is a slight worry that (elm.crossOrigin = '') is now not equal to
(elm.crossOrigin = null) (right?); have you evaluated the risks of making this
user-visible change (with a code search or otherwise)?

[Jens Widell, 2014-09-02 13:30:33]

On 2014/09/02 13:04:39, sof wrote:
> lgtm wrt now-spec'ed behavior.
> 
> It is a slight worry that (elm.crossOrigin = '') is now not equal to
> (elm.crossOrigin = null) (right?); have you evaluated the risks of making this
> user-visible change (with a code search or otherwise)?

They weren't strictly equal before either; one set the attribute to the empty
string and the other set it to the string "null".

I have done some code searching at GitHub (which I'm typically not at all good
at it) and have essentially found test cases checking crossOrigin-vs-null
behavior, and (non-current forks of) the three.js library that does

  if (this.crossOrigin) image.crossOrigin = this.crossOrigin;

where |this| is a plain JS object whose |crossOrigin| attribute is initialized
to null, and |image| is an HTMLImageElement.

[sof, 2014-09-02 13:39:14]

On 2014/09/02 13:30:33, Jens Widell wrote:
> On 2014/09/02 13:04:39, sof wrote:
> > lgtm wrt now-spec'ed behavior.
> > 
> > It is a slight worry that (elm.crossOrigin = '') is now not equal to
> > (elm.crossOrigin = null) (right?); have you evaluated the risks of making
this
> > user-visible change (with a code search or otherwise)?
> 
> They weren't strictly equal before either; one set the attribute to the empty
> string and the other set it to the string "null".
> 

I'm guessing scripts might care more about the IDL attribute crossOrigin; where
the two assignments both mapped to "anonymous" before.

[arv (Not doing code reviews), 2014-09-03 01:16:03]

https://codereview.chromium.org/529933002/diff/1/Source/core/html/HTMLImageEl...
File Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/529933002/diff/1/Source/core/html/HTMLImageEl...
Source/core/html/HTMLImageElement.idl:31: [Reflect,
ReflectOnly=("anonymous","use-credentials"), ReflectEmpty="anonymous",
ReflectInvalid="anonymous"] attribute DOMString? crossOrigin;
How does ReflectEmpty work here?

[sof, 2014-09-03 18:30:58]

https://codereview.chromium.org/529933002/diff/1/Source/core/html/HTMLImageEl...
File Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/529933002/diff/1/Source/core/html/HTMLImageEl...
Source/core/html/HTMLImageElement.idl:31: [Reflect,
ReflectOnly=("anonymous","use-credentials"), ReflectEmpty="anonymous",
ReflectInvalid="anonymous"] attribute DOMString? crossOrigin;
On 2014/09/03 01:16:03, arv wrote:
> How does ReflectEmpty work here?

The concepts could be stratified more clearly, but [ReflectEmpty] handles the
mapping from the empty content attribute keyword value, <foo crossorigin>, onto
the attribute state "anonymous" (which map to the IDL attribute value
"anonymous".)

The CORS setting attribute defines such an empty keyword,

 http://www.whatwg.org/specs/web-apps/current-work/#cors-settings-attribute

[ReflectEmpty] overlaps with the setting of elm.crossOrigin = '', but that's the
only intersection I can think of wrt the spec change here.

Water less muddy?

[tkent, 2014-09-09 02:19:42]

On 2014/09/02 10:38:21, Jens Widell wrote:
> On 2014/09/02 10:37:04, haraken wrote:
> > What's a behavior of other browsers?
> 
> I believe they all currently implement the old behavior.

Do other browser vendors agree with the behavior change?

[mlamouri (slow - plz ping), 2014-09-09 09:26:01]

On 2014/09/09 02:19:42, tkent (overloaded) wrote:
> On 2014/09/02 10:38:21, Jens Widell wrote:
> > On 2014/09/02 10:37:04, haraken wrote:
> > > What's a behavior of other browsers?
> > 
> > I believe they all currently implement the old behavior.
> 
> Do other browser vendors agree with the behavior change?

Mozilla does:
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLLinkElement.webidl
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLScriptElement.we...

[tkent, 2014-09-09 23:44:48]

On 2014/09/09 09:26:01, Mounir Lamouri wrote:
> On 2014/09/09 02:19:42, tkent (overloaded) wrote:
> > On 2014/09/02 10:38:21, Jens Widell wrote:
> > > On 2014/09/02 10:37:04, haraken wrote:
> > > > What's a behavior of other browsers?
> > > 
> > > I believe they all currently implement the old behavior.
> > 
> > Do other browser vendors agree with the behavior change?
> 
> Mozilla does:
>
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLLinkElement.webidl
>
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLScriptElement.we...

Thank you for the information.  Then, lgtm.

However, we should not change a specification if the specification and major
browsers agree on a behavior.  Changing an existing behavior would make
incompatibility and make web authors unhappy.
For example, the specification and major browsers had the same behavior of
'formMethod' IDL attribute.  Someone asked to change its behavior, and the
specification was updated.  WebKit and Firefox followed the behavior change, but
IE didn't follow it.

[Jens Widell, 2014-09-10 11:09:09]

On 2014/09/09 23:44:48, tkent (overloaded) wrote:
> On 2014/09/09 09:26:01, Mounir Lamouri wrote:
> > On 2014/09/09 02:19:42, tkent (overloaded) wrote:
> > > On 2014/09/02 10:38:21, Jens Widell wrote:
> > > > On 2014/09/02 10:37:04, haraken wrote:
> > > > > What's a behavior of other browsers?
> > > > 
> > > > I believe they all currently implement the old behavior.
> > > 
> > > Do other browser vendors agree with the behavior change?
> > 
> > Mozilla does:
> >
>
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLLinkElement.webidl
> >
>
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLScriptElement.we...
> 
> Thank you for the information.  Then, lgtm.

Many thanks!

> However, we should not change a specification if the specification and major
> browsers agree on a behavior.  Changing an existing behavior would make
> incompatibility and make web authors unhappy.
> For example, the specification and major browsers had the same behavior of
> 'formMethod' IDL attribute.  Someone asked to change its behavior, and the
> specification was updated.  WebKit and Firefox followed the behavior change,
but
> IE didn't follow it.

This is mostly a discussion for a different place, I guess. I agree that we
should be careful changing already established compatible behavior, but on the
other hand, we should be able to improve APIs even after they've been
implemented, given a positive cost-benefit review and all that. (I don't know
what took place with the 'formMethod' attribute, BTW.)

[Jens Widell, 2014-09-10 11:20:45]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-10 11:21:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/529933002/20001

[commit-bot: I haz the power, 2014-09-10 13:12:00]

Committed patchset #2 (id:20001) as 181728