[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





--- Comment #5 from James Hawkins <jhawkins at google.com>  2009-12-03 15:19:12 PST ---
(In reply to comment #3)
> (From update of attachment 44194 [details])
> > 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.
> 

Done.

> 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.
> 

Done.

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

Done.

> 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.)
> 

Done.  I added the WebTextCaseSensitivity enum definition to WebString.h
instead of creating a new file because this matches the definition of
TextCaseSensitivity in StringImpl.h.  I can make a new file if you think that's
a better option.

> 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.
> 

Done.

> 
> > 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.
> 

Done.

> 
> > +    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.
> 

Done.

> 
> > +    WebCore::RegularExpression* re = static_cast<WebCore::RegularExpression*>(m_private);
> 
> This static_cast is unnecessary since m_private "is a"
> WebCore::RegularExpression.

Done.

> The constructor and destructor also need to be exported.

Done.

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