Make [TreatNullAs=NullString] DOMString arguments nullable instead

Make [TreatNullAs=NullString] DOMString arguments nullable instead

An IDL method declaration such as

  void f([TreatNullAs=NullString] DOMString x);

can be transformed into

  void f(DOMString? x);

without affecting behavior or generated code, so do that.

Arguments that also have [TreatUndefinedAs=NullString] can't be converted
this way (yet), so this CL leaves them unchanged for now.

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

[Jens Widell, 2014-07-01 13:21:37]

PTAL

I will address the [TreatNullAs=NullString, TreatUndefinedAs=NullString] case in
a follow-up; shouldn't be too complicated.

[haraken, 2014-07-01 13:37:07]

I remembered that [TreatNullAs] is an IDL extended attribute defined in the Web
IDL spec.
http://www.w3.org/TR/WebIDL/#TreatNullAs

Our situation is a bit complicated.

First of all:

  [TreatNullAs=EmptyString] attribute DOMString owner;

is a valid IDL in terms of the Web IDL spec.

Blink implements it as:

  [TreatNullAs=NullString] attribute DOMString owner;

for performance & memory optimization. A null string and an empty string are not
distinguishable in terms of web-exposed behavior, and a null string is more
light-weight than an empty string.

Now we're changing the IDL to:

  attribute DOMString? owner;

I just want to make sure that this is an expected change for you.

(As we discussed in another thread, regarding nullable/optional/missing, it's OK
to not strictly following the Web IDL spec. As long as our syntax is consistent,
easy to understand and covers 99% of real IDL files, we're fine with the
syntax.)

[Jens Widell, 2014-07-01 13:56:37]

On 2014/07/01 13:37:07, haraken wrote:
> I remembered that [TreatNullAs] is an IDL extended attribute defined in the
Web
> IDL spec.
> http://www.w3.org/TR/WebIDL/#TreatNullAs

> I just want to make sure that this is an expected change for you.

Yeah, it can certainly be discussed what's actually better, the more compact,
and perhaps more readable "attribute DOMString?" or the
"[TreatNullAs=NullString] attribute DOMString" that is in some sense more
similar to the spec.

In practice, "attribute DOMString?" is what we actually implemented for the
"[TreatReturnedNullStringAs=Null, TreatNullAs=NullString]" attributes that I've
already converted, so I guess it's kind of honest to label them as such in the
IDL as well.

This patch doesn't touch any attributes, only method arguments, but for them the
same applies: in some cases that I change here the spec is actually
"[TreatNullAs=EmptyString] DOMString" rather than "DOMString?". But in those
cases too, "DOMString?" is what we actually have implemented, so it makes some
sense to actually label them as that.

For writable attributes, changing "[TreatNullAs=NullString] attribute DOMString"
into "attribute DOMString?" is somewhat more questionable, since [TreatNullAs]
only affects the setter, whereas "DOMString?" affects both the getter and the
setter, and can change what the getter returns (from the string value "null" to
the null value, if the C++ getter returns a null string).

[haraken, 2014-07-01 14:00:05]

On 2014/07/01 13:56:37, Jens Lindström wrote:
> On 2014/07/01 13:37:07, haraken wrote:
> > I remembered that [TreatNullAs] is an IDL extended attribute defined in the
> Web
> > IDL spec.
> > http://www.w3.org/TR/WebIDL/#TreatNullAs
> 
> > I just want to make sure that this is an expected change for you.
> 
> Yeah, it can certainly be discussed what's actually better, the more compact,
> and perhaps more readable "attribute DOMString?" or the
> "[TreatNullAs=NullString] attribute DOMString" that is in some sense more
> similar to the spec.
> 
> In practice, "attribute DOMString?" is what we actually implemented for the
> "[TreatReturnedNullStringAs=Null, TreatNullAs=NullString]" attributes that
I've
> already converted, so I guess it's kind of honest to label them as such in the
> IDL as well.
> 
> This patch doesn't touch any attributes, only method arguments, but for them
the
> same applies: in some cases that I change here the spec is actually
> "[TreatNullAs=EmptyString] DOMString" rather than "DOMString?". But in those
> cases too, "DOMString?" is what we actually have implemented, so it makes some
> sense to actually label them as that.

Regarding this part, I agree with your idea.

