Access to WebCore::RegularExpression is needed by the Chromium autofill code.
Created attachment 44194 [details] Implementation of WebRegularExpression
style-queue ran check-webkit-style on attachment 44194 [details] without any errors.
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.
The constructor and destructor also need to be exported.
(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.
Created attachment 44271 [details] try2: Implementation of WebRegularExpression
style-queue ran check-webkit-style on attachment 44271 [details] without any errors.
> > 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 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.
(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.
Created attachment 44330 [details] try3: Implementation of WebRegularExpression
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
Created attachment 44331 [details] try4: Implementation of WebRegularExpression Fixed header order oops.
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
Created attachment 44332 [details] try5: Implementation of WebRegularExpression Gah, actually modified the correct version of the file this time.
style-queue ran check-webkit-style on attachment 44332 [details] without any errors.
Comment on attachment 44332 [details] try5: Implementation of WebRegularExpression Clearing flags on attachment: 44332 Committed r51720: <http://trac.webkit.org/changeset/51720>
All reviewed patches have been landed. Closing bug.