Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(419)

Issue 159426: Reverting 21683. (Closed)

Created:
11 years, 5 months ago by brettw
Modified:
9 years, 7 months ago
Reviewers:
robertshield
CC:
chromium-reviews_googlegroups.com, darin (slow to review), brettw, jam, Ben Goodger (Google)
Visibility:
Public.

Description

Re-landing r21673 without re-enabling the BrowserTest, which apparently is still failing. Make downloads not prevent tabs from closing. If a download creates a cross-site transition (for example, if you click a link in Gmail that results in a download in a new tab), that tab will be stuck and you can't close it or the browser. This is the opposite problem with a similar cause as bug 16246. In both cases we were using some secondary signal to tell us if we're closing for a cross site transition or closing the tab, and that signal was wrong. In this case, we were running the onunload handler, but because there was a pending RenderViewHost, the RenderManager would think that the close was a cross-site one, and not forward the close message to actually close the tab. This patch adds a flag to the on unload handlers that indicates whether it's for a tab closure or a cross-site transition, so we can do the right thing unambiguously when the message returns. In this case I keep this information in the RenderView in case we send multiple close requests, we'll close the tab if any of them were for the entire tab, even if that particular one was dropped because we don't want to have more than one in flight at once. BUG=17560 TEST=none. Review URL: http://codereview.chromium.org/160122 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21685

Patch Set 1 #

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -38 lines) Patch
M chrome/browser/browser.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host.h View 2 chunks +14 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/render_view_host.cc View 3 chunks +20 lines, -5 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host_delegate.h View 1 chunk +5 lines, -3 lines 0 comments Download
M chrome/browser/tab_contents/render_view_host_manager.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/tab_contents/render_view_host_manager.cc View 2 chunks +20 lines, -21 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/tab_contents/tab_contents.cc View 1 chunk +0 lines, -4 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
brettw
11 years, 5 months ago (2009-07-27 20:06:50 UTC) #1
robertshield
11 years, 5 months ago (2009-07-27 20:16:36 UTC) #2
On 2009/07/27 20:06:50, brettw wrote:
> 

lgtm

Powered by Google App Engine
This is Rietveld 408576698