[webkit-reviews] review denied: [Bug 63618] Parsing CSS3 font-feature-settings property : [Attachment 99603] Patch V1

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 63618: Parsing CSS3 font-feature-settings property
https://bugs.webkit.org/show_bug.cgi?id=63618

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

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=99603&action=review

> LayoutTests/ChangeLog:12
> +	   * css3/script-tests/TEMPLATE.html: Added.

Script test in separete js file is obsolete. Please put it inside HTML.

> Source/WebCore/css/CSSParser.cpp:6021
> +    while (value) {

Is it possible to rewrite this with for statement? If it's possible, it's
desirable
because this loops is for list traversal.

This loop looks too complicated anyway...
I guess we can make this clear by extracting a per-item function.
What do you think?

> Source/WebCore/css/CSSParser.cpp:6026
> +	   if (value->string.length != 4)

having a named constant would be preferable.

> Source/WebCore/css/CSSParser.cpp:6051
> +	   settings->append(parsedValue.release());

You don't need release(). PassRefPtr does its job for you.

> Source/WebCore/css/CSSParser.cpp:6054
> +    if (settings && settings->length()) {

Can setting be null?

> Source/WebCore/css/CSSStyleSelector.cpp:5317
> +	       fontDescription.setFeatureSettings(0);

How about to define FontDescription::makeNormal() ? 
Then we can write something like
m_style->setFontDescription(m_style->fontDescription().makeNormal())

> Source/WebCore/css/FontFeatureValue.cpp:55
> +    String result = "'" + m_tag + "'";

How about to try StringBuilder? It would be more effective in this case.

> Source/WebCore/platform/graphics/FontDescription.h:78
> +	   , m_featureSettings(0)

We don't need this. RefPtr automatically gets zero-cleared.

> Source/WebCore/platform/graphics/FontDescription.h:160
> +    RefPtr<FontFeatureSettings> m_featureSettings; // The list of OpenType
font feature settings.

I think we don't need this comment. The class name is clear enough.

> Source/WebCore/platform/graphics/FontDescription.h:200
> +	   && m_featureSettings.get() == other.m_featureSettings.get();

It looks you are comparing pointers. Is this intentional?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:30
> +#include <wtf/RefPtr.h>

PassRefPtr.h?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:39
> +    FontFeature(const FontFeature& other);

Do we need the explicit copy constructor?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:40
> +    FontFeature& operator=(const FontFeature&);

...and an assignment operator?

> Source/WebCore/platform/graphics/FontFeatureSettings.h:56
> +    void appendFeature(const FontFeature& feature) { m_list.append(feature);
}

append() looks enough. (we have at() anyway.)

> Source/WebCore/platform/graphics/FontFeatureSettings.h:59
> +    const FontFeature& at(size_t index) const { return m_list[index]; }

let's use m_list.at() for similarity.