Bug 32095 – Chromium WebKit API needs a wrapper for WebCore::RegularExpression

• Component: WebKit API
• Status: RESOLVED FIXED

[James Hawkins]

Access to WebCore::RegularExpression is needed by the Chromium autofill code.

[James Hawkins]

Created attachment 44194 [details]
Implementation of WebRegularExpression

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44194 [details] without any errors.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44194 [details]
Implementation of WebRegularExpression

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

[Darin Fisher (:fishd, Google)]

The constructor and destructor also need to be exported.

[James Hawkins]

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

[James Hawkins]

Created attachment 44271 [details]
try2: Implementation of WebRegularExpression

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44271 [details] without any errors.

[Darin Fisher (:fishd, Google)]

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

Sorry, I should have made that clear.  Every top-level type in the WebKit API should have its own header file.  You should see that that rule is applied universally throughout the API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44271 [details]
try2: Implementation of WebRegularExpression

> Index: WebKit/chromium/public/WebRegularExpression.h
...
> +    WebRegularExpression(const WebString& pattern, WebTextCaseSensitivity);
> +    ~WebRegularExpression();

^^^ These need WEBKIT_API too.

Any public function that is not inline needs to be prefixed with WEBKIT_API.


> Index: WebKit/chromium/src/AssertMatchingEnums.cpp
...
> +COMPILE_ASSERT_MATCHING_ENUM(WebTextCaseSensitive, TextCaseSensitive);
> +COMPILE_ASSERT_MATCHING_ENUM(WebTextCaseInsensitive, TextCaseInsensitive);
> +
>  COMPILE_ASSERT_MATCHING_ENUM(WebTextAffinityUpstream, UPSTREAM);
>  COMPILE_ASSERT_MATCHING_ENUM(WebTextAffinityDownstream, DOWNSTREAM);

nit: please insert the new enum block alphabetically (after WebTextAffinity*)
to help keep this file somewhat organized.


> Index: WebKit/chromium/src/WebRegularExpression.cpp
...
> +WebRegularExpression::~WebRegularExpression()
> +{
> +    delete static_cast<WebRegularExpressionPrivate*>(m_private);
> +}

^^^ that static_cast is unnecessary since m_private is already a
pointer to WebRegularExpressionPrivate.

[James Hawkins]

(In reply to comment #9)
> (From update of attachment 44271 [details])
> > Index: WebKit/chromium/public/WebRegularExpression.h
> ...
> > +    WebRegularExpression(const WebString& pattern, WebTextCaseSensitivity);
> > +    ~WebRegularExpression();
> 
> ^^^ These need WEBKIT_API too.
> 
> Any public function that is not inline needs to be prefixed with WEBKIT_API.
> 

Done.

> 
> > Index: WebKit/chromium/src/AssertMatchingEnums.cpp
> ...
> > +COMPILE_ASSERT_MATCHING_ENUM(WebTextCaseSensitive, TextCaseSensitive);
> > +COMPILE_ASSERT_MATCHING_ENUM(WebTextCaseInsensitive, TextCaseInsensitive);
> > +
> >  COMPILE_ASSERT_MATCHING_ENUM(WebTextAffinityUpstream, UPSTREAM);
> >  COMPILE_ASSERT_MATCHING_ENUM(WebTextAffinityDownstream, DOWNSTREAM);
> 
> nit: please insert the new enum block alphabetically (after WebTextAffinity*)
> to help keep this file somewhat organized.
> 

Done.

> 
> > Index: WebKit/chromium/src/WebRegularExpression.cpp
> ...
> > +WebRegularExpression::~WebRegularExpression()
> > +{
> > +    delete static_cast<WebRegularExpressionPrivate*>(m_private);
> > +}
> 
> ^^^ that static_cast is unnecessary since m_private is already a
> pointer to WebRegularExpressionPrivate.

Done.

[James Hawkins]

Created attachment 44330 [details]
try3: Implementation of WebRegularExpression

[WebKit Review Bot]

Attachment 44330 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebRegularExpression.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebKit/chromium/src/WebRegularExpression.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2

[James Hawkins]

Created attachment 44331 [details]
try4: Implementation of WebRegularExpression

Fixed header order oops.

[WebKit Review Bot]

Attachment 44331 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebRegularExpression.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebKit/chromium/src/WebRegularExpression.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2

[James Hawkins]

Created attachment 44332 [details]
try5: Implementation of WebRegularExpression

Gah, actually modified the correct version of the file this time.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44332 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44332 [details]
try5: Implementation of WebRegularExpression

Clearing flags on attachment: 44332

Committed r51720: <http://trac.webkit.org/changeset/51720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.