Bug 77690 – Add UserGestureIndicator capability to Chromium API.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

Add UserGestureIndicator capability to Chromium API.

[Greg Billock]

Created attachment 125208 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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?

[Greg Billock]

(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.

[Greg Billock]

Created attachment 125368 [details]
Patch

[Darin Fisher (:fishd, Google)]

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...

[Bill Budge]

*** Bug 69190 has been marked as a duplicate of this bug. ***

[Bill Budge]

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).

[Greg Billock]

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?

[Greg Billock]

Created attachment 125690 [details]
Patch

[Darin Fisher (:fishd, Google)]

Oh, I see.  I misread WebPrivateOwnPtr as WebPrivatePtr.  Sorry for the confusion!

[Greg Billock]

Created attachment 126117 [details]
Patch

[Greg Billock]

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.

[Greg Billock]

Created attachment 126817 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[Greg Billock]

Created attachment 127266 [details]
Patch

[Greg Billock]

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.

[Darin Fisher (:fishd, Google)]

(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?

[Greg Billock]

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?

[Greg Billock]

Created attachment 127412 [details]
Patch

[Greg Billock]

OK, I think this is ready.

[WebKit Review Bot]

Comment on attachment 127412 [details]
Patch

Clearing flags on attachment: 127412

Committed r108665: <http://trac.webkit.org/changeset/108665>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.