[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





--- Comment #7 from Andy Estes <aestes at apple.com>  2011-03-14 12:29:05 PST ---
(In reply to comment #6)
> (From update of attachment 85647 [details])
> 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.

Perhaps interval is a better term both for the ctor argument and for my static constant, since that is the term used by TimerBase. I'm not sure how to rectify the float vs. int issue; perhaps including units in the variable names would help (e.g. intervalInMs and maxIntervalForUserGestureForwardingInMs)?

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

Will do.

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

Ok.

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

I was confused by the creation of the 'possibly' state, as the difference between PossiblyProcessingUserGesture and DefinitelyNotProcessingUserGesture is unclear to me. It seems like we should always know one way or the other.

PossiblyProcessingUserGesture is the default state for UserGestureIndicator so it's the state you'd get during a timer event prior to this patch when no gesture was present. That's why I chose it, and I don't see any behavior difference between it and DefinitelyNotProcessingUserGesture.

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

Ok, I'll stick with the "forwarded" terminology.

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

Are you saying I should rename all instances of "PASS" to "WINNING"?

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

I was unaware of leapForward(). I should be able to use that.

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