Bug 154662 – Drop [TreatReturnedNullStringAs=Undefined] WebKit-specific IDL attribute

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Drop [TreatReturnedNullStringAs=Undefined] WebKit-specific IDL attribute. This has no standard equivalent, it is very rarely used in WebKit and when it is, it is either useless or wrong.

[Chris Dumez]

Created attachment 272167 [details]
Patch

[Alex Christensen]

Comment on attachment 272167 [details]
Patch

I think a patch like this ought to add some tests.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 272167 [details]
> Patch
> 
> I think a patch like this ought to add some tests.

Really? This patch has no web-exposed behavior change apart from this minor one:

Document.defaultCharset used to return undefined for *frameless* documents and now it returns "UTF-8" in this case (Its behavior is unchanged for regular documents). As mentioned in the changelog, this attribute is non-standard and not supported by other browsers except IE. I personally think we should drop it entirely but this should not be done in this patch.

[Darin Adler]

(In reply to comment #3)
> (In reply to comment #2)
> > I think a patch like this ought to add some tests.
> 
> Really? This patch has no web-exposed behavior change apart from this minor
> one:
> 
> Document.defaultCharset used to return undefined for *frameless* documents
> and now it returns "UTF-8" in this case (Its behavior is unchanged for
> regular documents). As mentioned in the changelog, this attribute is
> non-standard and not supported by other browsers except IE. I personally
> think we should drop it entirely but this should not be done in this patch.

Simple tests, either existing ones or new ones, are the cleanest way to show that this has no web-exposed behavior.

Also, I *do* think it would be useful to have a test to show the minor behavior change of defaultCharset even if you think we are deleting it soon.

[Darin Adler]

Comment on attachment 272167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272167&action=review

I agree with Alex that it would be better to have a little bit of test coverage, just for defaultCharset. However, my comment about other tests was wrong. I see now that there is nothing to test!

> Source/WebCore/ChangeLog:25
> +        - Drop extented attribute for Document.defaultCharset as the

Typo: extented

> Source/WebCore/ChangeLog:27
> +        - Drop extented attribute for Document.readyState. It was useless

Typo: extented

> Source/WebCore/dom/Document.cpp:1297
> +    return UTF8Encoding().domName();

This may seem more elegant to you but I do not think this is as good as returning ASCIILiteral("UTF-8").

[Chris Dumez]

Comment on attachment 272167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272167&action=review

>> Source/WebCore/dom/Document.cpp:1297
>> +    return UTF8Encoding().domName();
> 
> This may seem more elegant to you but I do not think this is as good as returning ASCIILiteral("UTF-8").

Oh, OK. I merely replicated the code from Document::characterSetWithUTF8Fallback() above. I'll update accordingly.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 272167 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272167&action=review
> 
> I agree with Alex that it would be better to have a little bit of test
> coverage, just for defaultCharset. However, my comment about other tests was
> wrong. I see now that there is nothing to test!

Sure, I'll add a test for defaultCharset in frameless documents to cover the slight behavior change then.

[Chris Dumez]

Created attachment 272249 [details]
Patch

[Chris Dumez]

Committed r197139: <http://trac.webkit.org/changeset/197139>