[webkit-reviews] review denied: [Bug 77690] Add UserGestureIndicator capability to Chromium API. : [Attachment 126817] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 15 14:00:02 PST 2012
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 77690: Add UserGestureIndicator capability to Chromium API.
https://bugs.webkit.org/show_bug.cgi?id=77690
Attachment 126817: Patch
https://bugs.webkit.org/attachment.cgi?id=126817&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126817&action=review
> Source/WebKit/chromium/public/WebScopedUserGesture.h:44
> +// WebCore user gesture indicator. To use, create one, perform whatever
actions
no need to mention WebCore in the comments.
> Source/WebKit/chromium/public/WebScopedUserGesture.h:47
> +class WebScopedUserGesture : public WebNonCopyable {
nit: no need to extend from WebNonCopyable since you have a member variable
of type WebPrivateOwnPtr. that already makes this class non-copyable.
> Source/WebKit/chromium/public/WebScopedUserGesture.h:50
> + // DefinitelyProcessingUserGesture.
DefinitelyProcessingUserGesture is a WebCore thing. it is meaningless to
someone
who only reads the WebKit API. please avoid mentioning WebCore things since
your
comments can easily get stale, and we want consumers to be insulated from
WebCore
details.
> Source/WebKit/chromium/public/WebScopedUserGesture.h:54
> + WEBKIT_EXPORT static bool isProcessingUserGesture();
do you need to expose this method?
> Source/WebKit/chromium/public/WebScopedUserGesture.h:56
> + WEBKIT_EXPORT virtual ~WebScopedUserGesture();
nit: no need for a virtual destructor.
nit: we have the convention of implement constructors and destructors inline
in terms of other exported functions. here, i'd recommend the following:
class WebScopedUserGesture {
public:
WebScopedUserGesture() { initialize(); }
~WebScopedUserGesture() { reset(); }
private:
WEBKIT_EXPORT void initialize();
WEBKIT_EXPORT void reset();
WebPrivateOwnPtr<WebCore::UserGestureIndicator> m_indicator;
};
> Source/WebKit/chromium/src/WebFrameImpl.cpp:1116
> + return WebScopedUserGesture::isProcessingUserGesture();
we don't normally implement WebKit APIs in terms of other WebKit APIs. we
generally just go directly to WebCore.
More information about the webkit-reviews
mailing list