Bug 97483 – It's possible to clone consumable user gestures

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Since DOMTimer copies the user gesture state (but does not track whether or not the user gesture is consumed), you can effectively fork it like this:

<script>
function foo() {
 setTimeout(function() { var c = window.open("about:blank"); c.close(); }, 0);
 window.open("data:text/html,pop-under", "", "width=300,height=300");
}
</script>
<button onclick="foo()">Click Me</button>

A port supporting user gesture consumption should only create one window. Since the timeout is created before the first window.open call, however, two windows are created.

Created attachment 166662 [details]
Patch

[Build Bot]

Comment on attachment 166662 [details]
Patch

Attachment 166662 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14114676

New failing tests:
platform/mac/editing/deleting/backward-delete.html
fast/events/popup-blocking-timers.html
svg/custom/use-instanceRoot-event-bubbling.xhtml

[WebKit Review Bot]

Comment on attachment 166662 [details]
Patch

Attachment 166662 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14138159

New failing tests:
fast/events/popup-blocking-timers.html
svg/custom/use-instanceRoot-event-bubbling.xhtml

Created attachment 166685 [details]
Patch

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

[Adam Barth]

Comment on attachment 166685 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        is suppoed to forward the user gesture, take a reference to this token

suppoed -> supposed

> Source/WebCore/dom/UserGestureIndicator.cpp:37
> +        return adoptRef(static_cast<UserGestureIndicator::Token*>(new GestureToken));

Is this static_cast needed?  I thought PassRefPtr was smart enough to know how to do the type conversion for you.

> Source/WebCore/dom/UserGestureIndicator.cpp:42
> +    void inc() { m_consumableGestures++; }

inc -> increment?  Please use complete words in variable names.  Maybe addGesture() is a better name?

> Source/WebCore/dom/UserGestureIndicator.cpp:43
> +    bool consume()

consume -> consumeGesture?

> Source/WebCore/dom/UserGestureIndicator.cpp:50
> +    bool isValid() const { return m_consumableGestures > 0; }

isValid -> hasGestures() ?

[Adam Barth]

Comment on attachment 166685 [details]
Patch

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

> Source/WebCore/dom/UserGestureIndicator.h:64
> +    ProcessingUserGestureState m_state;
> +    Token* m_previousToken;
> +    RefPtr<Token> m_token;

It seems strange to need to add all this information to the stack.

[Adam Barth]

Comment on attachment 166685 [details]
Patch

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

> Source/WebCore/page/DOMTimer.h:73
> -        bool m_shouldForwardUserGesture;
> +        RefPtr<UserGestureIndicator::Token> m_userGestureToken;

This part makes sense.  DOMTimer wants to forward the user gesture only if it wasn't consumed.

(In reply to comment #7)
> (From update of attachment 166685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166685&action=review
> 
> > Source/WebCore/dom/UserGestureIndicator.h:64
> > +    ProcessingUserGestureState m_state;
> > +    Token* m_previousToken;
> > +    RefPtr<Token> m_token;
> 
> It seems strange to need to add all this information to the stack.

hum, indeed m_state is not required.

The other two, however, are required.

[Adam Barth]

This design seems more complicated than needed.

What question is DOMTimer really trying to answer?  It wants to know whether all the user gestures for its call stack where consumed before that stack unwound.  That means we only need one counter per call stack, not  stack of counters, right?

Created attachment 166738 [details]
Patch

Created attachment 166739 [details]
Patch

I believe I addressed all your comments

Created attachment 166747 [details]
Patch

[Adam Barth]

Comment on attachment 166747 [details]
Patch

Looks great.  Thanks.

[WebKit Review Bot]

Comment on attachment 166747 [details]
Patch

Clearing flags on attachment: 166747

Committed r130267: <http://trac.webkit.org/changeset/130267>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.