[webkit-reviews] review denied: [Bug 32095] Chromium WebKit API needs a wrapper for WebCore::RegularExpression : [Attachment 44194] Implementation of WebRegularExpression

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Hawkins
<jhawkins at google.com>'s request for review:
Bug 32095: Chromium WebKit API needs a wrapper for WebCore::RegularExpression
https://bugs.webkit.org/show_bug.cgi?id=32095

Attachment 44194: Implementation of WebRegularExpression
https://bugs.webkit.org/attachment.cgi?id=44194&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/public/WebRegularExpression.h
...
> +class WebRegularExpression {
> +public:
> +    WebRegularExpression(const WebString& pattern, WebStringCaseSensitivity
caseSensitivity);
> +    virtual ~WebRegularExpression();
> +
> +    int match(const WebString& str, int startFrom = 0, int* matchLength = 0)
const;
> +
> +private:
> +    WebRegularExpressionPrivate* m_private;
> +};

If an instance of this class is copied, it'll lead to a crash (double free).
Make this class extend from WebNonCopyable.

No need to make the destructor virtual since there are no other virtual
methods on this class.	Adding a virtual destructor when unnecessary
just adds the bloat of an implicit vtable pointer for no benefit.

The match method should be prefixed with WEBKIT_API to indicate that it
is an exported function.

You haven't included the definition of WebStringCaseSensitivity in this
patch.	I think it'd be better to call it WebTextCaseSensitivity.  (We
have enums like WebTextDirection and WebTextAffinity already, so the
WebText prefix is common already.)

nit: It is webkit style to not name parameters when the parameter name
is not informative, so you should drop the "str" and the "caseSensitivity"
parameters in the header.


> Index: WebKit/chromium/src/WebRegularExpression.cpp

Please add a 'using namespace WebCore;' line at the top so that you can
drop all of the WebCore:: prefixes.


> +    WebCore::TextCaseSensitivity sensitivity =
> +	   static_cast<WebCore::TextCaseSensitivity>(caseSensitivity);

To ensure that this static_cast doesn't become invalid, it is important to
update AssertMatchingEnums.cpp.


> +    WebCore::RegularExpression* re =
static_cast<WebCore::RegularExpression*>(m_private);

This static_cast is unnecessary since m_private "is a"
WebCore::RegularExpression.