[webkit-reviews] review granted: [Bug 55104] Timer-based events should inherit the user gesture state of their originating event in certain cases. : [Attachment 85647] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 55104: Timer-based events should inherit the user gesture state of their
originating event in certain cases.
https://bugs.webkit.org/show_bug.cgi?id=55104

Attachment 85647: Patch
https://bugs.webkit.org/attachment.cgi?id=85647&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.