Bug 163891

Summary: Remove CSSCharsetRule from the CSS OM
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: CSSAssignee: Dave Hyatt <hyatt>
Severity: Normal CC: achristensen, bfulgham, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch darin: review+

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