As a part of EditingStyle deployment effort, much of logic in ApplyStyleCommand::removeImplicitlyStyledElement needs to be extracted into EditingStyle. In order to do this, we need to first refactor removeImplicitlyStyledElement and HTMLEquivalents table.
Created attachment 83457 [details] Patch
Comment on attachment 83457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83457&action=review > Source/WebCore/ChangeLog:24 > + (WebCore::HTMLEquivalent::matches): Returns true if the element is an equivalent, meaning that > + the element's implicit style affects the property of this equivalence. > + (WebCore::HTMLEquivalent::hasAttribute): Returns true if this equivalence requires attribute; > + e.g. color, size, dir. > + (WebCore::HTMLEquivalent::propertyExistsInStyle): Returns true if the specified style has > + the property of this equivalence; for example, if the current equivalence is about size attribute, > + then it'll return true if the specified style has font-size property set. > + (WebCore::HTMLEquivalent::valueIsPresentInStyle): Returns true if the specified style has > + the implicit style of the specified element dictated by this equivalence. > + (WebCore::HTMLEquivalent::addToStyle): Adds the implicit style of the element dictated by this > + equivalence to the specified mutable style. I'm sorry these descriptions don't make sense. I'll try to revise them when my brain is functioning. > Source/WebCore/editing/ApplyStyleCommand.cpp:1300 > + static const HTMLEquivalent* HTMLEquivalents[] = { > + adoptPtr(new HTMLEquivalent(CSSPropertyFontWeight, CSSValueBold, bTag)).leakPtr(), > + adoptPtr(new HTMLEquivalent(CSSPropertyFontWeight, CSSValueBold, strongTag)).leakPtr(), > + adoptPtr(new HTMLEquivalent(CSSPropertyVerticalAlign, CSSValueSub, subTag)).leakPtr(), > + adoptPtr(new HTMLEquivalent(CSSPropertyVerticalAlign, CSSValueSuper, supTag)).leakPtr(), > + adoptPtr(new HTMLEquivalent(CSSPropertyFontStyle, CSSValueItalic, iTag)).leakPtr(), > + adoptPtr(new HTMLEquivalent(CSSPropertyFontStyle, CSSValueItalic, emTag)).leakPtr(), I don't know the proper way to construct this table :( Any suggestions?
Comment on attachment 83457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83457&action=review >> Source/WebCore/editing/ApplyStyleCommand.cpp:1300 >> + adoptPtr(new HTMLEquivalent(CSSPropertyFontStyle, CSSValueItalic, emTag)).leakPtr(), > > I don't know the proper way to construct this table :( Any suggestions? With an init() method and a bunch of addEquivalent() calls, would be the way I'd go. You've already moved it to the heap this way, might as well make the code look clean.
(In reply to comment #3) > (From update of attachment 83457 [details]) > With an init() method and a bunch of addEquivalent() calls, would be the way I'd go. You've already moved it to the heap this way, might as well make the code look clean. Would HTMLEquivalents be a vector/list of OwnPtr<HTMLEquivalent> in that case?
Comment on attachment 83457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83457&action=review > Source/WebCore/editing/ApplyStyleCommand.cpp:1170 > +class HTMLEquivalent { > +public: > + HTMLEquivalent(CSSPropertyID id) I think you’re slightly overdoing it with putting function definitions inside the class definition. Separate inline functions can make the class definition easier to read, and so makes it easier to understand the purpose of the class for people who come to the code later. > Source/WebCore/editing/ApplyStyleCommand.cpp:1206 > + const QualifiedName* m_tagName; I think you need a comment about why it’s OK to use a pointer to something reference-counted here. I presume that’s because these are all global immortal tag names. > Source/WebCore/editing/ApplyStyleCommand.cpp:1254 > + RefPtr<CSSValue> value = attributeValueAsCSSValue(element); > + if (value) > + style->setProperty(m_propertyID, value->cssText()); Either an early return or putting the definition inside the if would be good here. > Source/WebCore/editing/ApplyStyleCommand.cpp:1270 > + const QualifiedName& m_attrName; Same comment as above. You need to state why it’s OK to have a reference here. >>> Source/WebCore/editing/ApplyStyleCommand.cpp:1300 >>> + adoptPtr(new HTMLEquivalent(CSSPropertyFontStyle, CSSValueItalic, emTag)).leakPtr(), >> >> I don't know the proper way to construct this table :( Any suggestions? > > With an init() method and a bunch of addEquivalent() calls, would be the way I'd go. You've already moved it to the heap this way, might as well make the code look clean. I think this is already fine. I’d probably suggest create functions instead of putting the adoptPtr/new here, even if the create functions aren’t used much.
> (From update of attachment 83457 [details]) > I'm sorry these descriptions don't make sense. I'll try to revise them when my brain is functioning. I tried and failed: (WebCore::HTMLEquivalent::create): Added. (WebCore::HTMLEquivalent::~HTMLEquivalent): Added. (WebCore::HTMLEquivalent::matches): Returns true if the element is an equivalent, meaning that the element's implicit style affects the property of this equivalence. (WebCore::HTMLEquivalent::hasAttribute): Returns true if this equivalence requires attributes; e.g. color, size, dir. (WebCore::HTMLEquivalent::propertyExistsInStyle): Returns true if the property of this equivalence exists in the specified style. e.g. if this equivalence is for size attribute and font-size property, this function returns true if the specified style has font-size property set. (WebCore::HTMLEquivalent::HTMLEquivalent): Added. (WebCore::HTMLEquivalent::valueIsPresentInStyle): Returns true if the specified style has the implicit style of the specified element of this equivalence. (WebCore::HTMLEquivalent::addToStyle): Adds the implicit style of the element of this equivalence to the specified mutable style.(In reply to comment #2) These functions are incredibly hard to understand & explain conceptually. I'll land as stated above because the code itself is self-explanatory in my opinion.
Committed r79521: <http://trac.webkit.org/changeset/79521>