Bug 104268 - [chromium] expose UserGestureIndicator::Token via WebKit API so PPAPI plugins can correctly consume it
Summary: [chromium] expose UserGestureIndicator::Token via WebKit API so PPAPI plugins...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on: 111531
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-06 08:56 PST by jochen
Modified: 2013-03-07 00:14 PST (History)
8 users (show)

See Also:


Attachments
Patch (17.09 KB, patch)
2013-03-05 02:38 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (17.82 KB, patch)
2013-03-05 14:12 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (17.80 KB, patch)
2013-03-05 14:16 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (16.88 KB, patch)
2013-03-06 01:07 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (16.83 KB, patch)
2013-03-06 23:48 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-12-06 08:56:09 PST
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.
Comment 1 jochen 2013-03-05 02:38:42 PST
Created attachment 191442 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-05 02:40:24 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Darin Fisher (:fishd, Google) 2013-03-05 09:25:53 PST
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.
Comment 4 jochen 2013-03-05 14:12:57 PST
Created attachment 191561 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-05 14:14:43 PST
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.
Comment 6 jochen 2013-03-05 14:16:05 PST
Created attachment 191563 [details]
Patch
Comment 7 Darin Fisher (:fishd, Google) 2013-03-05 16:22:48 PST
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.
Comment 8 jochen 2013-03-06 01:07:12 PST
Created attachment 191680 [details]
Patch
Comment 9 jochen 2013-03-06 01:08:18 PST
This patch is on top of wkb.ug/111531 so it doesn't build yet on EWS
Comment 10 Darin Fisher (:fishd, Google) 2013-03-06 23:30:46 PST
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?
Comment 11 jochen 2013-03-06 23:48:08 PST
Created attachment 191933 [details]
Patch
Comment 12 jochen 2013-03-06 23:48:54 PST
(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
Comment 13 WebKit Review Bot 2013-03-07 00:14:35 PST
Comment on attachment 191933 [details]
Patch

Clearing flags on attachment: 191933

Committed r145046: <http://trac.webkit.org/changeset/145046>
Comment 14 WebKit Review Bot 2013-03-07 00:14:39 PST
All reviewed patches have been landed.  Closing bug.