RESOLVED FIXED 32095
Chromium WebKit API needs a wrapper for WebCore::RegularExpression
https://bugs.webkit.org/show_bug.cgi?id=32095
Summary Chromium WebKit API needs a wrapper for WebCore::RegularExpression
James Hawkins
Reported 2009-12-02 16:37:30 PST
Access to WebCore::RegularExpression is needed by the Chromium autofill code.
Attachments
Implementation of WebRegularExpression (7.06 KB, patch)
2009-12-02 16:40 PST, James Hawkins
fishd: review-
try2: Implementation of WebRegularExpression (8.69 KB, patch)
2009-12-03 15:19 PST, James Hawkins
fishd: review-
try3: Implementation of WebRegularExpression (10.87 KB, patch)
2009-12-04 13:34 PST, James Hawkins
no flags
try4: Implementation of WebRegularExpression (10.87 KB, patch)
2009-12-04 13:42 PST, James Hawkins
no flags
try5: Implementation of WebRegularExpression (10.87 KB, patch)
2009-12-04 13:43 PST, James Hawkins
no flags
James Hawkins
Comment 1 2009-12-02 16:40:55 PST
Created attachment 44194 [details] Implementation of WebRegularExpression
WebKit Review Bot
Comment 2 2009-12-02 21:13:22 PST
style-queue ran check-webkit-style on attachment 44194 [details] without any errors.
Darin Fisher (:fishd, Google)
Comment 3 2009-12-02 22:24:11 PST
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)
Comment 4 2009-12-02 22:25:10 PST
The constructor and destructor also need to be exported.
James Hawkins
Comment 5 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.
James Hawkins
Comment 6 2009-12-03 15:19:54 PST
Created attachment 44271 [details] try2: Implementation of WebRegularExpression
WebKit Review Bot
Comment 7 2009-12-03 15:25:09 PST
style-queue ran check-webkit-style on attachment 44271 [details] without any errors.
Darin Fisher (:fishd, Google)
Comment 8 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.
Darin Fisher (:fishd, Google)
Comment 9 2009-12-03 23:59:08 PST
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
Comment 10 2009-12-04 13:34:23 PST
(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
Comment 11 2009-12-04 13:34:56 PST
Created attachment 44330 [details] try3: Implementation of WebRegularExpression
WebKit Review Bot
Comment 12 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
James Hawkins
Comment 13 2009-12-04 13:42:03 PST
Created attachment 44331 [details] try4: Implementation of WebRegularExpression Fixed header order oops.
WebKit Review Bot
Comment 14 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
James Hawkins
Comment 15 2009-12-04 13:43:55 PST
Created attachment 44332 [details] try5: Implementation of WebRegularExpression Gah, actually modified the correct version of the file this time.
WebKit Review Bot
Comment 16 2009-12-04 13:49:20 PST
style-queue ran check-webkit-style on attachment 44332 [details] without any errors.
WebKit Commit Bot
Comment 17 2009-12-04 16:24:35 PST
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
Comment 18 2009-12-04 16:24:40 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.