[Webkit-unassigned] [Bug 37008] ScriptController::processingUserGestureEvent returns true for simulated clicks

[bugzilla-daemon at webkit.org]

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


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ggaren at apple.com




--- Comment #11 from Geoffrey Garen <ggaren at apple.com>  2010-04-02 10:12:40 PST ---
+namespace WTFNonHeapAllocatable {

You should include the same comment that Noncopyable has to explain why it gets
its own namespace.

+class NonHeapAllocatable {

What a mouthful! :)

I guess I don't have a better idea, though :(.

+        void* operator new(size_t)
+        {
+            ASSERT_NOT_REACHED();
+            return reinterpret_cast<void*>(0xbadbeef);
+        }
+
+        void* operator new[](size_t)
+        {
+            ASSERT_NOT_REACHED();
+            return reinterpret_cast<void*>(0xbadbeef);
+        }

It's better just to declare these functions, without defining them. That way,
if you somehow tricked a compiler into issuing a call to them, linking would
still fail. Also, it's more concise.

You might as well throw in the delete operators for good measure.

 bool Event::fromUserGesture()
 {
-    if (createdByDOM())
+    if (!UserGestureState::isProcessingUserGesture())

This will work, but it leaves the code in a bit of a confusing state. 

A good future cleanup would be for UserGestureState to keep a HashCountedSet of
frames that are processing user gestures. Then, instead of asking an event
whether it thought it was a user gesture, you could ask UserGestureState
whether a given frame was processing a user gesture. That would eliminate the
need for things like currentEvent, too.

+        static inline bool isProcessingUserGesture() { return
processingUserGesture; }

It's unfortunate when one thing has two names. Here, the accessor name gets the
word "is," but the variable name doesn't -- I assume to avoid name conflict.
One of the nice benefits of prefixing variable names is that it solves such
conflicts. I would recommend renaming processingUserGesture to s_
isProcessingUserGesture.

+    UserGestureState gestureIndicator;

This is another case of one thing having two names. (Is it a "user gesture" or
a "gesture?" Is it a "state" or an "indicator?")

I'd recommend picking one name and using it everywhere. I like "user gesture
indicator" or "user gesture record" best, since it's more specific about both
the gesture and the state. (Technically, "state" could be anything, including
"nothing's happening." So creating an empty "state" doesn't say as much.)

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