Bug 163891 - Remove CSSCharsetRule from the CSS OM
Summary: Remove CSSCharsetRule from the CSS OM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-24 08:15 PDT by Dave Hyatt
Modified: 2016-10-24 14:05 PDT (History)
3 users (show)

See Also:

Attachments
Patch (25.79 KB, patch)
2016-10-24 09:00 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch (48.34 KB, patch)
2016-10-24 10:30 PDT, Dave Hyatt 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2016-10-24 08:15:01 PDT 
Remove the CSSCharsetRule from the CSS OM. All other browser engines have removed it.
Comment 1 Dave Hyatt 2016-10-24 09:00:13 PDT 
Created attachment 292615 [details]
Patch
Comment 2 Dave Hyatt 2016-10-24 10:30:04 PDT 
Created attachment 292624 [details]
Patch
Comment 3 Darin Adler 2016-10-24 10:40:23 PDT 
Comment on attachment 292624 [details]
Patch

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

Makes sense to remove it if websites don’t depend on it.

> Source/WebCore/css/CSSStyleSheet.cpp:276
>      for (unsigned i = 0; i < ruleCount; ++i) {
>          CSSRule* rule = item(i);
> -        if (rule->type() == CSSRule::CHARSET_RULE)
> -            continue;
> -        nonCharsetRules->rules().append(rule);
> +        ruleList->rules().append(rule);
>      }

Would be nice to get rid of the local variable too.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:201
>      for (unsigned i = 0, size = styleSheet->length(); i < size; ++i) {
>          CSSRule* item = styleSheet->item(i);
> -        if (item->type() == CSSRule::CHARSET_RULE)
> -            continue;
>          listRules.append(item);
>      }

Would be nice to get rid of the local variable too.

> Source/WebKit/mac/DOM/DOMCSS.mm:-69
> -        case DOM_CHARSET_RULE:
> -            return [DOMCSSCharsetRule class];

Why is it important to change this? Does it do any harm to leave this concept behind in our Objective-C code?

> Source/WebKit/mac/DOM/DOMCSSCharsetRule.h:-33
> -WEBKIT_CLASS_AVAILABLE_MAC(10_4)
> -@interface DOMCSSCharsetRule : DOMCSSRule
> -@property (readonly, copy) NSString *encoding;
> -@end

I’m not sure we should remove this without even deprecating it first. Not our usual approach to API. If someone references this class for whatever reason, removing it will make their app crash. We can probably do something different and then the app won’t crash. Unless we have some reason to believe nobody has made the mistake of trying to do something with this class.

> Source/WebKit/mac/DOM/DOMCSSCharsetRule.mm:-46
> -- (NSString *)encoding
> -{
> -    WebCore::JSMainThreadNullState state;
> -    return IMPL->encoding();
> -}

Could have this return nil, or the empty string, or always return "UTF-8" or whatever we think is best for compatibility. I guess because someone can never create one of these, we don’t really need to implement the method at all, so that makes me lean toward the "return nil" option.
Comment 4 Dave Hyatt 2016-10-24 10:42:25 PDT 
Ok, yeah I guess it's safest to just leave it in the API.
Comment 5 Dave Hyatt 2016-10-24 11:21:23 PDT 
Landed in r207767.
Comment 6 Darin Adler 2016-10-24 12:35:11 PDT 
Looks like this broke the Windows build.

c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\derivedsources\webcore\JSCSSCharsetRule.h(23): fatal error C1083: Cannot open include file: 'CSSCharsetRule.h': No such file or directory [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCoreDerivedSources.vcxproj]

Not sure why DerivedSources.cpp is still trying to include JSCSSCharsetRule.h.
Comment 7 Alex Christensen 2016-10-24 13:22:32 PDT 
https://trac.webkit.org/changeset/207772