When any event is ongoing, we don't set user gesture correctly...

When a function handling an event doesn't return a value, it leaves
the "event" object on the global context. This namespace pollution
was causing some functionality that was trying to detect if we were
in an event, such as popup blocking, to go wrong.

BUG=6367
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8050

[Elliot Glaysher, 2009-01-13 20:30:14]

(I don't know this section of the code. Please really double check my solution.
I may just be treating the symptoms...)

[dglazkov, 2009-01-13 21:40:07]

It might be good to have a second pair of eyes on this, so I am adding Darin.

http://codereview.chromium.org/18013/diff/1/2
File webkit/port/bindings/v8/ScriptController.cpp (right):

http://codereview.chromium.org/18013/diff/1/2#newcode214
Line 214: }
This is completely different from the way the JSC bindings do it, but this will
work, I think.

I could be wrong, but can't we move this check all the way up to right after
initializing active_proxy?

[Elliot Glaysher, 2009-01-13 23:10:26]

http://codereview.chromium.org/18013/diff/1/2
File webkit/port/bindings/v8/ScriptController.cpp (right):

http://codereview.chromium.org/18013/diff/1/2#newcode214
Line 214: }
On 2009/01/13 21:40:08, Dimitri Glazkov wrote:
> This is completely different from the way the JSC bindings do it, but this
will
> work, I think.
> 
> I could be wrong, but can't we move this check all the way up to right after
> initializing active_proxy?

Done.

[dglazkov, 2009-01-14 17:36:57]

Feng, can you take a look?

[Feng Qian, 2009-01-14 17:43:38]

Looks good to me, a layout test?

On 2009/01/14 17:36:57, Dimitri Glazkov wrote:
> Feng, can you take a look?

[dglazkov, 2009-01-14 17:46:39]

Damn. I think this ain't gonna fly. All we're doing is making sure it's an
inline event not under a timer.

Unfortunately, inline event could also mean an "onload" handler for say, a page
or an image.

In other words, this is not the right fix. Apologies.

We should see how WebKit does it and make sure they don't make the same mistake.
Their mechanics are different in naming (they use windowShell->currentEvent or
somesuch), but similar in nature.

[Elliot Glaysher, 2009-01-14 21:24:28]

> We should see how WebKit does it and make sure they don't make the same
mistake.
> Their mechanics are different in naming (they use windowShell->currentEvent or
> somesuch), but similar in nature.

Looking at
http://trac.webkit.org/browser/trunk/WebCore/bindings/js/ScriptController.cpp...
, our current implementation is functionally identical to  theirs. I've tried
the test case that's in the bug in Safari 3.1 and there doesn't seem to be a
problem. 

I'm wondering if we use a superset of events that normal webkit does. What is a
"load" event?

[Elliot Glaysher, 2009-01-14 23:11:05]

I have completely redone the patch to address the root problem. Please take
another look.

[Mike Belshe, 2009-01-14 23:36:00]

LGTM

http://codereview.chromium.org/18013/diff/206/10
File webkit/port/bindings/v8/v8_events.cpp (right):

http://codereview.chromium.org/18013/diff/206/10#newcode87
Line 87: v8::Local<v8::String> event_symbol = v8::String::NewSymbol("event");
Let's add a comment here:

For compatibility, we store the event object as a property on the window called
"event".  Because this is the global namespace, we save away any existing
"event" property, and then restore it after executing the javascript handler.

http://codereview.chromium.org/18013/diff/206/10#newcode110
Line 110: // Restore the old event.
Update comment to:

Restore the old event.  This must be done for all exit paths through this
method.