Bug 154662

Summary: Drop [TreatReturnedNullStringAs=Undefined] WebKit-specific IDL attribute
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, darin, esprehn+autocc, ggaren, kangil.han, kondapallykalyan, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 154659    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch cdumez: commit-queue?

Description Chris Dumez 2016-02-24 17:12:54 PST
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.
Comment 1 Chris Dumez 2016-02-24 19:05:00 PST
Created attachment 272167 [details]
Patch
Comment 2 Alex Christensen 2016-02-24 22:33:55 PST
Comment on attachment 272167 [details]
Patch

I think a patch like this ought to add some tests.
Comment 3 Chris Dumez 2016-02-24 22:45:24 PST
(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.
Comment 4 Darin Adler 2016-02-25 09:06:46 PST
(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.
Comment 5 Darin Adler 2016-02-25 09:09:05 PST
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").
Comment 6 Chris Dumez 2016-02-25 09:13:11 PST
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.
Comment 7 Chris Dumez 2016-02-25 09:14:34 PST
(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.
Comment 8 Chris Dumez 2016-02-25 15:17:03 PST
Created attachment 272249 [details]
Patch
Comment 9 Chris Dumez 2016-02-25 15:24:09 PST
Committed r197139: <http://trac.webkit.org/changeset/197139>