WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
try2: Implementation of WebRegularExpression
(8.69 KB, patch)
2009-12-03 15:19 PST
,
James Hawkins
fishd
: review-
Details
Formatted Diff
Diff
try3: Implementation of WebRegularExpression
(10.87 KB, patch)
2009-12-04 13:34 PST
,
James Hawkins
no flags
Details
Formatted Diff
Diff
try4: Implementation of WebRegularExpression
(10.87 KB, patch)
2009-12-04 13:42 PST
,
James Hawkins
no flags
Details
Formatted Diff
Diff
try5: Implementation of WebRegularExpression
(10.87 KB, patch)
2009-12-04 13:43 PST
,
James Hawkins
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug