Summary: | [chromium] expose UserGestureIndicator::Token via WebKit API so PPAPI plugins can correctly consume it | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||||||
Component: | WebKit Misc. | Assignee: | jochen | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, cdn, dglazkov, fishd, jamesr, jochen, tkent+wkapi, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 111531 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
jochen
2012-12-06 08:56:09 PST
Created attachment 191442 [details]
Patch
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 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
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
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 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 Comment on attachment 191933 [details] Patch Clearing flags on attachment: 191933 Committed r145046: <http://trac.webkit.org/changeset/145046> All reviewed patches have been landed. Closing bug. |