[Webkit-unassigned] [Bug 55104] Timer-based events should inherit the user gesture state of their originating event in certain cases.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55104


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #85647|review?                     |review+
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2011-03-14 09:46:04 PST ---
(From update of attachment 85647)
View in context: https://bugs.webkit.org/attachment.cgi?id=85647&action=review

> Source/WebCore/page/DOMTimer.cpp:42
> +static const int maxTimeoutForUserGestureForwarding = 1000; // One second matches Gecko.

The word “timeout” is used multiple ways in this file that are confusing. For example, the number of milliseconds is called a timeout in the name of the argument to the constructor and the data member m_originalTimeout. But the timers themselves are also referred to as timeouts, as in the data member m_timeoutId.

Because of that ambiguity, and because we also have two other constants here that are floating point numbers with time in milliseconds, I think this constant is slightly confusing.

> Source/WebCore/page/DOMTimer.cpp:64
> +    m_indicateUserGestureWhenFired = UserGestureIndicator::processingUserGesture()
> +        && timeout <= maxTimeoutForUserGestureForwarding
> +        && m_nestingLevel == 1;

I’d like to see all the initialization of data members be done with data member initialization instead of assignment. That could easily be done by making a separate function to return the ID and putting this logic in a function too. They can be inline functions if desired. I wouldn’t insist on that change for this patch, but it would be nice.

I think this expression needs a why comment for the nesting level check. The other two parts of this expression explain themselves, given the names of what’s involved, but the reason the nesting level should have an effect is more subtle.

> Source/WebCore/page/DOMTimer.cpp:111
> +    UserGestureIndicator gestureIndicator(m_indicateUserGestureWhenFired ? DefinitelyProcessingUserGesture : PossiblyProcessingUserGesture);

Is it helpful to set the gesture indication to “possibly” when the boolean is false? Does the test somehow cover that?

> Source/WebCore/page/DOMTimer.h:73
> +        bool m_indicateUserGestureWhenFired;

For a boolean data member, we normally do not want to use something that can be read as a verb phrase. Perhaps add the word “should”.

Another possible name is m_representsForwardedUserGesture, which is consistent with the name of the maxTimeoutForUserGestureForwarding constant.

> LayoutTests/fast/events/popup-blocking-timers.html:5
> +        var win;

I’m as big a fan of winning as the next guy, but I don’t think this is a great variable name.

> LayoutTests/fast/events/popup-blocking-timers.html:12
> +            layoutTestController.waitUntilDone();

Can we make this test run fast by using the leapForward feature or something else like that? It’s unfortunate that the test takes a second to run.

> LayoutTests/fast/events/popup-blocking-timers.html:54
> +                }
> +                else {

Brace style.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.