Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1385543003:
Have RenderWidgetHostViewAndroid own sync compositor (Closed)
|
Created:
5 years, 2 months ago by boliu Modified:
5 years, 2 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su, android-webview-reviews_chromium.org, gsennton Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHave RenderWidgetHostViewAndroid own sync compositor
Also simplify object lifetimes. Have SynchronousCompositorClient
outlive SynchronousCompositor so no need to have code to handle
setting and unsetting client.
BUG=509702
Committed: https://crrev.com/4f295c427a1b0f08fadf03df85be74ff83ca1734
Cr-Commit-Position: refs/heads/master@{#353221}
Patch Set 1 #Patch Set 2 : mostly works #Patch Set 3 : multiprocess works #Patch Set 4 : clean up #Patch Set 5 : rebase #
Total comments: 3
Patch Set 6 : add dcheck #
Total comments: 6
Patch Set 7 : review #
Total comments: 12
Patch Set 8 : review #
Total comments: 2
Patch Set 9 : rebase #Messages
Total messages: 31 (3 generated)
boliu@chromium.org changed reviewers: + jdduke@chromium.org, sievers@chromium.org
ptal
No objections on my end. https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1771: sync_compositor_.reset(); What about the case where we transition from one valid CVC to another valid CVC? Should this reset go aove in the |content_view_core != content_view_core_| check?
https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1771: sync_compositor_.reset(); On 2015/10/05 19:56:03, jdduke wrote: > What about the case where we transition from one valid CVC to another valid CVC? > Should this reset go aove in the |content_view_core != content_view_core_| > check? Assume for now there will be only one RenderView per WebContents in webview. Given that... Why does CVC change here? I mean what is it for?
https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1771: sync_compositor_.reset(); On 2015/10/06 00:05:30, boliu (slow to review) wrote: > On 2015/10/05 19:56:03, jdduke wrote: > > What about the case where we transition from one valid CVC to another valid > CVC? > > Should this reset go aove in the |content_view_core != content_view_core_| > > check? > > Assume for now there will be only one RenderView per WebContents in webview. > > Given that... Why does CVC change here? I mean what is it for? Let me re-phrase that a bit better, now that I'm not on a phone anymore. In what siuation does clank need to switch the CVC instance of RWHVA? I thought the relationship is one to many, but the many is RVHWVA, not CVC?
On 2015/10/06 01:28:04, boliu (slow to review) wrote: > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_android.cc:1771: > sync_compositor_.reset(); > On 2015/10/06 00:05:30, boliu (slow to review) wrote: > > On 2015/10/05 19:56:03, jdduke wrote: > > > What about the case where we transition from one valid CVC to another valid > > CVC? > > > Should this reset go aove in the |content_view_core != content_view_core_| > > > check? > > > > Assume for now there will be only one RenderView per WebContents in webview. > > > > Given that... Why does CVC change here? I mean what is it for? > > Let me re-phrase that a bit better, now that I'm not on a phone anymore. > > In what siuation does clank need to switch the CVC instance of RWHVA? I thought > the relationship is one to many, but the many is RVHWVA, not CVC? Interstitials? Or preconnect perhaps? Why else would we specifically allow for that case, rather than the usual assert that we're switching from or to null.
On 2015/10/06 04:03:10, jdduke wrote: > On 2015/10/06 01:28:04, boliu (slow to review) wrote: > > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > > > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_view_android.cc:1771: > > sync_compositor_.reset(); > > On 2015/10/06 00:05:30, boliu (slow to review) wrote: > > > On 2015/10/05 19:56:03, jdduke wrote: > > > > What about the case where we transition from one valid CVC to another > valid > > > CVC? > > > > Should this reset go aove in the |content_view_core != content_view_core_| > > > > check? > > > > > > Assume for now there will be only one RenderView per WebContents in webview. > > > > > > Given that... Why does CVC change here? I mean what is it for? > > > > Let me re-phrase that a bit better, now that I'm not on a phone anymore. > > > > In what siuation does clank need to switch the CVC instance of RWHVA? I > thought > > the relationship is one to many, but the many is RVHWVA, not CVC? > > Interstitials? Or preconnect perhaps? Why else would we specifically allow for > that case, rather than the usual assert that we're switching from or to null. In these cases, the WebContents gets a new RenderViewHost. But RenderViewHost still points to a single WebContents (or null), right? And since CVC is per WebContents, and RWHVA is per RVH, RWHVA should still map to a single ContentViewCore (or null)? I didn't find code that says otherwise, so I thought my reasoning was correct..?
On 2015/10/06 12:13:50, boliu (slow to review) wrote: > On 2015/10/06 04:03:10, jdduke wrote: > > On 2015/10/06 01:28:04, boliu (slow to review) wrote: > > > > > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_view_android.cc > (right): > > > > > > > > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_view_android.cc:1771: > > > sync_compositor_.reset(); > > > On 2015/10/06 00:05:30, boliu (slow to review) wrote: > > > > On 2015/10/05 19:56:03, jdduke wrote: > > > > > What about the case where we transition from one valid CVC to another > > valid > > > > CVC? > > > > > Should this reset go aove in the |content_view_core != > content_view_core_| > > > > > check? > > > > > > > > Assume for now there will be only one RenderView per WebContents in > webview. > > > > > > > > Given that... Why does CVC change here? I mean what is it for? > > > > > > Let me re-phrase that a bit better, now that I'm not on a phone anymore. > > > > > > In what siuation does clank need to switch the CVC instance of RWHVA? I > > thought > > > the relationship is one to many, but the many is RVHWVA, not CVC? > > > > Interstitials? Or preconnect perhaps? Why else would we specifically allow for > > that case, rather than the usual assert that we're switching from or to null. > > In these cases, the WebContents gets a new RenderViewHost. But RenderViewHost > still points to a single WebContents (or null), right? > > And since CVC is per WebContents, and RWHVA is per RVH, RWHVA should still map > to a single ContentViewCore (or null)? I didn't find code that says otherwise, > so I thought my reasoning was correct..? Let's see what the trybot says about https://codereview.chromium.org/1387563005/. You are probably right, but you just haven't pointed to me where this switch happens.
On 2015/10/06 12:27:07, boliu (slow to review) wrote: > On 2015/10/06 12:13:50, boliu (slow to review) wrote: > > On 2015/10/06 04:03:10, jdduke wrote: > > > On 2015/10/06 01:28:04, boliu (slow to review) wrote: > > > > > > > > > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > > > > File content/browser/renderer_host/render_widget_host_view_android.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1385543003/diff/80001/content/browser/rendere... > > > > content/browser/renderer_host/render_widget_host_view_android.cc:1771: > > > > sync_compositor_.reset(); > > > > On 2015/10/06 00:05:30, boliu (slow to review) wrote: > > > > > On 2015/10/05 19:56:03, jdduke wrote: > > > > > > What about the case where we transition from one valid CVC to another > > > valid > > > > > CVC? > > > > > > Should this reset go aove in the |content_view_core != > > content_view_core_| > > > > > > check? > > > > > > > > > > Assume for now there will be only one RenderView per WebContents in > > webview. > > > > > > > > > > Given that... Why does CVC change here? I mean what is it for? > > > > > > > > Let me re-phrase that a bit better, now that I'm not on a phone anymore. > > > > > > > > In what siuation does clank need to switch the CVC instance of RWHVA? I > > > thought > > > > the relationship is one to many, but the many is RVHWVA, not CVC? > > > > > > Interstitials? Or preconnect perhaps? Why else would we specifically allow > for > > > that case, rather than the usual assert that we're switching from or to > null. > > > > In these cases, the WebContents gets a new RenderViewHost. But RenderViewHost > > still points to a single WebContents (or null), right? > > > > And since CVC is per WebContents, and RWHVA is per RVH, RWHVA should still map > > to a single ContentViewCore (or null)? I didn't find code that says otherwise, > > so I thought my reasoning was correct..? > > Let's see what the trybot says about > https://codereview.chromium.org/1387563005/. You are probably right, but you > just haven't pointed to me where this switch happens. The CHECK fails in content_browsertests from what looks like cross-domain navigations. The check fails on the new RVH/RWHVA that went from pending to active when the navigate commits. I tried to look at what the WebContents instance was the RVH pointing to, when it was first created as a pending navigation, but I gave up on cs... I assume it's some kind of temporary WebContents or something. (And the bot didn't run any chrome instrumentation tests, wtf?) So, webview assumes the WebContents <-> RVH switch never happens, so webview never supported interstitials, preconnect etc. And all the webview tests passed. So can think of this patch as the first step to supporting the switch. Ccould add that check say behind !using_browser_compositor_?
On 2015/10/06 16:27:36, boliu (slow to review) wrote: > The CHECK fails in content_browsertests from what looks like cross-domain > navigations. The check fails on the new RVH/RWHVA that went from pending to > active when the navigate commits. I tried to look at what the WebContents > instance was the RVH pointing to, when it was first created as a pending > navigation, but I gave up on cs... I assume it's some kind of temporary > WebContents or something. (And the bot didn't run any chrome instrumentation > tests, wtf?) > > So, webview assumes the WebContents <-> RVH switch never happens, so webview > never supported interstitials, preconnect etc. And all the webview tests passed. > So can think of this patch as the first step to supporting the switch. Ccould > add that check say behind !using_browser_compositor_? Fine with me, as long as it's a CHECK initially.
On 2015/10/06 16:56:30, jdduke wrote: > On 2015/10/06 16:27:36, boliu (slow to review) wrote: > > The CHECK fails in content_browsertests from what looks like cross-domain > > navigations. The check fails on the new RVH/RWHVA that went from pending to > > active when the navigate commits. I tried to look at what the WebContents > > instance was the RVH pointing to, when it was first created as a pending > > navigation, but I gave up on cs... I assume it's some kind of temporary > > WebContents or something. (And the bot didn't run any chrome instrumentation > > tests, wtf?) > > > > So, webview assumes the WebContents <-> RVH switch never happens, so webview > > never supported interstitials, preconnect etc. And all the webview tests > passed. > > So can think of this patch as the first step to supporting the switch. Ccould > > add that check say behind !using_browser_compositor_? > > Fine with me, as long as it's a CHECK initially. You added it as a DCHECK, which we've found aren't very useful on WebView, right? Particularly if there's some uncertainty here?
On 2015/10/06 16:57:15, jdduke wrote: > On 2015/10/06 16:56:30, jdduke wrote: > > On 2015/10/06 16:27:36, boliu (slow to review) wrote: > > > The CHECK fails in content_browsertests from what looks like cross-domain > > > navigations. The check fails on the new RVH/RWHVA that went from pending to > > > active when the navigate commits. I tried to look at what the WebContents > > > instance was the RVH pointing to, when it was first created as a pending > > > navigation, but I gave up on cs... I assume it's some kind of temporary > > > WebContents or something. (And the bot didn't run any chrome instrumentation > > > tests, wtf?) > > > > > > So, webview assumes the WebContents <-> RVH switch never happens, so webview > > > never supported interstitials, preconnect etc. And all the webview tests > > passed. > > > So can think of this patch as the first step to supporting the switch. > Ccould > > > add that check say behind !using_browser_compositor_? > > > > Fine with me, as long as it's a CHECK initially. > > You added it as a DCHECK, which we've found aren't very useful on WebView, > right? Particularly if there's some uncertainty here? I dont think it's worth the cost of a release-time check, because if it happens today, then webview rendering will break, with or without this change.
On 2015/10/06 16:58:32, boliu (slow to review) wrote: > On 2015/10/06 16:57:15, jdduke wrote: > > On 2015/10/06 16:56:30, jdduke wrote: > > > On 2015/10/06 16:27:36, boliu (slow to review) wrote: > > > > The CHECK fails in content_browsertests from what looks like cross-domain > > > > navigations. The check fails on the new RVH/RWHVA that went from pending > to > > > > active when the navigate commits. I tried to look at what the WebContents > > > > instance was the RVH pointing to, when it was first created as a pending > > > > navigation, but I gave up on cs... I assume it's some kind of temporary > > > > WebContents or something. (And the bot didn't run any chrome > instrumentation > > > > tests, wtf?) > > > > > > > > So, webview assumes the WebContents <-> RVH switch never happens, so > webview > > > > never supported interstitials, preconnect etc. And all the webview tests > > > passed. > > > > So can think of this patch as the first step to supporting the switch. > > Ccould > > > > add that check say behind !using_browser_compositor_? > > > > > > Fine with me, as long as it's a CHECK initially. > > > > You added it as a DCHECK, which we've found aren't very useful on WebView, > > right? Particularly if there's some uncertainty here? > > I dont think it's worth the cost of a release-time check, because if it happens > today, then webview rendering will break, with or without this change. OK, sounds like you know all the ways this could possibly break, and it would be immediately obvious why it's broken, to whomever encounters the breakage... I technically own just input in this directory so deferring to sievers@ for the rest.
https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:331: using_browser_compositor_(CompositorImpl::IsInitialized()), Can we replace occurences of |using_browser_compositor_| with |sync_compositor_| checks? For the place where we decide to create it - any chance we could change the logic to 'is using sync compositor' rather than 'browser compositor is initialized' (which really isn't a thing)? https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1289: if (!sync_compositor_) { Just DCHECK() here, the call site should be checking this already. https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1743: // Android webview does not currently support switching ContentViewCoreImpl. Hmm, I don't think this should ever happen, but maybe I'm missing something. It seems really odd that we'd be transitioning a RWHV to a different WebContents.
ptal https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:331: using_browser_compositor_(CompositorImpl::IsInitialized()), On 2015/10/07 00:31:51, sievers wrote: > Can we replace occurences of |using_browser_compositor_| with |sync_compositor_| > checks? > > For the place where we decide to create it - any chance we could change the > logic to 'is using sync compositor' rather than 'browser compositor is > initialized' (which really isn't a thing)? Done. https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1289: if (!sync_compositor_) { On 2015/10/07 00:31:51, sievers wrote: > Just DCHECK() here, the call site should be checking this already. Done. https://codereview.chromium.org/1385543003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1743: // Android webview does not currently support switching ContentViewCoreImpl. On 2015/10/07 00:31:51, sievers wrote: > Hmm, I don't think this should ever happen, but maybe I'm missing something. It > seems really odd that we'd be transitioning a RWHV to a different WebContents. So you are in my camp :) I don't really understand either. I'll find out more after I have time to investigate, but this should be good enough for now.
On 2015/10/07 15:01:33, boliu (slow to review) wrote: > So you are in my camp :) > > I don't really understand either. I'll find out more after I have time to > investigate, but this should be good enough for now. ? I'm not in a camp, I simply mentioned that we appear to properly handle this case now, so we should figure out if we really need to handle this case or it's a bug that we handle it.
On 2015/10/07 15:09:05, jdduke wrote: > On 2015/10/07 15:01:33, boliu (slow to review) wrote: > > So you are in my camp :) > > > > I don't really understand either. I'll find out more after I have time to > > investigate, but this should be good enough for now. > > ? I'm not in a camp, I simply mentioned that we appear to properly handle this > case now, so we should figure out if we really need to handle this case or it's > a bug that we handle it. The camp of "feels like this case should not happen for chrome either" :p
On 2015/10/07 15:15:11, boliu (slow to review) wrote: > On 2015/10/07 15:09:05, jdduke wrote: > > On 2015/10/07 15:01:33, boliu (slow to review) wrote: > > > So you are in my camp :) > > > > > > I don't really understand either. I'll find out more after I have time to > > > investigate, but this should be good enough for now. > > > > ? I'm not in a camp, I simply mentioned that we appear to properly handle this > > case now, so we should figure out if we really need to handle this case or > it's > > a bug that we handle it. > > The camp of "feels like this case should not happen for chrome either" :p feel free to make it a CHECK() (and regardless of webview or not). then we'll know for sure.
On 2015/10/07 18:39:03, sievers wrote: > On 2015/10/07 15:15:11, boliu (slow to review) wrote: > > On 2015/10/07 15:09:05, jdduke wrote: > > > On 2015/10/07 15:01:33, boliu (slow to review) wrote: > > > > So you are in my camp :) > > > > > > > > I don't really understand either. I'll find out more after I have time to > > > > investigate, but this should be good enough for now. > > > > > > ? I'm not in a camp, I simply mentioned that we appear to properly handle > this > > > case now, so we should figure out if we really need to handle this case or > > it's > > > a bug that we handle it. > > > > The camp of "feels like this case should not happen for chrome either" :p > > feel free to make it a CHECK() (and regardless of webview or not). then we'll > know for sure. Ehh, it definitely do happen in chrome, apparently during cross-domain navigations: https://codereview.chromium.org/1387563005/ It's just I don't understand why it happens, and haven't had time to find out
https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:874: if (!sync_compositor_ && !IsSurfaceAvailableForCopy()) { nit: Actually even better would be to remove the sync_compositor_ check here and add this: bool RenderWidgetHostViewAndroid::IsSurfaceAvailableForCopy() const { // We can always produce a frame in that case. if (sync_compositor_) return true; https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1740: !content_view_core || !content_view_core_); So this is what I think we support here: - setting CVC to NULL when WebContents gets destroyed - setting CVC to NULL when Java CVC gets destroyed (because CVC is not functional then) - reset to null from destructor - calling this from RWHVA constructor i.e. init with CVC/WebContents - the renderer crashed and we create a new RWHV for an existing CVC/WebContents. I'm *guessing* that RWHVA might then get created without a CVC and we later call SetCVC() from ContentViewCoreImpl::RenderViewHostChanged(). So the check could actually go a bit further and also rule out this: RWHVA::SetCVC(cvc1); RWHVA::SetCVC(null); RWHVA::SetCVC(cvc2); https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1769: sync_compositor_.reset(); Actually I missed that if you do this, then some of the check for sync_compositor_ in here might be dangerous and lead you to take the browser compositor path when the WebContents went away or so.
On 2015/10/07 18:43:30, boliu (slow to review) wrote: > On 2015/10/07 18:39:03, sievers wrote: > > On 2015/10/07 15:15:11, boliu (slow to review) wrote: > > > On 2015/10/07 15:09:05, jdduke wrote: > > > > On 2015/10/07 15:01:33, boliu (slow to review) wrote: > > > > > So you are in my camp :) > > > > > > > > > > I don't really understand either. I'll find out more after I have time > to > > > > > investigate, but this should be good enough for now. > > > > > > > > ? I'm not in a camp, I simply mentioned that we appear to properly handle > > this > > > > case now, so we should figure out if we really need to handle this case or > > > it's > > > > a bug that we handle it. > > > > > > The camp of "feels like this case should not happen for chrome either" :p > > > > feel free to make it a CHECK() (and regardless of webview or not). then we'll > > know for sure. > > Ehh, it definitely do happen in chrome, apparently during cross-domain > navigations: https://codereview.chromium.org/1387563005/ > It's just I don't understand why it happens, and haven't had time to find out Interesting. Maybe content_view_core_ == content_view_core?
https://codereview.chromium.org/1385543003/diff/120001/content/browser/androi... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/1385543003/diff/120001/content/browser/androi... content/browser/android/in_process/synchronous_compositor_impl.cc:35: class ClientHolder : public base::SupportsUserData::Data { That seems really complicated to store a pointer. Can the ContentViewCore or something else (WebContentsAndroid?) maybe just have the pointer?
On 2015/10/07 18:54:51, sievers wrote: > On 2015/10/07 18:43:30, boliu (slow to review) wrote: > > On 2015/10/07 18:39:03, sievers wrote: > > > On 2015/10/07 15:15:11, boliu (slow to review) wrote: > > > > On 2015/10/07 15:09:05, jdduke wrote: > > > > > On 2015/10/07 15:01:33, boliu (slow to review) wrote: > > > > > > So you are in my camp :) > > > > > > > > > > > > I don't really understand either. I'll find out more after I have time > > to > > > > > > investigate, but this should be good enough for now. > > > > > > > > > > ? I'm not in a camp, I simply mentioned that we appear to properly > handle > > > this > > > > > case now, so we should figure out if we really need to handle this case > or > > > > it's > > > > > a bug that we handle it. > > > > > > > > The camp of "feels like this case should not happen for chrome either" :p > > > > > > feel free to make it a CHECK() (and regardless of webview or not). then > we'll > > > know for sure. > > > > Ehh, it definitely do happen in chrome, apparently during cross-domain > > navigations: https://codereview.chromium.org/1387563005/ > > It's just I don't understand why it happens, and haven't had time to find out > > Interesting. Maybe content_view_core_ == content_view_core? FYI you are right! (Even though the bot did not run any chrome tests..)
https://codereview.chromium.org/1385543003/diff/120001/content/browser/androi... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/1385543003/diff/120001/content/browser/androi... content/browser/android/in_process/synchronous_compositor_impl.cc:35: class ClientHolder : public base::SupportsUserData::Data { On 2015/10/07 19:13:38, sievers wrote: > That seems really complicated to store a pointer. > Can the ContentViewCore or something else (WebContentsAndroid?) maybe just have > the pointer? Yeah can't be CVC because that could be created much later in the pop up case. WebContentsAndroid it is! https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:874: if (!sync_compositor_ && !IsSurfaceAvailableForCopy()) { On 2015/10/07 18:52:58, sievers wrote: > nit: Actually even better would be to remove the sync_compositor_ check here and > add this: > > bool RenderWidgetHostViewAndroid::IsSurfaceAvailableForCopy() const { > // We can always produce a frame in that case. > if (sync_compositor_) > return true; Done. https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1740: !content_view_core || !content_view_core_); On 2015/10/07 18:52:58, sievers wrote: > So this is what I think we support here: > > - setting CVC to NULL when WebContents gets destroyed > - setting CVC to NULL when Java CVC gets destroyed (because CVC is not > functional then) > - reset to null from destructor > - calling this from RWHVA constructor i.e. init with CVC/WebContents > - the renderer crashed and we create a new RWHV for an existing CVC/WebContents. That case sounds the same as normally creating CVC/WebContents, ie constructor has the CVC pointer? Other case where CVC can be set after construction include pop up windows (at least in webview) > I'm *guessing* that RWHVA might then get created without a CVC and we later call > SetCVC() from ContentViewCoreImpl::RenderViewHostChanged(). > > So the check could actually go a bit further and also rule out this: > RWHVA::SetCVC(cvc1); > RWHVA::SetCVC(null); > RWHVA::SetCVC(cvc2); Err, how do you do that inside SetCVC itself? Want me to add a bool that's like "had_cvc_before_"? https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1769: sync_compositor_.reset(); On 2015/10/07 18:52:57, sievers wrote: > Actually I missed that if you do this, then some of the check for > sync_compositor_ in here might be dangerous and lead you to take the browser > compositor path when the WebContents went away or so. Hmm, you are right. CVC can be set much later after RWHVA is created, so even checking if there is a sync client set is not good enough in general. Reverted back to using_browser_compositor_ logic I guess
lgtm w/comments https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1740: !content_view_core || !content_view_core_); On 2015/10/08 03:14:44, boliu (slow to review) wrote: > On 2015/10/07 18:52:58, sievers wrote: > > So this is what I think we support here: > > > > - setting CVC to NULL when WebContents gets destroyed > > - setting CVC to NULL when Java CVC gets destroyed (because CVC is not > > functional then) > > - reset to null from destructor > > - calling this from RWHVA constructor i.e. init with CVC/WebContents > > - the renderer crashed and we create a new RWHV for an existing > CVC/WebContents. > > That case sounds the same as normally creating CVC/WebContents, ie constructor > has the CVC pointer? > > Other case where CVC can be set after construction include pop up windows (at > least in webview) > > > I'm *guessing* that RWHVA might then get created without a CVC and we later > call > > SetCVC() from ContentViewCoreImpl::RenderViewHostChanged(). > > > > So the check could actually go a bit further and also rule out this: > > RWHVA::SetCVC(cvc1); > > RWHVA::SetCVC(null); > > RWHVA::SetCVC(cvc2); > > Err, how do you do that inside SetCVC itself? Want me to add a bool that's like > "had_cvc_before_"? Maybe splitting up this function into AttachToContentViewCore(CVC*) and DetachFromContentViewCore() would also make it a bit clearer what's going on here. But would still need some bool was_attached_ that can never be reset. Maybe something for a separate patch. Although... you do care about this, right? Because if it happened you'd end up with an existing SyncCompositor that points to the wrong (and possibly deleted) SyncCompositorClient. (There are also most likely corner case bugs in this anyways since calling WCV->SetContentViewCore()from CVC is dynamic wrt what RWHV it ends up forwarding this to.) https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1769: sync_compositor_.reset(); On 2015/10/08 03:14:44, boliu (slow to review) wrote: > On 2015/10/07 18:52:57, sievers wrote: > > Actually I missed that if you do this, then some of the check for > > sync_compositor_ in here might be dangerous and lead you to take the browser > > compositor path when the WebContents went away or so. > > Hmm, you are right. CVC can be set much later after RWHVA is created, so even > checking if there is a sync client set is not good enough in general. Reverted > back to using_browser_compositor_ logic I guess Ok. Maybe we can revisit later. For example, maybe it's nicer to always have a fixed interface here that always exist (but will then internally get detached from the SyncCompositor or maybe just the SyncCompositor getting detached from its client). It's a bit related to my other comment about WebContents and views/windows. https://codereview.chromium.org/1385543003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1793: this, content_view_core_->GetWebContents()); It's a bit ugly to see WebContents in RWHV. Really this is supposed to be decoupled (the browser-owned object vs. the one with lifetime coupled to renderer-side object). (*) Maybe you can at least hide it in CVC by having something like ContentViewCore::GetSynchronousCompositorClient()? (*) But so is the nature of SetCVC() and all of this here - I think what we'd really want to have for all of this is another level of indirection that's more like how this traditionally was supposed to work, where each RWHV represents its own platform window instance.
tl;dr: did not change anything https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1740: !content_view_core || !content_view_core_); On 2015/10/08 22:39:29, sievers wrote: > On 2015/10/08 03:14:44, boliu (slow to review) wrote: > > On 2015/10/07 18:52:58, sievers wrote: > > > So this is what I think we support here: > > > > > > - setting CVC to NULL when WebContents gets destroyed > > > - setting CVC to NULL when Java CVC gets destroyed (because CVC is not > > > functional then) > > > - reset to null from destructor > > > - calling this from RWHVA constructor i.e. init with CVC/WebContents > > > - the renderer crashed and we create a new RWHV for an existing > > CVC/WebContents. > > > > That case sounds the same as normally creating CVC/WebContents, ie constructor > > has the CVC pointer? > > > > Other case where CVC can be set after construction include pop up windows (at > > least in webview) > > > > > I'm *guessing* that RWHVA might then get created without a CVC and we later > > call > > > SetCVC() from ContentViewCoreImpl::RenderViewHostChanged(). > > > > > > So the check could actually go a bit further and also rule out this: > > > RWHVA::SetCVC(cvc1); > > > RWHVA::SetCVC(null); > > > RWHVA::SetCVC(cvc2); > > > > Err, how do you do that inside SetCVC itself? Want me to add a bool that's > like > > "had_cvc_before_"? > > Maybe splitting up this function into AttachToContentViewCore(CVC*) and > DetachFromContentViewCore() would also make it a bit clearer what's going on > here. But would still need some bool was_attached_ that can never be reset. > > Maybe something for a separate patch. Although... you do care about this, right? > Because if it happened you'd end up with an existing SyncCompositor that points > to the wrong (and possibly deleted) SyncCompositorClient. Yes. But that never happens (yet) in webview, since it would have completely broken rendering, with or without this patch. > > (There are also most likely corner case bugs in this anyways since calling > WCV->SetContentViewCore()from CVC is dynamic wrt what RWHV it ends up forwarding > this to.) https://codereview.chromium.org/1385543003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1769: sync_compositor_.reset(); On 2015/10/08 22:39:29, sievers wrote: > On 2015/10/08 03:14:44, boliu (slow to review) wrote: > > On 2015/10/07 18:52:57, sievers wrote: > > > Actually I missed that if you do this, then some of the check for > > > sync_compositor_ in here might be dangerous and lead you to take the browser > > > compositor path when the WebContents went away or so. > > > > Hmm, you are right. CVC can be set much later after RWHVA is created, so even > > checking if there is a sync client set is not good enough in general. Reverted > > back to using_browser_compositor_ logic I guess > > Ok. Maybe we can revisit later. For example, maybe it's nicer to always have a > fixed interface here that always exist (but will then internally get detached > from the SyncCompositor or maybe just the SyncCompositor getting detached from > its client). It's a bit related to my other comment about WebContents and > views/windows. Maybe don't need that. I think most RVHWA is created with a CVC, so has the SyncCompositorClient set already. For the edge case RVHWA created for pop up, could just inherit the value from the source RVHWA where the pop up is created from. But yeah, separate patch maybe. https://codereview.chromium.org/1385543003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1385543003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1793: this, content_view_core_->GetWebContents()); On 2015/10/08 22:39:29, sievers wrote: > It's a bit ugly to see WebContents in RWHV. Really this is supposed to be > decoupled (the browser-owned object vs. the one with lifetime coupled to > renderer-side object). (*) > > Maybe you can at least hide it in CVC by having something like > ContentViewCore::GetSynchronousCompositorClient()? The practical argument: lots of code here already does this. I don't think it's worth "hiding" this one call. > > (*) But so is the nature of SetCVC() and all of this here - I think what we'd > really want to have for all of this is another level of indirection that's more > like how this traditionally was supposed to work, where each RWHV represents its > own platform window instance. IMO there is nothing wrong with accessing objects with lifetimes tied to WebContents here. But maybe it could be like a delegate interface instead of accessing the types directly. (Did I just rephrase exactly what you said..?)
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1385543003/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1385543003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1385543003/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/4f295c427a1b0f08fadf03df85be74ff83ca1734 Cr-Commit-Position: refs/heads/master@{#353221} |
