Make Attr.value/nodeValue/textContent not nullable

Make Attr.value/nodeValue/textContent not nullable

https://dom.spec.whatwg.org/#interface-attr

The [TreatNullAs=EmptyString] addition has been proposed:
https://github.com/whatwg/dom/pull/36

An ad-hoc test for the behavior of setting these to null:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3528

Firefox Nightly:
attr.value=null => getAttribute()=="null"
attr.nodeValue=null => getAttribute()==""
attr.textContent=null => getAttribute()==""

IE11 and Edge:
attr.value=null => getAttribute()=="null"
attr.nodeValue=null => getAttribute()==""
attr.textContent=null => getAttribute()==""

Safari 8:
attr.value=null => getAttribute()==null
attr.nodeValue=null => getAttribute()==null
attr.textContent=null => getAttribute()==""

Blink before this change:
attr.value=null => getAttribute()==null
attr.nodeValue=null => getAttribute()==null
attr.textContent=null => getAttribute()==null

Blink after this change:
attr.value=null => getAttribute()=="null"
attr.nodeValue=null => getAttribute()==""
attr.textContent=null => getAttribute()==""

Note that in the getAttribute()==null cases in WebKit and Blink,
hasAttribute() will still return true, which is very strange.

This change brings Blink into alignment with Firefox and IE, so the
risk of breakage ought to be very low.

The update-attribute-node-null-value-no-crash.html test serves no
purpose when Attr.value cannot be null, so it is removed.

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

[philipj_slow, 2015-05-22 08:05:43]

philipj@opera.com changed reviewers:
+ jl@opera.com

[philipj_slow, 2015-05-22 08:05:44]

PTAL

[vivekg, 2015-05-22 08:14:37]

LGTM. Do we have test coverage for these cases? It would be great addition if
not already there.

[Jens Widell, 2015-05-22 08:26:39]

LGTM

If we add [TreatNullAs=EmptyString] to nodeValue/textContent, we'd be fully
compatible with FF/IE, I guess? But less spec compliant.

It looks like nodeValue is used on 1 % of page loads, but that doesn't say much
about how common assigning null is. Hopefully not common at all.

[philipj_slow, 2015-05-22 09:17:48]

On 2015/05/22 08:14:37, vivekg_ wrote:
> LGTM. Do we have test coverage for these cases? It would be great addition if
> not already there.

Oops, I updated the tests but apparently forgot to upload them, but did remember
to update the description to mention it. Will fix :)

[philipj_slow, 2015-05-22 09:29:37]

On 2015/05/22 08:26:39, Jens Widell wrote:
> LGTM
> 
> If we add [TreatNullAs=EmptyString] to nodeValue/textContent, we'd be fully
> compatible with FF/IE, I guess? But less spec compliant.

Yeah. Since value/nodeValue/textContent are all aliases I think the same
behavior for all of them is preferable, but if it turns out there's content that
depends on "if after setting attr.value to null and it is a string the string
must be empty" it's an easy fix. In at least Gecko, the reason for difference
appears to be that they don't have nodeValue and textContent in their
Attr.webidl.

[philipj_slow, 2015-05-22 09:41:12]

update tests

[vivekg, 2015-05-22 09:54:58]

On 2015/05/22 at 09:41:12, philipj wrote:
> update tests

LGTM

[philipj_slow, 2015-05-22 09:56:02]

Hmm, 1% usage of nodeValue makes me nervous, I'll try to change the spec:
https://github.com/whatwg/dom/pull/36

[philipj_slow, 2015-05-22 11:07:34]

Add [TreatNullAs=EmptyString] to Attr.nodeValue/textContent

[philipj_slow, 2015-05-22 11:30:42]

Can you take another look? I went with the [TreatNullAs=EmptyString] change and
think it's likely that the spec change will be accepted. (If not, I'll just
treat that as a separate change.)

[Jens Widell, 2015-05-22 11:37:41]

LGTM

[philipj_slow, 2015-05-22 12:02:53]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-05-22 12:02:53]

The patchset sent to the CQ was uploaded after l-g-t-m from vivekg@chromium.org
Link to the patchset: https://codereview.chromium.org/1146393003/#ps40001
(title: "Add [TreatNullAs=EmptyString] to Attr.nodeValue/textContent")

[commit-bot: I haz the power, 2015-05-22 12:03:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146393003/40001

[commit-bot: I haz the power, 2015-05-22 13:19:45]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195785