> For writable attributes, changing "[TreatNullAs=NullString] attribute
DOMString"
> into "attribute DOMString?" is somewhat more questionable, since [TreatNullAs]
> only affects the setter, whereas "DOMString?" affects both the getter and the
> setter, and can change what the getter returns (from the string value "null"
to
> the null value, if the C++ getter returns a null string).

Is there any such attribute in real IDL files?

[Jens Widell, 2014-07-01 14:11:05]

On 2014/07/01 14:00:05, haraken wrote:
> > For writable attributes, changing "[TreatNullAs=NullString] attribute
> DOMString"
> > into "attribute DOMString?" is somewhat more questionable, since
[TreatNullAs]
> > only affects the setter, whereas "DOMString?" affects both the getter and
the
> > setter, and can change what the getter returns (from the string value "null"
> to
> > the null value, if the C++ getter returns a null string).
> 
> Is there any such attribute in real IDL files?

There are quite a lot of them, actually. Many attributes reflecting attributes
on HTML elements are specified this way. They should never return null (they
return "" when attributes are missing), so declaring them "DOMString?" is
misleading, and also leads to the generated getter behaving incorrectly in case
the C++ getter returns a null string.

So I think we need to keep [TreatNullAs=NullString] for these attributes, at
least.

[haraken, 2014-07-01 15:34:11]

On 2014/07/01 14:11:05, Jens Lindström wrote:
> On 2014/07/01 14:00:05, haraken wrote:
> > > For writable attributes, changing "[TreatNullAs=NullString] attribute
> > DOMString"
> > > into "attribute DOMString?" is somewhat more questionable, since
> [TreatNullAs]
> > > only affects the setter, whereas "DOMString?" affects both the getter and
> the
> > > setter, and can change what the getter returns (from the string value
"null"
> > to
> > > the null value, if the C++ getter returns a null string).
> > 
> > Is there any such attribute in real IDL files?
> 
> There are quite a lot of them, actually. Many attributes reflecting attributes
> on HTML elements are specified this way. They should never return null (they
> return "" when attributes are missing), so declaring them "DOMString?" is
> misleading, and also leads to the generated getter behaving incorrectly in
case
> the C++ getter returns a null string.
> 
> So I think we need to keep [TreatNullAs=NullString] for these attributes, at
> least.

ok, understand the situation.

Another idea would be:

- Deprecate [TreatNullAs=NullString].
- Deprecate [TreatReturnedNullStringAs=Null].
- Introduce [TreatReturnedNullStringAs=EmptyString].

Then the current '[TreatReturnedNullStringAs=Null, TreatNullAs=NullString]
attribute DOMString' becomes 'attribute DOMString?'. The current
'[TreatNullAs=NullString] attribute DOMString' becomes
'[TreatReturnedNullStringAs=EmptyString] attribute DOMString?'.

What do you think?

Either way, this CL LGTM.

[Jens Widell, 2014-07-01 15:44:26]

On 2014/07/01 15:34:11, haraken wrote:
> Another idea would be:
> 
> - Deprecate [TreatNullAs=NullString].
> - Deprecate [TreatReturnedNullStringAs=Null].
> - Introduce [TreatReturnedNullStringAs=EmptyString].
> 
> Then the current '[TreatReturnedNullStringAs=Null, TreatNullAs=NullString]
> attribute DOMString' becomes 'attribute DOMString?'. The current
> '[TreatNullAs=NullString] attribute DOMString' becomes
> '[TreatReturnedNullStringAs=EmptyString] attribute DOMString?'.
> 
> What do you think?

That would work too, but it feels like we move farther away from the
specification then; we get "wrong nullable" and "custom extended attribute"
instead of just "custom value for spec's extended attribute". For someone
implementing a specification, the path from specification text to what you need
to put in the IDL file becomes rather more unclear, I think.

We could also investigate how much we actually gain from using null strings
where the specifications have empty strings (given WTF::emptyString() I wouldn't
expect it to be an enormous gain exactly) and, depending on the result, replace
[TreatNullAs=NullString] with [TreatNullAs=EmptyString].

[haraken, 2014-07-01 15:52:07]

