Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2863003002:
Use the WebContents visibility as the initial visibility when creating a swapped-out RenderView. (Closed)
DescriptionUse the WebContents visibility as the initial visibility when
creating a swapped-out RenderView.
This CL changes the RenderView creation to use the WebContents
visibility when creating a swapped-out RenderView. This is
important because the browser doesn't track changes to the
visibility state of swapped-out RenderViewHosts, so the renderer
can end up having the wrong visibility when recovering from a
crash.
BUG=717597, 718213
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2863003002
Cr-Commit-Position: refs/heads/master@{#470116}
Committed: https://chromium.googlesource.com/chromium/src/+/b592bfa82d1cad4997766f751002f44ef37bd8a5
Patch Set 1 #
Total comments: 5
Patch Set 2 : address comment #
Messages
Total messages: 25 (15 generated)
Description was changed from ========== WIP ========== to ========== WIP CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WIP CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use the WebContents visibility as the initial visibility when creating a swapped-out RenderView. This CL changes the RenderView creation to use the WebContents visibility when creating a swapped-out RenderView. This is important because the browser doesn't track changes to the visibility state of swapped-out RenderViewHosts, so the renderer can end up having the wrong visibility when recovering from a crash. BUG=717597,718213 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use the WebContents visibility as the initial visibility when creating a swapped-out RenderView. This CL changes the RenderView creation to use the WebContents visibility when creating a swapped-out RenderView. This is important because the browser doesn't track changes to the visibility state of swapped-out RenderViewHosts, so the renderer can end up having the wrong visibility when recovering from a crash. BUG=717597,718213 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use the WebContents visibility as the initial visibility when creating a swapped-out RenderView. This CL changes the RenderView creation to use the WebContents visibility when creating a swapped-out RenderView. This is important because the browser doesn't track changes to the visibility state of swapped-out RenderViewHosts, so the renderer can end up having the wrong visibility when recovering from a crash. BUG=717597,718213 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
lfg@chromium.org changed reviewers: + nasko@chromium.org
Nasko, can you take a look?
https://codereview.chromium.org/2863003002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2863003002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_view_host_impl.cc:313: : GetWidget()->delegate()->IsHidden(); Wouldn't the value of GetWidget()->delegate()->IsHidden() always be correct? https://codereview.chromium.org/2863003002/diff/1/content/browser/site_per_pr... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2863003002/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:8865: // Make sure that the child frame has submitted at least one frame. nit: This talks about compositor frames, right? Maybe add that to make it clear.
https://codereview.chromium.org/2863003002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2863003002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_view_host_impl.cc:313: : GetWidget()->delegate()->IsHidden(); On 2017/05/05 18:42:55, nasko wrote: > Wouldn't the value of GetWidget()->delegate()->IsHidden() always be correct? It's not correct for interstitials. It's also not correct for BrowserPlugin based guests. https://codereview.chromium.org/2863003002/diff/1/content/browser/site_per_pr... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2863003002/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:8865: // Make sure that the child frame has submitted at least one frame. On 2017/05/05 18:42:55, nasko wrote: > nit: This talks about compositor frames, right? Maybe add that to make it clear. Done.
LGTM https://codereview.chromium.org/2863003002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2863003002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_view_host_impl.cc:313: : GetWidget()->delegate()->IsHidden(); On 2017/05/05 18:53:02, lfg wrote: > On 2017/05/05 18:42:55, nasko wrote: > > Wouldn't the value of GetWidget()->delegate()->IsHidden() always be correct? > > It's not correct for interstitials. It's also not correct for BrowserPlugin > based guests. Acknowledged.
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494275798578540,
"parent_rev": "c78da1ff9f01a79c89aa715e2dd42ff0dfde0cec", "commit_rev":
"b592bfa82d1cad4997766f751002f44ef37bd8a5"}
Message was sent while issue was closed.
Description was changed from ========== Use the WebContents visibility as the initial visibility when creating a swapped-out RenderView. This CL changes the RenderView creation to use the WebContents visibility when creating a swapped-out RenderView. This is important because the browser doesn't track changes to the visibility state of swapped-out RenderViewHosts, so the renderer can end up having the wrong visibility when recovering from a crash. BUG=717597,718213 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use the WebContents visibility as the initial visibility when creating a swapped-out RenderView. This CL changes the RenderView creation to use the WebContents visibility when creating a swapped-out RenderView. This is important because the browser doesn't track changes to the visibility state of swapped-out RenderViewHosts, so the renderer can end up having the wrong visibility when recovering from a crash. BUG=717597,718213 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2863003002 Cr-Commit-Position: refs/heads/master@{#470116} Committed: https://chromium.googlesource.com/chromium/src/+/b592bfa82d1cad4997766f751002... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b592bfa82d1cad4997766f751002... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
