Bug 32095 - Chromium WebKit API needs a wrapper for WebCore::RegularExpression
: Chromium WebKit API needs a wrapper for WebCore::RegularExpression
Status: RESOLVED FIXED
: WebKit
WebKit API
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-12-02 16:37 PST by
Modified: 2009-12-04 16:24 PST (History)


Attachments
Implementation of WebRegularExpression (7.06 KB, patch)
2009-12-02 16:40 PST, James Hawkins
fishd: review-
Review Patch | Details | Formatted Diff | Diff
try2: Implementation of WebRegularExpression (8.69 KB, patch)
2009-12-03 15:19 PST, James Hawkins
fishd: review-
Review Patch | Details | Formatted Diff | Diff
try3: Implementation of WebRegularExpression (10.87 KB, patch)
2009-12-04 13:34 PST, James Hawkins
no flags Review Patch | Details | Formatted Diff | Diff
try4: Implementation of WebRegularExpression (10.87 KB, patch)
2009-12-04 13:42 PST, James Hawkins
no flags Review Patch | Details | Formatted Diff | Diff
try5: Implementation of WebRegularExpression (10.87 KB, patch)
2009-12-04 13:43 PST, James Hawkins
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-12-02 16:37:30 PST
Access to WebCore::RegularExpression is needed by the Chromium autofill code.
------- Comment #1 From 2009-12-02 16:40:55 PST -------
Created an attachment (id=44194) [details]
Implementation of WebRegularExpression
------- Comment #2 From 2009-12-02 21:13:22 PST -------
style-queue ran check-webkit-style on attachment 44194 [details] without any errors.
------- Comment #3 From 2009-12-02 22:24:11 PST -------
(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.

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.
------- Comment #4 From 2009-12-02 22:25:10 PST -------
The constructor and destructor also need to be exported.
------- Comment #5 From 2009-12-03 15:19:12 PST -------
(In reply to comment #3)
> (From update of attachment 44194 [details] [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.
------- Comment #6 From 2009-12-03 15:19:54 PST -------
Created an attachment (id=44271) [details]
try2: Implementation of WebRegularExpression
------- Comment #7 From 2009-12-03 15:25:09 PST -------
style-queue ran check-webkit-style on attachment 44271 [details] without any errors.
------- Comment #8 From 2009-12-03 23:55:00 PST -------
> > 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.
------- Comment #9 From 2009-12-03 23:59:08 PST -------
(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.


> 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.
------- Comment #10 From 2009-12-04 13:34:23 PST -------
(In reply to comment #9)
> (From update of attachment 44271 [details] [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.
------- Comment #11 From 2009-12-04 13:34:56 PST -------
Created an attachment (id=44330) [details]
try3: Implementation of WebRegularExpression
------- Comment #12 From 2009-12-04 13:38:09 PST -------
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
------- Comment #13 From 2009-12-04 13:42:03 PST -------
Created an attachment (id=44331) [details]
try4: Implementation of WebRegularExpression

Fixed header order oops.
------- Comment #14 From 2009-12-04 13:43:45 PST -------
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
------- Comment #15 From 2009-12-04 13:43:55 PST -------
Created an attachment (id=44332) [details]
try5: Implementation of WebRegularExpression

Gah, actually modified the correct version of the file this time.
------- Comment #16 From 2009-12-04 13:49:20 PST -------
style-queue ran check-webkit-style on attachment 44332 [details] without any errors.
------- Comment #17 From 2009-12-04 16:24:35 PST -------
(From update of attachment 44332 [details])
Clearing flags on attachment: 44332

Committed r51720: <http://trac.webkit.org/changeset/51720>
------- Comment #18 From 2009-12-04 16:24:40 PST -------
All reviewed patches have been landed.  Closing bug.