On 2014/07/01 15:44:26, Jens Lindström wrote:
> On 2014/07/01 15:34:11, haraken wrote:
> > Another idea would be:
> > 
> > - Deprecate [TreatNullAs=NullString].
> > - Deprecate [TreatReturnedNullStringAs=Null].
> > - Introduce [TreatReturnedNullStringAs=EmptyString].
> > 
> > Then the current '[TreatReturnedNullStringAs=Null, TreatNullAs=NullString]
> > attribute DOMString' becomes 'attribute DOMString?'. The current
> > '[TreatNullAs=NullString] attribute DOMString' becomes
> > '[TreatReturnedNullStringAs=EmptyString] attribute DOMString?'.
> > 
> > What do you think?
> 
> That would work too, but it feels like we move farther away from the
> specification then; we get "wrong nullable" and "custom extended attribute"
> instead of just "custom value for spec's extended attribute". For someone
> implementing a specification, the path from specification text to what you
need
> to put in the IDL file becomes rather more unclear, I think.

Yeah, neither of our plans is perfect. I don't have a strong opinion here.

In your plan, we'll have two ways to express [TreatNullAs=NullString], which is
a bit confusing.

> We could also investigate how much we actually gain from using null strings
> where the specifications have empty strings (given WTF::emptyString() I
wouldn't
> expect it to be an enormous gain exactly) and, depending on the result,
replace
> [TreatNullAs=NullString] with [TreatNullAs=EmptyString].

I couldn't remember the details, but I remember that someone did the experiment
before (two years ago or so) and the conclusion was that performance was not
acceptable in Dromaeo. That being said, the situation would have changed a lot
since then, so re-investigation is welcome :)

[Jens Widell, 2014-07-01 16:00:08]

On 2014/07/01 15:52:07, haraken wrote:
> In your plan, we'll have two ways to express [TreatNullAs=NullString], which
is
> a bit confusing.

True, but we could restrict it to attributes (i.e. disallow for method
arguments,) where it's not exactly the same as making the attribute nullable,
since [TreatNullAs] only affects the getter.

[haraken, 2014-07-01 16:03:43]

On 2014/07/01 16:00:08, Jens Lindström wrote:
> On 2014/07/01 15:52:07, haraken wrote:
> > In your plan, we'll have two ways to express [TreatNullAs=NullString], which
> is
> > a bit confusing.
> 
> True, but we could restrict it to attributes (i.e. disallow for method
> arguments,) where it's not exactly the same as making the attribute nullable,
> since [TreatNullAs] only affects the getter.

Makes sense, let's go with the plan.

[Jens Widell, 2014-07-01 16:12:15]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-01 16:12:37]

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

[commit-bot: I haz the power, 2014-07-01 17:27:07]

Change committed as 177322

[Jens Widell, 2014-07-02 14:03:00]

On 2014/07/01 15:52:07, haraken wrote:
> On 2014/07/01 15:44:26, Jens Lindström wrote:
> > replace [TreatNullAs=NullString] with [TreatNullAs=EmptyString].
> 
> I couldn't remember the details, but I remember that someone did the
experiment
> before (two years ago or so) and the conclusion was that performance was not
> acceptable in Dromaeo. That being said, the situation would have changed a lot
> since then, so re-investigation is welcome :)

I've implemented [TreatNullAs=EmptyString] and converted all the attributes to
use it, and here are some random performance results (repeatedly assigning null
to the attribute):

- HTMLBodyElement.bgColor: slows down a lot, because assigning null to an
attribute that reflects a content attribute currently doesn't set, and in fact
removes, the reflected content attribute. This is wrong per HTML5 (if I read it
correctly) and doesn't match what Firefox (nightly) does. My EmptyString build
matches Firefox's behavior. IE11 doesn't create a missing content attribute when
you assign null, but it also doesn't remove an existing content attribute when
you assign null, so there's not much consensus here, I guess.

