[webkit-reviews] review denied: [Bug 104268] [chromium] expose UserGestureIndicator::Token via WebKit API so PPAPI plugins can correctly consume it : [Attachment 191563] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied
jochen at chromium.org's request for review:
Bug 104268: [chromium] expose UserGestureIndicator::Token via WebKit API so
PPAPI plugins can correctly consume it
https://bugs.webkit.org/show_bug.cgi?id=104268

Attachment 191563: Patch
https://bugs.webkit.org/attachment.cgi?id=191563&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191563&action=review


> Source/WebKit/chromium/public/WebFrame.h:415
> +    // The caller takes ownership of the WebUserGestureToken returned.

nit: No need for this last sentence now.

> Source/WebKit/chromium/public/WebScopedUserGesture.h:64
> +    WEBKIT_EXPORT void initialize(const WebUserGestureToken&);

nit: I suggest giving this method a more descriptive name like
"initializeWithToken"

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1123
> +    return WebUserGestureToken(UserGestureIndicator::currentToken());

nit: I didn't notice this before, but it appears as though there is no need to
have a WebFrame here.
The |this| pointer isn't relevant.  That suggests that you could just have a
static method on
WebUserGestureToken, like so:

  class WebUserGestureToken {
  public:
      WEBKIT_EXPORT static WebUserGestureToken current();
      ...
  };

> Source/WebKit/chromium/src/WebScopedUserGesture.cpp:47
> +	   m_indicator.reset(new
WebCore::UserGestureIndicator(static_cast<WebCore::UserGestureToken*>(token)));


nit: I'm surprised the static_cast was needed given that WebUserGestureToken
has a conversion operator.

> Source/WebKit/chromium/tests/WebUserGestureTokenTest.cpp:62
> +	   token = frame->currentUserGestureToken();

nit: It is kind of unfortunate that this test needs a WebFrame.  There is no
real
reason for WebFrame to have the isProcessingUserGesture method.  It is really
just
a global property.