[Webkit-unassigned] [Bug 32095] Chromium WebKit API needs a wrapper for WebCore::RegularExpression

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32095


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44194|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2009-12-02 22:24:11 PST ---
(From update of attachment 44194)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.