- HTMLInputElement.value: speeds up a lot, because initially, the 'value'
content attribute is returned from HTMLInputElement::value(), but once a
non-null value is set (which doesn't affect the content attribute, of course),
it's returned instead, which is much faster.

- HTMLTextAreaElement.value: no affected at all.

(These were all attributes I tested.)

My guess, based on this rather incomplete study, is:

- In the bindings layer, it doesn't really matter at all whether we use null or
empty strings. The only difference I can think of is that the empty string has a
StringImpl whose reference counter needs to be touched, whereas a null string
doesn't, but I think that difference is insignificant in practice.

- For reflected attributes, our current null behavior is certainly much faster
(in pathological cases) but it's also wrong per spec, in a way that is
observable to scripts.

- For other things, it can be better or worse, depending on the attribute. To
guard against regressions, we'd need to consider and test each one, I think.

[haraken, 2014-07-02 15:57:26]

On 2014/07/02 14:03:00, Jens Lindström wrote:
> On 2014/07/01 15:52:07, haraken wrote:
> > On 2014/07/01 15:44:26, Jens Lindström wrote:
> > > replace [TreatNullAs=NullString] with [TreatNullAs=EmptyString].
> > 
> > I couldn't remember the details, but I remember that someone did the
> experiment
> > before (two years ago or so) and the conclusion was that performance was not
> > acceptable in Dromaeo. That being said, the situation would have changed a
lot
> > since then, so re-investigation is welcome :)
> 
> I've implemented [TreatNullAs=EmptyString] and converted all the attributes to
> use it, and here are some random performance results (repeatedly assigning
null
> to the attribute):
> 
> - HTMLBodyElement.bgColor: slows down a lot, because assigning null to an
> attribute that reflects a content attribute currently doesn't set, and in fact
> removes, the reflected content attribute. This is wrong per HTML5 (if I read
it
> correctly) and doesn't match what Firefox (nightly) does. My EmptyString build
> matches Firefox's behavior. IE11 doesn't create a missing content attribute
when
> you assign null, but it also doesn't remove an existing content attribute when
> you assign null, so there's not much consensus here, I guess.
> 
> - HTMLInputElement.value: speeds up a lot, because initially, the 'value'
> content attribute is returned from HTMLInputElement::value(), but once a
> non-null value is set (which doesn't affect the content attribute, of course),
> it's returned instead, which is much faster.
> 
> - HTMLTextAreaElement.value: no affected at all.
> 
> (These were all attributes I tested.)
> 
> My guess, based on this rather incomplete study, is:
> 
> - In the bindings layer, it doesn't really matter at all whether we use null
or
> empty strings. The only difference I can think of is that the empty string has
a
> StringImpl whose reference counter needs to be touched, whereas a null string
> doesn't, but I think that difference is insignificant in practice.
> 
> - For reflected attributes, our current null behavior is certainly much faster
> (in pathological cases) but it's also wrong per spec, in a way that is
> observable to scripts.
> 
> - For other things, it can be better or worse, depending on the attribute. To
> guard against regressions, we'd need to consider and test each one, I think.

Thanks for the detailed study! Another thing you might want to do is to measure
performance in Dromaeo (http://dromaeo.com/?dom).

The reason why Blink's string has a concept of a null string is exactly because
a null string is faster and more light-weight than an empty string. In fact,
Blink uses a ton of null strings in the code base. The binding layer also uses a
null string for the same reason. So, in order to change the null string to an
empty string, we need to convince the community that the difference between a
null string or an empty string doesn't matter in the binding layer.

I think it's worth raising this discussion to blink-dev@ once you get enough
performance & memory results.

[Jens Widell, 2014-07-02 16:19:36]

On 2014/07/02 15:57:26, haraken wrote:
> Thanks for the detailed study! Another thing you might want to do is to
measure
> performance in Dromaeo (http://dromaeo.com/?dom).

Should have mentioned that I did that too. No significant changes, but I think
that's because none of the tests are testing anything that's relevant here.

> The reason why Blink's string has a concept of a null string is exactly
because
> a null string is faster and more light-weight than an empty string. In fact,
> Blink uses a ton of null strings in the code base. The binding layer also uses
a
> null string for the same reason. So, in order to change the null string to an
> empty string, we need to convince the community that the difference between a
> null string or an empty string doesn't matter in the binding layer.
> 
> I think it's worth raising this discussion to blink-dev@ once you get enough
> performance & memory results.

I don't think we should stop using null strings on any larger scale, really.
Nullable DOMString should certainly still use null strings. And for all sorts of
"internal" usage I have no opinion at all, and see no reason to change anything.

What I think we're really dealing with here is a spec compliance issue: we treat
"body.bgColor=null" as something other than what spec says and Firefox+IE do.
The way we've implemented this spec compliance issue is by using
[TreatNullAs=NullString] and by handling those null strings as "remove the
attribute". The obvious way to fix it would be to add [TreatNullAs=EmptyString].

We may not want to fix it, for performance reasons and memory usage reasons.
There's certainly a huge performance impact here (when assigning null to these
attributes,) but it's all about what Element::setAttributeInternal() does, not
about how the bindings code translated the value, I'm quite sure.

[Jens Widell, 2014-07-02 16:24:24]

Looks like http://crbug.com/310298 and http://crbug.com/304959 are related to
this discussion (which might have made more sense to have in one of those issues
instead.)