Summary: | Remove CSSCharsetRule from the CSS OM | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||
Component: | CSS | Assignee: | Dave Hyatt <hyatt> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, bfulgham, darin | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Dave Hyatt
2016-10-24 08:15:01 PDT
Created attachment 292615 [details]
Patch
Created attachment 292624 [details]
Patch
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. Ok, yeah I guess it's safest to just leave it in the API. 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. |