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

Issue 459613007: Fix for renderer visibility on Android that doesn't break Aura and Mac. (Closed)

Created:
6 years, 4 months ago by ppi
Modified:
6 years, 4 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Project:
chromium
Visibility:
Public.

Description

Fix for renderer visibility on Android that doesn't break Aura and Mac. http://crrev.com/287864 fixed an issue where a renderer that was killed in background and later reloaded was being respawned with incorrect process visibility, causing it to be killed on Android. The fix skipped resetting the |is_hidden_| flag of RenderWidgetHost, so that RenderWidgetHost crashing in background would retain its visibility status and trigger the ::Show() paths correctly when recreating the View in foreground, fixing the process visibility issues. Unfortunetely, this wasn't that good of an idea. It turns out that while it is the RenderWidgetHost that is the authoritative source of visibility truth that views rely on, it is the view that is responsible for providing signals (WasShown / WasHidden) to the widget host. That means that after a renderer crashes and the view is destroyed, RenderWidgetHost has no way to track its visibility anymore and all bets are off. It turns out that view implementations on Aura and Mac relied on the RenderWidgetHost visibility being reset upon crash (http://crbug.com/401859). Hence, this patch undoes the original "fix" and instead ensures that resetting the |is_hidden_| flag issues a notification for the RenderProcessHost, so that the process visibility signal that we attempted to fix with the original patch is still working. Regression test for http://crbug.com/399521 is in Chrome for Android repo (BindingManagerIntegrationTest). BUG=399521 BUG=401859 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289276

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -1 line) Patch
M content/browser/renderer_host/render_widget_host_impl.h View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
ppi
Sigh... Alex, ptal.
6 years, 4 months ago (2014-08-12 15:54:10 UTC) #1
aelias_OOO_until_Jul13
On 2014/08/12 15:54:10, ppi wrote: > Sigh... Alex, ptal. lgtm
6 years, 4 months ago (2014-08-12 17:52:30 UTC) #2
aelias_OOO_until_Jul13
On 2014/08/12 17:52:30, aelias wrote: > On 2014/08/12 15:54:10, ppi wrote: > > Sigh... Alex, ...
6 years, 4 months ago (2014-08-12 17:56:00 UTC) #3
ppi
Thanks! Yup, I will explore this asynchronously.
6 years, 4 months ago (2014-08-13 10:04:45 UTC) #4
ppi
The CQ bit was checked by ppi@chromium.org
6 years, 4 months ago (2014-08-13 10:05:20 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ppi@chromium.org/459613007/1
6 years, 4 months ago (2014-08-13 10:08:00 UTC) #6
commit-bot: I haz the power
6 years, 4 months ago (2014-08-13 13:01:45 UTC) #7
Message was sent while issue was closed.
Change committed as 289276

Powered by Google App Engine
This is Rietveld 408576698