[webkit-reviews] review denied: [Bug 32095] Chromium WebKit API needs a wrapper for WebCore::RegularExpression : [Attachment 44271] try2: Implementation of WebRegularExpression

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Hawkins
<jhawkins at google.com>'s request for review:
Bug 32095: Chromium WebKit API needs a wrapper for WebCore::RegularExpression
https://bugs.webkit.org/show_bug.cgi?id=32095

Attachment 44271: try2: Implementation of WebRegularExpression
https://bugs.webkit.org/attachment.cgi?id=44271&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.