Bug 104268 – [chromium] expose UserGestureIndicator::Token via WebKit API so PPAPI plugins can correctly consume it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

In https://code.google.com/codesearch#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppapi_plugin_instance.cc&exact_package=chromium&q=PluginInstance::IsProcessingUserGesture&l=1747 the user gesture state is remembered for 10s and allows a PPAPI plugin to execute javascript with a user gesture indicator in that timeframe. It should use a token instead, so we only get one user gesture gated action per user gesture.

Created attachment 191442 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 191442 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191442&action=review

> Source/WebKit/chromium/public/WebFrame.h:416
> +    virtual WebUserGestureToken* currentUserGestureToken() const = 0;

methods that return objects which need to be destroyed by the caller are usually
prefixed with "create".  That might make this method sound odd.

> Source/WebKit/chromium/public/WebUserGestureToken.h:59
> +    WebPrivatePtr<WebCore::UserGestureToken> m_token;

Since you are using WebPrivatePtr here, it seems like you should arrange for
embedders of WebKit to pass around WebUserGestureToken by value.  Treat it like
a smart pointer (like RefPtr) just as we do with WebNode and many other similar
wrapper types.  This way the "currentUserGestureToken" method can just return
a WebUserGestureToken by value.

Created attachment 191561 [details]
Patch

[WebKit Review Bot]

Attachment 191561 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/WebKit.gyp', u'Source/WebKit/chromium/WebKit.gypi', u'Source/WebKit/chromium/public/WebFrame.h', u'Source/WebKit/chromium/public/WebScopedUserGesture.h', u'Source/WebKit/chromium/public/WebUserGestureToken.h', u'Source/WebKit/chromium/src/WebFrameImpl.cpp', u'Source/WebKit/chromium/src/WebFrameImpl.h', u'Source/WebKit/chromium/src/WebScopedUserGesture.cpp', u'Source/WebKit/chromium/src/WebUserGestureToken.cpp', u'Source/WebKit/chromium/tests/WebUserGestureTokenTest.cpp']" exit_code: 1
Source/WebKit/chromium/public/WebUserGestureToken.h:56:  WEBKIT_EXPORT should not be used on a function with a body.  [readability/webkit_export] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 191563 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 191563 [details]
Patch

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.

Created attachment 191680 [details]
Patch

This patch is on top of wkb.ug/111531 so it doesn't build yet on EWS

[Darin Fisher (:fishd, Google)]

Comment on attachment 191680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191680&action=review

> Source/WebKit/chromium/public/WebScopedUserGesture.h:52
> +// Instead, obtain the current WebUserGestureToken from the WebFrame, and use

nit: This comment seems out-of-date.

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

nit: The static_cast is really needed?

Created attachment 191933 [details]
Patch

(In reply to comment #10)
> (From update of attachment 191680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191680&action=review
> 
> > Source/WebKit/chromium/public/WebScopedUserGesture.h:52
> > +// Instead, obtain the current WebUserGestureToken from the WebFrame, and use
> 
> nit: This comment seems out-of-date.
> 

updated

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

I made the operator return a PassRefPtr, then it works without the cast

[WebKit Review Bot]

Comment on attachment 191933 [details]
Patch

Clearing flags on attachment: 191933

Committed r145046: <http://trac.webkit.org/changeset/145046>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.