Bug 163891 – Remove CSSCharsetRule from the CSS OM

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dave Hyatt]

Remove the CSSCharsetRule from the CSS OM. All other browser engines have removed it.

[Dave Hyatt]

Created attachment 292615 [details]
Patch

[Dave Hyatt]

Created attachment 292624 [details]
Patch

[Darin Adler]

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.

[Dave Hyatt]

Ok, yeah I guess it's safest to just leave it in the API.

[Dave Hyatt]

Landed in r207767.

[Darin Adler]

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.

[Alex Christensen]

https://trac.webkit.org/changeset/207772

[Alex Christensen]

https://trac.webkit.org/changeset/207776
https://trac.webkit.org/changeset/207777