Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308623003:
Assume crashed RenderWidgetHost to be invisible (Closed)
|
Created:
5 years, 4 months ago by Jaekyun Seok (inactive) Modified:
5 years, 3 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAssume crashed RenderWidgetHost to be invisible
On Android, we assume dead RenderWidgetHost to be invisible to correctly
keep the priority of a restored renderer.
Instead, we set the correct visibility when it is restored.
BUG=522795
Committed: https://crrev.com/a163192a8629025a3dd21b9684354b244ce354a2
Cr-Commit-Position: refs/heads/master@{#347883}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Assume dead RenderWidgetHost to be invisible #
Total comments: 4
Patch Set 5 : Do not assume the visibility of dead RenderWidgetHost #Patch Set 6 : Revive patch set 4 #
Total comments: 4
Patch Set 7 : Apply the change to all platforms #
Total comments: 10
Patch Set 8 : Apply Daniel's comments #
Total comments: 2
Patch Set 9 : Check dest_render_frame_host->render_view_host()->is_hidden() #
Messages
Total messages: 46 (8 generated)
jaekyun@chromium.org changed reviewers: + sievers@chromium.org
sievers@, please review this CL.
On 2015/08/21 04:01:31, Jaekyun Seok wrote: > sievers@, please review this CL. Ping?
this is a good find, let's discuss in the bug. i think it's not necesarily android specific, plus we don't want to break android+aura with ifdefs here either.
PTAL. I modified codes to assume dead RenderWidgetHost to be invisible on Android.
On 2015/08/31 06:15:07, Jaekyun Seok wrote: > PTAL. > > I modified codes to assume dead RenderWidgetHost to be invisible on Android. Ping?
https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:430: #if defined(OS_ANDROID) Can we remove the ifdef Android? There shouldn't be a difference in behavior. And 'defined(AURA) && defined(OS_ANDROID)' needs to be working also. You can try on AURA Linux and see if that works for crashed tabs. In other words I was wondering if the fix could just be to 'remove the visibility workaround for when the renderer crashes, and instead set the visibility correctly, according to what the app thinks the visibility should be, when we reload the crashed tab'. https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1262: #if defined(OS_ANDROID) Can we just remove 1260-1272?
PTAL. https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:430: #if defined(OS_ANDROID) On 2015/09/01 18:30:54, sievers wrote: > Can we remove the ifdef Android? > There shouldn't be a difference in behavior. And 'defined(AURA) && > defined(OS_ANDROID)' needs to be working also. > > You can try on AURA Linux and see if that works for crashed tabs. > > In other words I was wondering if the fix could just be to 'remove the > visibility workaround for when the renderer crashes, and instead set the > visibility correctly, according to what the app thinks the visibility should be, > when we reload the crashed tab'. Removed. I tested on AURA Linux manually, and confirmed that it worked. https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1262: #if defined(OS_ANDROID) On 2015/09/01 18:30:54, sievers wrote: > Can we just remove 1260-1272? Done.
On 2015/09/02 01:25:50, Jaekyun Seok wrote: > PTAL. > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > content/browser/frame_host/render_frame_host_manager.cc:430: #if > defined(OS_ANDROID) > On 2015/09/01 18:30:54, sievers wrote: > > Can we remove the ifdef Android? > > There shouldn't be a difference in behavior. And 'defined(AURA) && > > defined(OS_ANDROID)' needs to be working also. > > > > You can try on AURA Linux and see if that works for crashed tabs. > > > > In other words I was wondering if the fix could just be to 'remove the > > visibility workaround for when the renderer crashes, and instead set the > > visibility correctly, according to what the app thinks the visibility should > be, > > when we reload the crashed tab'. > > Removed. > > I tested on AURA Linux manually, and confirmed that it worked. > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.cc:1262: #if > defined(OS_ANDROID) > On 2015/09/01 18:30:54, sievers wrote: > > Can we just remove 1260-1272? > > Done. Ping?
On 2015/09/03 22:04:00, Jaekyun Seok wrote: > On 2015/09/02 01:25:50, Jaekyun Seok wrote: > > PTAL. > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > > File content/browser/frame_host/render_frame_host_manager.cc (right): > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > > content/browser/frame_host/render_frame_host_manager.cc:430: #if > > defined(OS_ANDROID) > > On 2015/09/01 18:30:54, sievers wrote: > > > Can we remove the ifdef Android? > > > There shouldn't be a difference in behavior. And 'defined(AURA) && > > > defined(OS_ANDROID)' needs to be working also. > > > > > > You can try on AURA Linux and see if that works for crashed tabs. > > > > > > In other words I was wondering if the fix could just be to 'remove the > > > visibility workaround for when the renderer crashes, and instead set the > > > visibility correctly, according to what the app thinks the visibility should > > be, > > > when we reload the crashed tab'. > > > > Removed. > > > > I tested on AURA Linux manually, and confirmed that it worked. > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_impl.cc:1262: #if > > defined(OS_ANDROID) > > On 2015/09/01 18:30:54, sievers wrote: > > > Can we just remove 1260-1272? > > > > Done. > > Ping? sorry for the long delay! sorry i'm actually wondering now: if we don't Hide() the widget when the renderer crashed, won't it still be visible if it happened to be visible before? do we actually have to make the logic you were adding for android the default and call WasHidden()? does your test actually pass now?
On 2015/09/04 01:51:47, sievers wrote: > On 2015/09/03 22:04:00, Jaekyun Seok wrote: > > On 2015/09/02 01:25:50, Jaekyun Seok wrote: > > > PTAL. > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > > > File content/browser/frame_host/render_frame_host_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > > > content/browser/frame_host/render_frame_host_manager.cc:430: #if > > > defined(OS_ANDROID) > > > On 2015/09/01 18:30:54, sievers wrote: > > > > Can we remove the ifdef Android? > > > > There shouldn't be a difference in behavior. And 'defined(AURA) && > > > > defined(OS_ANDROID)' needs to be working also. > > > > > > > > You can try on AURA Linux and see if that works for crashed tabs. > > > > > > > > In other words I was wondering if the fix could just be to 'remove the > > > > visibility workaround for when the renderer crashes, and instead set the > > > > visibility correctly, according to what the app thinks the visibility > should > > > be, > > > > when we reload the crashed tab'. > > > > > > Removed. > > > > > > I tested on AURA Linux manually, and confirmed that it worked. > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_impl.cc:1262: #if > > > defined(OS_ANDROID) > > > On 2015/09/01 18:30:54, sievers wrote: > > > > Can we just remove 1260-1272? > > > > > > Done. > > > > Ping? > > sorry for the long delay! > > sorry i'm actually wondering now: if we don't Hide() the widget when the > renderer crashed, won't it still be visible if it happened to be visible before? > do we actually have to make the logic you were adding for android the default > and call WasHidden()? does your test actually pass now? I confirmed that my test passed.
On 2015/09/04 02:11:45, Jaekyun Seok wrote: > On 2015/09/04 01:51:47, sievers wrote: > > On 2015/09/03 22:04:00, Jaekyun Seok wrote: > > > On 2015/09/02 01:25:50, Jaekyun Seok wrote: > > > > PTAL. > > > > > > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > > > > File content/browser/frame_host/render_frame_host_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/frame_h... > > > > content/browser/frame_host/render_frame_host_manager.cc:430: #if > > > > defined(OS_ANDROID) > > > > On 2015/09/01 18:30:54, sievers wrote: > > > > > Can we remove the ifdef Android? > > > > > There shouldn't be a difference in behavior. And 'defined(AURA) && > > > > > defined(OS_ANDROID)' needs to be working also. > > > > > > > > > > You can try on AURA Linux and see if that works for crashed tabs. > > > > > > > > > > In other words I was wondering if the fix could just be to 'remove the > > > > > visibility workaround for when the renderer crashes, and instead set the > > > > > visibility correctly, according to what the app thinks the visibility > > should > > > > be, > > > > > when we reload the crashed tab'. > > > > > > > > Removed. > > > > > > > > I tested on AURA Linux manually, and confirmed that it worked. > > > > > > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > > > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1308623003/diff/60001/content/browser/rendere... > > > > content/browser/renderer_host/render_widget_host_impl.cc:1262: #if > > > > defined(OS_ANDROID) > > > > On 2015/09/01 18:30:54, sievers wrote: > > > > > Can we just remove 1260-1272? > > > > > > > > Done. > > > > > > Ping? > > > > sorry for the long delay! > > > > sorry i'm actually wondering now: if we don't Hide() the widget when the > > renderer crashed, won't it still be visible if it happened to be visible > before? > > do we actually have to make the logic you were adding for android the default > > and call WasHidden()? does your test actually pass now? > > I confirmed that my test passed. But if the order of restoring tabs is changed, the test fails. I will revive the previous logic.
PTAL. I revived patch set 4.
On 2015/09/04 02:59:30, Jaekyun Seok wrote: > PTAL. > > I revived patch set 4. FYI, the above failures on chomeos test bots aren't related to this patch.
https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:436: if (dest_render_frame_host->GetView()->IsShowing() == Instead of the if-condition, does it work to put: DCHECK(!dest_render_frame_host->GetView()->IsShowing()) << "Crashed hosts should be set invisible."; if (delegate_->IsHidden()) dest_render_frame_host->GetView()->Hide(); else dest_render_frame_host->GetView()->Show(); https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1248: #if defined(OS_ANDROID) We don't need the ifdef, since the your change for restoring the tab works regardless of the platform, right?
PTAL. https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:436: if (dest_render_frame_host->GetView()->IsShowing() == On 2015/09/04 16:18:03, sievers wrote: > Instead of the if-condition, does it work to put: > > DCHECK(!dest_render_frame_host->GetView()->IsShowing()) << "Crashed hosts should > be set invisible."; > if (delegate_->IsHidden()) > dest_render_frame_host->GetView()->Hide(); > else > dest_render_frame_host->GetView()->Show(); I tested the DCHECK, and I realized that loading a page in a new tab also reached there because dest_render_frame_host->IsRenderFrameLive() only checks if the renderer is alive or not; it doesn't check if it was crashed or not. So we can't add the DCHECK. https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1248: #if defined(OS_ANDROID) On 2015/09/04 16:18:03, sievers wrote: > We don't need the ifdef, since the your change for restoring the tab works > regardless of the platform, right? Done.
On 2015/09/07 07:40:04, Jaekyun Seok wrote: > PTAL. > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... > content/browser/frame_host/render_frame_host_manager.cc:436: if > (dest_render_frame_host->GetView()->IsShowing() == > On 2015/09/04 16:18:03, sievers wrote: > > Instead of the if-condition, does it work to put: > > > > DCHECK(!dest_render_frame_host->GetView()->IsShowing()) << "Crashed hosts > should > > be set invisible."; > > if (delegate_->IsHidden()) > > dest_render_frame_host->GetView()->Hide(); > > else > > dest_render_frame_host->GetView()->Show(); > > I tested the DCHECK, and I realized that loading a page in a new tab also > reached there because dest_render_frame_host->IsRenderFrameLive() only checks if > the renderer is alive or not; it doesn't check if it was crashed or not. > > So we can't add the DCHECK. > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... > content/browser/renderer_host/render_widget_host_impl.cc:1248: #if > defined(OS_ANDROID) > On 2015/09/04 16:18:03, sievers wrote: > > We don't need the ifdef, since the your change for restoring the tab works > > regardless of the platform, right? > > Done. FYI, the second try on linux_android_rel_ng succeeded; http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r... .
On 2015/09/07 23:22:18, Jaekyun Seok wrote: > On 2015/09/07 07:40:04, Jaekyun Seok wrote: > > PTAL. > > > > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... > > File content/browser/frame_host/render_frame_host_manager.cc (right): > > > > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/frame_... > > content/browser/frame_host/render_frame_host_manager.cc:436: if > > (dest_render_frame_host->GetView()->IsShowing() == > > On 2015/09/04 16:18:03, sievers wrote: > > > Instead of the if-condition, does it work to put: > > > > > > DCHECK(!dest_render_frame_host->GetView()->IsShowing()) << "Crashed hosts > > should > > > be set invisible."; > > > if (delegate_->IsHidden()) > > > dest_render_frame_host->GetView()->Hide(); > > > else > > > dest_render_frame_host->GetView()->Show(); > > > > I tested the DCHECK, and I realized that loading a page in a new tab also > > reached there because dest_render_frame_host->IsRenderFrameLive() only checks > if > > the renderer is alive or not; it doesn't check if it was crashed or not. > > > > So we can't add the DCHECK. > > > > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > https://codereview.chromium.org/1308623003/diff/100001/content/browser/render... > > content/browser/renderer_host/render_widget_host_impl.cc:1248: #if > > defined(OS_ANDROID) > > On 2015/09/04 16:18:03, sievers wrote: > > > We don't need the ifdef, since the your change for restoring the tab works > > > regardless of the platform, right? > > > > Done. > > FYI, the second try on linux_android_rel_ng succeeded; > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r... > . Ping?
thanks for testing that out, that's good to know that new tabs also go through this path. just a few nits... https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java (right): https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:486: @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE) Why is the behavior different on low end wrt this test? https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:547: assertTrue("isInForeground() was not called for the process.", you mean 'setInForeground()' *was* called? https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:566: assertTrue("Renderer process isn't gone to the background.", you mean 'renderer process *was* backgrounded'? https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:567: CriteriaHelper.pollForCriteria(new Criteria() { do we really need to poll for visibility changes? or can we just check that right after we called hide()? https://codereview.chromium.org/1308623003/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:435: if (dest_render_frame_host->GetView() && Can you put a comment saying that after a renderer crash we'd have marked the host as invisible, so we need to set the visibility of the new View to the correct value here after reload?
PTAL. https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java (right): https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:486: @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE) On 2015/09/08 19:56:34, sievers wrote: > Why is the behavior different on low end wrt this test? I thought that this test didn't run on low end because only one renderer would be foreground one on such devices. But I realized the logic isn't related to this test. I will remove the line. https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:547: assertTrue("isInForeground() was not called for the process.", On 2015/09/08 19:56:34, sievers wrote: > you mean 'setInForeground()' *was* called? This is an error message when the condition is wrong. So "setInForeground() was not called for the process." is right. https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:566: assertTrue("Renderer process isn't gone to the background.", On 2015/09/08 19:56:34, sievers wrote: > you mean 'renderer process *was* backgrounded'? This is an error message when the condition is wrong. So the current message is right. Anyway this is removed. https://codereview.chromium.org/1308623003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java:567: CriteriaHelper.pollForCriteria(new Criteria() { On 2015/09/08 19:56:34, sievers wrote: > do we really need to poll for visibility changes? or can we just check that > right after we called hide()? I moved the checking behind tabs[1].hide(). https://codereview.chromium.org/1308623003/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:435: if (dest_render_frame_host->GetView() && On 2015/09/08 19:56:34, sievers wrote: > Can you put a comment saying that after a renderer crash we'd have marked the > host as invisible, so we need to set the visibility of the new View to the > correct value here after reload? Done.
lgtm
jaekyun@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@, please review chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java as an owner of chrome/android/.
The CQ bit was checked by jaekyun@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308623003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308623003/140001
lgtm
i just tried this on Mac, and unfortunately it breaks visibility of crashed tabs after reload there.
On 2015/09/08 23:58:22, sievers wrote: > i just tried this on Mac, and unfortunately it breaks visibility of crashed tabs > after reload there. Then how about applying this only to Android for safety?
On 2015/09/09 00:00:59, Jaekyun Seok wrote: > On 2015/09/08 23:58:22, sievers wrote: > > i just tried this on Mac, and unfortunately it breaks visibility of crashed > tabs > > after reload there. > > Then how about applying this only to Android for safety? no ifdefs in the code please if we can avoid it
On 2015/09/09 00:01:56, sievers wrote: > On 2015/09/09 00:00:59, Jaekyun Seok wrote: > > On 2015/09/08 23:58:22, sievers wrote: > > > i just tried this on Mac, and unfortunately it breaks visibility of crashed > > tabs > > > after reload there. > > > > Then how about applying this only to Android for safety? > > no ifdefs in the code please if we can avoid it I don't know how to test on Mac. Can I build one for Mac on linux machine?
On 2015/09/09 00:03:36, Jaekyun Seok wrote: > On 2015/09/09 00:01:56, sievers wrote: > > On 2015/09/09 00:00:59, Jaekyun Seok wrote: > > > On 2015/09/08 23:58:22, sievers wrote: > > > > i just tried this on Mac, and unfortunately it breaks visibility of > crashed > > > tabs > > > > after reload there. > > > > > > Then how about applying this only to Android for safety? > > > > no ifdefs in the code please if we can avoid it > > I don't know how to test on Mac. Can I build one for Mac on linux machine? i'll take a look on Mac
On 2015/09/09 00:05:45, sievers wrote: > On 2015/09/09 00:03:36, Jaekyun Seok wrote: > > On 2015/09/09 00:01:56, sievers wrote: > > > On 2015/09/09 00:00:59, Jaekyun Seok wrote: > > > > On 2015/09/08 23:58:22, sievers wrote: > > > > > i just tried this on Mac, and unfortunately it breaks visibility of > > crashed > > > > tabs > > > > > after reload there. > > > > > > > > Then how about applying this only to Android for safety? > > > > > > no ifdefs in the code please if we can avoid it > > > > I don't know how to test on Mac. Can I build one for Mac on linux machine? > > i'll take a look on Mac Thanks~
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1308623003/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:438: if (dest_render_frame_host->GetView() && Ok the bug is that we're checking the RWHV's visibility, because a newly created platform window is visible (at least on Mac), so we never end up going into the clause here. It works if you check |dest_render_frame_host->render_view_host()->is_hidden() != delegate_->IsHidden()| instead.
PTAL. https://codereview.chromium.org/1308623003/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1308623003/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:438: if (dest_render_frame_host->GetView() && On 2015/09/09 01:34:59, sievers wrote: > Ok the bug is that we're checking the RWHV's visibility, because a newly created > platform window is visible (at least on Mac), so we never end up going into the > clause here. It works if you check > |dest_render_frame_host->render_view_host()->is_hidden() != > delegate_->IsHidden()| instead. Thanks. I will change it.
The CQ bit was checked by jaekyun@chromium.org to run a CQ dry run
lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308623003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308623003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jaekyun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/1308623003/#ps160001 (title: "Check dest_render_frame_host->render_view_host()->is_hidden()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308623003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308623003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a163192a8629025a3dd21b9684354b244ce354a2 Cr-Commit-Position: refs/heads/master@{#347883} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
