Add UserGestureIndicator capability to Chromium API.
Created attachment 125208 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 125208 [details] Patch Can you say more about what you need? We already have WebFrame::isProcessingUserGesture(). Usually, web platform features that depend on user gesture checking perform their user gesture checking within WebKit. That won't work for WebIntents?
(In reply to comment #3) > (From update of attachment 125208 [details]) > Can you say more about what you need? We already have WebFrame::isProcessingUserGesture(). Usually, web platform features that depend on user gesture checking perform their user gesture checking within WebKit. That won't work for WebIntents? This is for setting the user gesture indicator, not for reading it. The need we have for this is that when executing a script in an extension context menu context, we are in a user-caused gesture situation, but the indicator doesn't realize that (because it hasn't been told). There are probably other situations where we execute code in extensions that is user-caused, but which we don't set the indicator to reflect that.
Created attachment 125368 [details] Patch
Comment on attachment 125368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125368&action=review > Source/WebKit/chromium/public/WebUserGestureIndicator.h:45 > +class WebUserGestureIndicator { I think you should do a more traditional WebKit API wrapper for a reference counted type. That way, users can either lay one of these down on the stack to get a scoped gesture, or they can heap allocate a copy to extend its lifetime. Do you really need all of these variants exposed to Chrome? I also recall that bbudge was needing to simulate a user-gesture for something. Hmm...
*** Bug 69190 has been marked as a duplicate of this bug. ***
We will need this for plugins that need to respond to a user gesture and are running on an asynchronous out-of-process proxy (Chrome IPC when we switch).
Comment on attachment 125368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125368&action=review >> Source/WebKit/chromium/public/WebUserGestureIndicator.h:45 >> +class WebUserGestureIndicator { > > I think you should do a more traditional WebKit API wrapper for a reference counted type. > That way, users can either lay one of these down on the stack to get a scoped gesture, > or they can heap allocate a copy to extend its lifetime. > > Do you really need all of these variants exposed to Chrome? > > I also recall that bbudge was needing to simulate a user-gesture for something. Hmm... You can do that with scoped_ptr<WUGI>(WUGI::xxx()); The underlying UserGestureIndicator isn't ref-counted, though. I can make this class look like it is, though, but it'd mean doing internal shenanigans to correctly maintain ownership of the internal pointer, or simply disallowing refcount-style object sharing. I'm not sure how much scope expansion will be useful; I don't need it for my immediate use. Bill, do you need to pass these objects between scopes?
Created attachment 125690 [details] Patch
Oh, I see. I misread WebPrivateOwnPtr as WebPrivatePtr. Sorry for the confusion!
Created attachment 126117 [details] Patch
As discussed, I added the state accessor to WebUserGestureIndicator. I also re-pointed the WebFrame method to use it. Follow-ups will repoint users of that API here, and then remove it.
Created attachment 126817 [details] Patch
Comment on attachment 126817 [details] Patch 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.
Created attachment 127266 [details] Patch
Comment on attachment 126817 [details] Patch 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. Done >> 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. Done >> 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. Done >> Source/WebKit/chromium/public/WebScopedUserGesture.h:54 >> + WEBKIT_EXPORT static bool isProcessingUserGesture(); > > do you need to expose this method? Our plan was to move WebFrameImpl callers to call this. Shall we abort that plan? >> 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; > }; Done. >> 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. Here our plan was to move all callers of WebFrameImpl::isProcessingUserGesture to calling WebScopedUserGestuer::isProcessingUserGesture. I made this change so that refactor will be a no-op.
(In reply to comment #17) > >> Source/WebKit/chromium/public/WebScopedUserGesture.h:54 > >> + WEBKIT_EXPORT static bool isProcessingUserGesture(); > > > > do you need to expose this method? > > Our plan was to move WebFrameImpl callers to call this. Shall we abort that plan? I'm sorry, I recall discussing that too. I think we probably should abort since the name of this class is WebScopedUserGesture. I assume you don't really need isProcessingUserGesture, right?
OK (In reply to comment #18) > (In reply to comment #17) > > >> Source/WebKit/chromium/public/WebScopedUserGesture.h:54 > > >> + WEBKIT_EXPORT static bool isProcessingUserGesture(); > > > > > > do you need to expose this method? > > > > Our plan was to move WebFrameImpl callers to call this. Shall we abort that plan? > > I'm sorry, I recall discussing that too. I think we probably should abort > since the name of this class is WebScopedUserGesture. I assume you don't > really need isProcessingUserGesture, right?
Created attachment 127412 [details] Patch
OK, I think this is ready.
Comment on attachment 127412 [details] Patch Clearing flags on attachment: 127412 Committed r108665: <http://trac.webkit.org/changeset/108665>
All reviewed patches have been landed. Closing bug.