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

See Also:

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

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]
Comment 2 Dave Hyatt 2016-10-24 10:30:04 PDT
Created attachment 292624 [details]
Comment 3 Darin Adler 2016-10-24 10:40:23 PDT
Comment on attachment 292624 [details]

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
> -@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