[webkit-reviews] review granted: [Bug 55025] Refactor HTMLEquivalent into a hierachy of classes : [Attachment 83457] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 55025: Refactor HTMLEquivalent into a hierachy of classes
https://bugs.webkit.org/show_bug.cgi?id=55025

Attachment 83457: Patch
https://bugs.webkit.org/attachment.cgi?id=83457&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.