Please refer to http://code.google.com/p/chromium/issues/detail?id=47276 for the bug details. After investigation, this bug was because chromium thought the popup was not user initiated. In function: bool ScriptController::processingUserGesture(DOMWrapperWorld*) const (WebCore/binding/v8/ScriptController.cpp) Chromium tests whether UserGestureIndicator::processingUserGesture() returns true before checking the current event object. If not, it returns false to indicate the disposition was not user initiated. However, in JSC (WebCore/binding/js/ScriptController.cp), it tests the current event object at first, if the event can be treated as user gesture, JSC will treat the disposition as user initiated. For why UserGestureIndicator::processingUserGesture() returns false (both JSC and V8) when current event is from select element (widget), it's because now we set the UserGestureIndicator as DefinitelyUserGesture only in the event handlers written in event_handler.cpp which are called by webview. For the event handlers which are called by widget (derived from FrameScrollView), we didn't change UserGestureIndicator to DefinitelyUserGesture. I think we probably need to fix this behavior. I will provide a patch to make sure the ScriptController::processingUserGesture follows the JSC's behavior.
Created attachment 59509 [details] test case
Created attachment 59520 [details] patch v1
Comment on attachment 59520 [details] patch v1 The JSC implementation of this function is 10 lines long. Why is the V8 version 50 lines? Please update the V8 version to match the JSC version.
The main reason for this issue is in chromium the select was implemented by a widget (derived from FramelessScrollView), all input events are delivered to widget's event handlers which don't set UserGestureIndicator to DefinitelyUserGesture. But in safari, all input events are delivered by webview and handled by event handlers in WebCore/page/EventHandler.cpp) in which UserGestureIndicator is set to DefinitelyUserGesture. I also changed the V8 version ScriptController::processingUserGesture to match the JSC version as much as possible.
Created attachment 59650 [details] patch v2
Comment on attachment 59650 [details] patch v2 Much better. Thanks.
Since it is not actual v8 binding bug, I removed the [V8 binding] from the summary
Hi Adam, Sorry to bother you again, I think I didn't exactly make the V8 version ScriptController::processingUserGesture to match the JSC version since I missed one thing in patch v2, In JSC version ScriptController::processingUserGesture, if there is no available event, then use the returned value of UserGestureIndicator::processingUserGesture() to indicate the user gesture status. In V8, we should match this. Please see my patch v3. Sorry again for the inconvenience.
Created attachment 59755 [details] patch v3
Comment on attachment 59650 [details] patch v2 Rejecting patch 59650 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: e Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19223 test cases. fast/workers/storage/execute-sql-args-worker.html -> crashed Exiting early after 1 failures. 15317 tests run. 633.17s total testing time 15316 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3341799
Comment on attachment 59755 [details] patch v3 ok
Comment on attachment 59755 [details] patch v3 Clearing flags on attachment: 59755 Committed r61941: <http://trac.webkit.org/changeset/61941>
http://trac.webkit.org/changeset/61941 might have broken GTK Linux 64-bit Release
Strange. Does GTK use V8? Oh, I bet they just don't implemented the needed layoutTestController methods. --- /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/fast/events/popup-when-select-change-expected.txt 2010-06-25 22:51:01.000000000 -0700 +++ /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/fast/events/popup-when-select-change-actual.txt 2010-06-25 22:51:00.000000000 -0700 @@ -1,4 +1,5 @@ -ALERT: PASSED +CONSOLE MESSAGE: line 19: TypeError: Result of expression 'eventSender.keyDown' [null] is not an object. +FAIL: Timed out waiting for notifyDone to be called If the pop-up was not blocked then there will be an PASS message. Otherwise, the test fails.
Adam, Seems like there are some limitations when writing event related testcases on Linux. I changed the test case to make it work on both Mac & Linux by referring to other testcases. Please review the patch V4. Thanks!
Created attachment 59891 [details] patch V4 to make the test work on both Mac & Linux.
Comment on attachment 59891 [details] patch V4 to make the test work on both Mac & Linux. Huh. Interesting. I wonder why Linux behaves different... Maybe there's some problem with event sender?
Hi Adam For error: 'eventSender.keyDown' [null] is not an object, I think it's a problem of event sender on linux. But have no time to dig in now, will take a look later. Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch? Thanks!
> For error: 'eventSender.keyDown' [null] is not an object, I think it's a problem of event sender on linux. But have no time to dig in now, will take a look later. Yes. That function of layoutTestController isn't implemented on Gtk, which means they can't run a ton of tests.
> Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch? Is the test on the appropriate Skip lists? If so, we can land it using commit-queue. However, if it's going to make some builders red, we can't land it.
(In reply to comment #20) > > Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch? > > Is the test on the appropriate Skip lists? If so, we can land it using commit-queue. However, if it's going to make some builders red, we can't land it. Yes, this test is in appropriate Skip lists, they are LayoutTests/platform/chromium/test_expectations.txt (Chromium:Linux) and LayoutTests/platform/gtk/Skipped. Since it is only affected Linux version, I have tested this test case on my local Linux build of both Chrome and WebKit, they are all green. So I think it's safe to land the patch 4.
Comment on attachment 59891 [details] patch V4 to make the test work on both Mac & Linux. okiedokes
Comment on attachment 59891 [details] patch V4 to make the test work on both Mac & Linux. Rejecting patch 59891 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: iling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19232 test cases. fast/dom/HTMLScriptElement/script-set-src.html -> failed Exiting early after 1 failures. 6663 tests run. 147.91s total testing time 6662 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3356037
(In reply to comment #23) > (From update of attachment 59891 [details]) > Rejecting patch 59891 from commit-queue. > > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 > Last 500 characters of output: > iling Java tests > make: Nothing to be done for `default'. > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found > Testing 19232 test cases. > fast/dom/HTMLScriptElement/script-set-src.html -> failed > > Exiting early after 1 failures. 6663 tests run. > 147.91s total testing time > > 6662 test cases (99%) succeeded > 1 test case (<1%) had incorrect layout > 1 test case (<1%) had stderr output > > Full output: http://webkit-commit-queue.appspot.com/results/3356037 Interesting, since the patch V4 only affect a test file (fast/events/popup-when-select-change.html). I am sure that it doesn't affect script-set-src.html. Anything should I do?
Comment on attachment 59891 [details] patch V4 to make the test work on both Mac & Linux. Possible it was a flaky test.
Comment on attachment 59891 [details] patch V4 to make the test work on both Mac & Linux. Clearing flags on attachment: 59891 Committed r62088: <http://trac.webkit.org/changeset/62088>
(In reply to comment #25) > (From update of attachment 59891 [details]) > Possible it was a flaky test. Thanks Eric!
Make this bug as fixed