Summary: | Track the right gesture state during the asynchronous form submission. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Johnny(Jianning) Ding <jnd> | ||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, cevans, commit-queue, enrica, eric, inferno, mihaip, scarybeasts, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Johnny(Jianning) Ding
2010-08-31 11:57:19 PDT
Created attachment 66081 [details]
test case
Yes. That's the right solution. We do something similar is a couple other cases where we need to track the user gesture state across async operations. That's actually why we created UserGestureIndicator in the first place. (In reply to comment #2) > Yes. That's the right solution. We do something similar is a couple other cases where we need to track the user gesture state across async operations. That's actually why we created UserGestureIndicator in the first place. Thanks for super fast response:) Will give a patch after getting up. @jnd, @abarth: we in fact had exactly this logic for exactly this case :) Adam and I added it a while ago in this CL: http://trac.webkit.org/changeset/57313/trunk/WebCore/loader/RedirectScheduler.cpp (plus follow-up fix): http://trac.webkit.org/changeset/59462/trunk/WebCore/loader/RedirectScheduler.cpp However, this subsequent CL seems to have backed part of our changes out: http://trac.webkit.org/changeset/65340/trunk/WebCore/loader/RedirectScheduler.cpp Adding Mihai - do you perhaps remember why the UserGestureIndicator part changed in 65340? (In reply to comment #4) > However, this subsequent CL seems to have backed part of our changes out: > http://trac.webkit.org/changeset/65340/trunk/WebCore/loader/RedirectScheduler.cpp > > Adding Mihai - do you perhaps remember why the UserGestureIndicator part changed in 65340? It looked like an unused local variable. Now I see that simply creating a UserGestureIndicator instance toggles a static member. I can put it back (perhaps with a comment). (In reply to comment #5) > It looked like an unused local variable. Now I see that simply creating a UserGestureIndicator instance toggles a static member. I can put it back (perhaps with a comment). It's also weird that fast/events/popup-blocked-to-post-blank.html still passes (based on http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fevents%2Fpopup-blocked-to-post-blank.html) even with my change. @mihaip: unfortunately, the Chromium and Safari pop-up blockers work in totally different ways. Both were broken and I fixed both; but the test only tests the Safari blocker. I don't believe it's possible to test the Chromium blocker with a LayoutTest. I take blame here for not adding a Chromium UI test. Johnny, how bribe-able are you to add such a thing? Pop-up blocking keeps regressing; a Chromium UI test that checks all of the test cases we've been dealing with recently would be invaluable. (In reply to comment #7) > @mihaip: unfortunately, the Chromium and Safari pop-up blockers work in totally different ways. Both were broken and I fixed both; but the test only tests the Safari blocker. I don't believe it's possible to test the Chromium blocker with a LayoutTest. > I take blame here for not adding a Chromium UI test. > Johnny, how bribe-able are you to add such a thing? Pop-up blocking keeps regressing; a Chromium UI test that checks all of the test cases we've been dealing with recently would be invaluable. Hi Chris, Yes, I am sorry I didn't find your original patch. and sure thing, I will add a chrome UI test to address this bug, also I will bring your fix back into WebKit with comments. Thanks! Created attachment 66211 [details] fix patch Hi Adam, Chris, This patch is to bring the Chris's original fix (in r57313 and r594620) back. Since WebKit and Chromium implement popup blocker in different ways, the test fast/events/popup-blocked-to-post-blank.html can not cover this bug in chromium. I will add a browser_test to address this bug in chromium once this patch is landed and taken by chromium trunk. Thanks! Comment on attachment 66211 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=66211&action=prettypatch Mostly just questions. > WebCore/ChangeLog:9 > + fast/events/popup-blocked-to-post-blank.html can cover the test in WebKit. > + A UI test will be added in chromium to address chromium's bug. Is fast/events/popup-blocked-to-post-blank.html test currently failing? If so, do we need to remove it from the Skipped list? If not, then it doesn't seem to cover this change. > WebCore/loader/RedirectScheduler.cpp:188 > + // Please do not delete the following local variable gestureIndicator definition. > + // The variable gestureIndicator toggles a static gesture state to let WebKit correctly > + // track the user gesture state across async operations. This comment is not needed. Instead, we should have a test that breaks if you remove this line. > WebCore/loader/RedirectScheduler.cpp:324 > bool lockBackForwardList = mustLockBackForwardList(m_frame, UserGestureIndicator::processingUserGesture()) || (submission->state()->formSubmissionTrigger() == SubmittedByJavaScript && m_frame->tree()->parent()); > > - schedule(adoptPtr(new ScheduledFormSubmission(submission, lockBackForwardList, duringLoad))); > + schedule(adoptPtr(new ScheduledFormSubmission(submission, lockBackForwardList, duringLoad, m_frame->loader()->isProcessingUserGesture()))); Why do we use m_frame->loader()->isProcessingUserGesture() here but UserGestureIndicator::processingUserGesture() two lines above? (In reply to comment #10) Thanks, Adam! Please see my inline comments. > Is fast/events/popup-blocked-to-post-blank.html test currently failing? If so, do we need to remove it from the Skipped list? If not, then it doesn't seem to cover this change. At now fast/events/popup-blocked-to-post-blank.html is not failed in WebKit layout test, and I think it does cover this issue, which is to check whether the form submission issued popup window can bypass popup blocker or not. AFAIK this test is not in any skip lists, we can still keep it. (Please correct me if I was wrong). But since Chromium's popup feature is implemented in another way (judgement in embedder), this test can not cover the same issue in Chromium. I will add a Chromium test once this patch is landed and taken by Chromium. Does that make sense? > This comment is not needed. Instead, we should have a test that breaks if you remove this line. Yes, you are right! The reason to add this comment is because the line looks like useless if someone is not familiar with the context. I want to remind him/her. However removing it does not break WebKit, but does break Chromium. so a chromium test case to address this issue is definitely necessary. Like I said in above. I will add that test in chromium. Of course, I will drop this comment in new patch. > Why do we use m_frame->loader()->isProcessingUserGesture() here but UserGestureIndicator::processingUserGesture() two lines above? The reason to use m_frame->loader()->isProcessingUserGesture() is because that call can cover all scenarios of user gesture, but UserGestureIndicator::processingUserGesture() does not. FrameLoader::isProcessingUserGesture() can handle right gesture state in the scenario of form submission when javascript is disabled, can handle right gesture state in the scenario of form submission via href="javascript:form.submit()", can handle right gesture state in the scenario of form submission inside gesture event or non-gesture event (See Event::fromUserGesture). Why the code in two lines above used serGestureIndicator::processingUserGesture(). To be honest, I don't know, but I 'd like to change it to use FrameLoader::isProcessingUserGesture(). Any comments? Created attachment 66365 [details]
new patch
Comment on attachment 66365 [details]
new patch
We need a test for this patch. Honestly, the test is more important than the fix in cases like this. Having a test in Chromium-land is nice, but we should have a LayoutTest too, otherwise we're only going to discover regressions after a merge, which is bad news bears. This is common code to all WebKit ports. We should be able to test it with a LayoutTest.
@abarth - let us know if you have a way to actually test this in WebKit. The Chromium port actually runs with WebKit's notification of a pop-up blocker _disabled_. Chromium instead relies on WebKit's UserGestureIndicator API returning the correct results at various times, such as in the asynchronous form submission context. (The original patch for this did add a LayoutTest for this with WebKit's pop-up blocker enabled). Comment on attachment 66365 [details]
new patch
I see. Ok, we'll this is likely to cause us more pain in the future...
Thanks, Adam. Johnny - a good Chromium test in the area of pop-up blocking would go some way to lessening any future pain... (In reply to comment #16) > Thanks, Adam. > Johnny - a good Chromium test in the area of pop-up blocking would go some way to lessening any future pain... Thanks Adam, Chris! The chromium test will be added after landing this patch. I will send the cl to you tomorrow.(ooo today) Added commit queue flag so that this gets committed. Comment on attachment 66365 [details] new patch Clearing flags on attachment: 66365 Committed r66742: <http://trac.webkit.org/changeset/66742> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/66742 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/66742 http://trac.webkit.org/changeset/66743 |