Bug 154662 - Drop [TreatReturnedNullStringAs=Undefined] WebKit-specific IDL attribute
Summary: Drop [TreatReturnedNullStringAs=Undefined] WebKit-specific IDL attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 154659
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-24 17:12 PST by Chris Dumez
Modified: 2022-02-27 23:09 PST (History)
8 users (show)

See Also:


Attachments
Patch (7.32 KB, patch)
2016-02-24 19:05 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.82 KB, patch)
2016-02-25 15:17 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>