Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26753005:
Adding compositor callbacks to RenderWidgetHostViewAndroid (Closed)
|
Created:
7 years, 2 months ago by powei Modified:
7 years, 1 month ago CC:
chromium-reviews, yusukes+watch_chromium.org, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org, jochen (gone - plz use gerrit), jdduke (slow), jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdding compositor callbacks to RenderWidgetHostViewAndroid
The coupling between the compositor and the RenderWidgetHostViewAndroid (RWHVA)
is weak. This causes problem such as when RWHVA needs to act on a compositor
commit (as in the current DidCommitFrameData callback).
We create a WindowAndroidObserver class and add a list of observers to the
android window, which then forwards the callbacks from the compositor to the
observers.
android= https://gerrit-int.chromium.org/#/c/44978/
BUG=273250
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236598
Patch Set 1 #Patch Set 2 : removed blank lines #
Total comments: 10
Patch Set 3 : Addressed comments #Patch Set 4 : rebased #Patch Set 5 : Fixed null pointer problems #Patch Set 6 : Change RWHVA to add itself to the compositor as per sievers@ comment #
Total comments: 18
Patch Set 7 : Separate Compositor plumbing from SetContentView #
Total comments: 12
Patch Set 8 : Plumb compositor provider through instead of compositor #
Total comments: 10
Patch Set 9 : host observers through WindowAndroid #
Total comments: 10
Patch Set 10 : addressed comments #
Total comments: 10
Patch Set 11 : addressed comments #
Total comments: 17
Patch Set 12 : addressed comments and rebased #
Total comments: 4
Patch Set 13 : addressed one more comment #Patch Set 14 : rebased and fixed bug with assert #Patch Set 15 : rebase #Messages
Total messages: 43 (0 generated)
PTAL. Thanks! Is it now safe to remove the implementation of DelegatedRendererLayerClient from RWHVA?
On 2013/10/10 01:34:12, powei wrote: > PTAL. Thanks! > > Is it now safe to remove the implementation of DelegatedRendererLayerClient from > RWHVA? drive-by: if so, please remove it all the way to cc, that's the only user.
ContentViewRenderView is only used by content shell (and some secondary uses like legal docs webview), so you also need to plumb it through Clank code.
https://codereview.chromium.org/26753005/diff/2001/content/browser/android/co... File content/browser/android/content_view_render_view.cc (right): https://codereview.chromium.org/26753005/diff/2001/content/browser/android/co... content/browser/android/content_view_render_view.cc:60: if (rwhva) { This is the sort of logic I was afraid of :) And is it not broken if you navigate across domains in a given tab? Because in that case a new RWHV gets created and attached to the ContentViewCore without us noticing here. In other words, I don't think it's safe to use content_view->GetRWHV(). You also have to make sure that a RWHV is not blocked on any callbacks when it gets removed as an observer (pending ack callbacks). If we mimicked what AURA does, it could go something like this: - The RWHV adds itself as an observer - when it's being detached from the compositor (or root window, i.e. layer tree) it runs pending ack/commit callbacks https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/compositor_observer.h (right): https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... content/browser/renderer_host/compositor_observer.h:2: // Use of this source code is governed by a BSD-style license that can be nit: (C) 2013 https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... content/browser/renderer_host/compositor_observer.h:6: #define CONTENT_BROWSER_RENDER_HOST_COMPOSITOR_OBSERVER_H_ nit: RENDER_HOST -> RENDERER_HOST https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_android.cc:1196: RunAckCallbacks(); You can actually remove both DelegatedRendererClient and TextureLayerClient as a base class now. Instead of returning the texture id here, you can add a call to set it on the layer in BuffersSwapped() (call texture_layer_->SetTextureId(texture_id_in_layer_) where we set it to drawable). https://codereview.chromium.org/26753005/diff/2001/content/public/browser/and... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/26753005/diff/2001/content/public/browser/and... content/public/browser/android/compositor.h:108: virtual bool HasObserver(CompositorObserver* observer) = 0; nit: newline before 'protected:'
+jdduke@ +yfriedman@ for OWNER PTAL. Android-side is up too. Please set me know if I missed anything. Thanks. https://codereview.chromium.org/26753005/diff/2001/content/browser/android/co... File content/browser/android/content_view_render_view.cc (right): https://codereview.chromium.org/26753005/diff/2001/content/browser/android/co... content/browser/android/content_view_render_view.cc:60: if (rwhva) { On 2013/10/10 17:48:03, sievers wrote: > This is the sort of logic I was afraid of :) > > And is it not broken if you navigate across domains in a given tab? Because in > that case a new RWHV gets created and attached to the ContentViewCore without us > noticing here. In other words, I don't think it's safe to use > content_view->GetRWHV(). > > You also have to make sure that a RWHV is not blocked on any callbacks when it > gets removed as an observer (pending ack callbacks). > > If we mimicked what AURA does, it could go something like this: > - The RWHV adds itself as an observer > - when it's being detached from the compositor (or root window, i.e. layer tree) > it runs pending ack/commit callbacks > Done. Good point. Please take a look at the new patch to make sure i did not make any more of these ownership mistakes. https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/compositor_observer.h (right): https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... content/browser/renderer_host/compositor_observer.h:2: // Use of this source code is governed by a BSD-style license that can be On 2013/10/10 17:48:03, sievers wrote: > nit: (C) 2013 Done. https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... content/browser/renderer_host/compositor_observer.h:6: #define CONTENT_BROWSER_RENDER_HOST_COMPOSITOR_OBSERVER_H_ On 2013/10/10 17:48:03, sievers wrote: > nit: RENDER_HOST -> RENDERER_HOST Done. https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/2001/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_android.cc:1196: RunAckCallbacks(); On 2013/10/10 17:48:03, sievers wrote: > You can actually remove both DelegatedRendererClient and TextureLayerClient as a > base class now. > > Instead of returning the texture id here, you can add a call to set it on the > layer in BuffersSwapped() (call > texture_layer_->SetTextureId(texture_id_in_layer_) where we set it to drawable). Done. https://codereview.chromium.org/26753005/diff/2001/content/public/browser/and... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/26753005/diff/2001/content/public/browser/and... content/public/browser/android/compositor.h:108: virtual bool HasObserver(CompositorObserver* observer) = 0; On 2013/10/10 17:48:03, sievers wrote: > nit: newline before 'protected:' Done. https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1629: empty lines to be removed.
Also I'll open another CL to address piman@'s comment. This one is plenty as is in my opinion.
https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1603: // Calling SetContentViewCore triggers the call set the ContentViewCore on Nit: "triggers the call to set" https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1608: } So, the desired effect here is simply to trigger RWHVA to add/remove itself as an observer to the Compositor? Why don't we do that directly right here (RWHVA::SetCompositor()), instead of relying on side effects of SetContentViewCore()? Or do we need to go through the attach/detach layers cycle when SetCompositor() is called? https://codereview.chromium.org/26753005/diff/19001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java (right): https://codereview.chromium.org/26753005/diff/19001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:178: contentView != null ? mCurrentContentView.getContentViewCore() : null; Do you mean "mCurrentContentView != null"?
See my comments - I'm wondering if we can simplify things a lot if we don't support moving a given view from one compositor to another (unlike on Aura, this might never be needed on Android and only seems to make things more complicated). https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1607: ->SetContentViewCore(this); Can we avoid piggy-backing on this other function? In the past we had a lot of problems with the complicated relationship between our objects (and unclear roles), and this is not intuitive. Setting the compositor should have nothing to do with setting the CVC-WC-RWHV relationship. See my other comment - if we pass the compositor into the constructor, we don't need to do this, right? https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.h:307: void SetCompositor(Compositor* compositor); We actually need a public content API for this to be usable (for example, something exposed in content_view_core.h). https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_render_view.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_render_view.cc:64: jobject obj, I don't think we need this here, see my comment in render_widget_host_view_android.cc What if we passed the compositor instance (or NULL for WebView) into the ContentViewCore constructor? I think it's easy enough for an app to let the browser compositor outlive all ContentViewCores. If not, then we could have a SetCompositor() (or WillDestroyCompositor() or something), that still doesn't have to be called every time visibility changes. https://codereview.chromium.org/26753005/diff/19001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:510: void RenderWidgetHostViewAndroid::DetachFromContentViewCore( Can we make the changes to this file really simple if we add/remove ourselves as compositor observers in Show/Hide (or WasShown/WasHidden)? https://codereview.chromium.org/26753005/diff/19001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:527: ack_callbacks_ = std::queue<base::Closure>(); Why this?
PTAL. New patch separates out the compositor from SetContentView. Now that we're working under the assumption of one persistent compositor, the logic is simpler. Is there anything I need to do to make this assumption a rule? (other than just commenting the code?) https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1603: // Calling SetContentViewCore triggers the call set the ContentViewCore on On 2013/10/16 23:12:26, jdduke wrote: > Nit: "triggers the call to set" Done. Remove. https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1607: ->SetContentViewCore(this); On 2013/10/16 23:34:23, sievers wrote: > Can we avoid piggy-backing on this other function? In the past we had a lot of > problems with the complicated relationship between our objects (and unclear > roles), and this is not intuitive. Setting the compositor should have nothing to > do with setting the CVC-WC-RWHV relationship. > > See my other comment - if we pass the compositor into the constructor, we don't > need to do this, right? Done. https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1608: } On 2013/10/16 23:12:26, jdduke wrote: > So, the desired effect here is simply to trigger RWHVA to add/remove itself as > an observer to the Compositor? Why don't we do that directly right here > (RWHVA::SetCompositor()), instead of relying on side effects of > SetContentViewCore()? Or do we need to go through the attach/detach layers cycle > when SetCompositor() is called? Done. https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1629: On 2013/10/16 22:42:35, powei wrote: > empty lines to be removed. Done. https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_core_impl.h:307: void SetCompositor(Compositor* compositor); On 2013/10/16 23:34:23, sievers wrote: > We actually need a public content API for this to be usable (for example, > something exposed in content_view_core.h). Done. https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... File content/browser/android/content_view_render_view.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/android/c... content/browser/android/content_view_render_view.cc:64: jobject obj, On 2013/10/16 23:34:23, sievers wrote: > I don't think we need this here, see my comment in > render_widget_host_view_android.cc > > What if we passed the compositor instance (or NULL for WebView) into the > ContentViewCore constructor? > > I think it's easy enough for an app to let the browser compositor outlive all > ContentViewCores. If not, then we could have a SetCompositor() (or > WillDestroyCompositor() or something), that still doesn't have to be called > every time visibility changes. Done. Removed. https://codereview.chromium.org/26753005/diff/19001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/19001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:510: void RenderWidgetHostViewAndroid::DetachFromContentViewCore( On 2013/10/16 23:34:23, sievers wrote: > Can we make the changes to this file really simple if we add/remove ourselves as > compositor observers in Show/Hide (or WasShown/WasHidden)? Done. WasHidden is not always guarranteed to run before the destructor, so a remove is still necessary in the desctructor. https://codereview.chromium.org/26753005/diff/19001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:527: ack_callbacks_ = std::queue<base::Closure>(); On 2013/10/16 23:34:23, sievers wrote: > Why this? Done. Removed. https://codereview.chromium.org/26753005/diff/19001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java (right): https://codereview.chromium.org/26753005/diff/19001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:178: contentView != null ? mCurrentContentView.getContentViewCore() : null; On 2013/10/16 23:12:26, jdduke wrote: > Do you mean "mCurrentContentView != null"? Done. Removed.
This looks okay to me, I'll let sievers@ do the final review. https://codereview.chromium.org/26753005/diff/60001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/26753005/diff/60001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:235: nit: unnecessary newline https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:158: if (compositor_) { nit: no {}. Likewise in many other clauses with only one line in this patch. https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:205: if (compositor_ && !compositor_->HasObserver(this)) { Please make the HasObserver check an early return inside AddObserver instead.
Just some random nits/suggestions. Leaving the final review to sievers@ https://chromiumcodereview.appspot.com/26753005/diff/60001/content/browser/re... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/26753005/diff/60001/content/browser/re... content/browser/renderer_host/render_widget_host_view_android.cc:1269: compositor_->RemoveObserver(this); Is it safe to call RemoveObserver if it isn't added (short of putting the HasObserver check in Compositor::RemoveObserver)? I haven't looked at ObserverList. Should you add a HasObserver call here? https://chromiumcodereview.appspot.com/26753005/diff/60001/content/browser/re... content/browser/renderer_host/render_widget_host_view_android.cc:1274: if (are_layers_attached_ && compositor_ && !compositor_->HasObserver(this)) { Can remove HasObserver if you put it into AddObserver. https://chromiumcodereview.appspot.com/26753005/diff/60001/content/browser/we... File content/browser/web_contents/web_contents_view_android.cc (right): https://chromiumcodereview.appspot.com/26753005/diff/60001/content/browser/we... content/browser/web_contents/web_contents_view_android.cc:174: view->SetCompositor(compositor_); Was there one other code path to build a RWHVA?
This is ready for another look after our offline discussion a few weeks back. Android side has been updated as well. Thanks. https://codereview.chromium.org/26753005/diff/60001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/26753005/diff/60001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:235: On 2013/10/19 03:43:27, aelias wrote: > nit: unnecessary newline Done. https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:158: if (compositor_) { On 2013/10/19 03:43:27, aelias wrote: > nit: no {}. Likewise in many other clauses with only one line in this patch. Done. https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:205: if (compositor_ && !compositor_->HasObserver(this)) { On 2013/10/19 03:43:27, aelias wrote: > Please make the HasObserver check an early return inside AddObserver instead. Done. https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:1269: compositor_->RemoveObserver(this); On 2013/10/21 15:26:08, David Trainor wrote: > Is it safe to call RemoveObserver if it isn't added (short of putting the > HasObserver check in Compositor::RemoveObserver)? I haven't looked at > ObserverList. Should you add a HasObserver call here? It is safe. observer_list has an early out for removing non-existent observer. https://codereview.chromium.org/26753005/diff/60001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:1274: if (are_layers_attached_ && compositor_ && !compositor_->HasObserver(this)) { On 2013/10/21 15:26:08, David Trainor wrote: > Can remove HasObserver if you put it into AddObserver. Done. https://codereview.chromium.org/26753005/diff/60001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/26753005/diff/60001/content/browser/web_conte... content/browser/web_contents/web_contents_view_android.cc:174: view->SetCompositor(compositor_); On 2013/10/21 15:26:08, David Trainor wrote: > Was there one other code path to build a RWHVA? No, but there is a path where RWHVA is created before the compositor (or now the CompositorProvider) is set for WebContentsViewAndroid.
https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:120: compositor_provider_(NULL) { nit: init to |compositor_provider| here https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1280: void RenderWidgetHostViewAndroid::UnobserveCompositor( No need for an extra function. And then it avoids the awkward name too :) https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1417: return new RenderWidgetHostViewAndroid(rwhi, NULL, NULL); Hmm what's the point if we can't create it with the CompositorProvider here? https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:207: virtual void ChangeCompositor(Compositor* new_compositor) OVERRIDE; nit: maybe just 'SetCompositor'? https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:223: void SetCompositorProvider(CompositorProvider* compositor_provider); That's weird. Isn't the whole point of the provider interface to have an instance of something that outlives the RWHVA and never changes? And it gets passed into the RWHVA constructor.
adding OWNERS +boliu@ for android_webview +jochen@ for content/browser/web_contents/ +jam@ for content/content_browser.gypi +tedchoc@ for content/public/android, content/public/browser/android, and content/shell/android PTAL. Thanks.
https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:120: compositor_provider_(NULL) { On 2013/11/08 22:14:04, sievers wrote: > nit: init to |compositor_provider| here Done. https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1280: void RenderWidgetHostViewAndroid::UnobserveCompositor( On 2013/11/08 22:14:04, sievers wrote: > No need for an extra function. And then it avoids the awkward name too :) Done. https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1417: return new RenderWidgetHostViewAndroid(rwhi, NULL, NULL); On 2013/11/08 22:14:04, sievers wrote: > Hmm what's the point if we can't create it with the CompositorProvider here? I don't think this is ever called. Maybe a NOTREACH? https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:207: virtual void ChangeCompositor(Compositor* new_compositor) OVERRIDE; On 2013/11/08 22:14:04, sievers wrote: > nit: maybe just 'SetCompositor'? Done. https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:223: void SetCompositorProvider(CompositorProvider* compositor_provider); On 2013/11/08 22:14:04, sievers wrote: > That's weird. Isn't the whole point of the provider interface to have an > instance of something that outlives the RWHVA and never changes? And it gets > passed into the RWHVA constructor. I agree this isn't great. Maybe you can suggest a better way? There's a path that creates a WebContentViewAndroid and calls WCVA::CreateViewForWidget before WCVA::SetContentViewCore is called. So RWHVA is created with a null CompositorProvider. This method exists so that we can pass the CompositorProvider (from ContentViewCore) to RWHVA after it's constructed.
On 2013/11/08 22:28:52, powei wrote: > adding OWNERS > > +boliu@ for android_webview > +jochen@ for content/browser/web_contents/ > +jam@ for content/content_browser.gypi > +tedchoc@ for content/public/android, content/public/browser/android, and > content/shell/android > > PTAL. Thanks. lgtm
for the new single-method interfaces, are they going to have more methods in the future? if no, or you don't know for certain, then use callbacks instead per the content api convention
aw lgtm
On 2013/11/08 22:51:15, powei wrote: > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_android.cc:120: > compositor_provider_(NULL) { > On 2013/11/08 22:14:04, sievers wrote: > > nit: init to |compositor_provider| here > > Done. > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_android.cc:1280: void > RenderWidgetHostViewAndroid::UnobserveCompositor( > On 2013/11/08 22:14:04, sievers wrote: > > No need for an extra function. And then it avoids the awkward name too :) > > Done. > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_android.cc:1417: return > new RenderWidgetHostViewAndroid(rwhi, NULL, NULL); > On 2013/11/08 22:14:04, sievers wrote: > > Hmm what's the point if we can't create it with the CompositorProvider here? > > I don't think this is ever called. Maybe a NOTREACH? > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_android.h (right): > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_android.h:207: virtual > void ChangeCompositor(Compositor* new_compositor) OVERRIDE; > On 2013/11/08 22:14:04, sievers wrote: > > nit: maybe just 'SetCompositor'? > > Done. > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_android.h:223: void > SetCompositorProvider(CompositorProvider* compositor_provider); > On 2013/11/08 22:14:04, sievers wrote: > > That's weird. Isn't the whole point of the provider interface to have an > > instance of something that outlives the RWHVA and never changes? And it gets > > passed into the RWHVA constructor. > > I agree this isn't great. Maybe you can suggest a better way? There's a path > that creates a WebContentViewAndroid and calls WCVA::CreateViewForWidget before > WCVA::SetContentViewCore is called. Is it possible to create the WebContentsViewAndroid with the CompositorProvider then? Maybe it's not even necessary to plumb this native pointer through so many Java classes. So RWHVA is created with a null > CompositorProvider. This method exists so that we can pass the > CompositorProvider (from ContentViewCore) to RWHVA after it's constructed.
On 2013/11/11 18:32:02, sievers wrote: > On 2013/11/08 22:51:15, powei wrote: > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_android.cc:120: > > compositor_provider_(NULL) { > > On 2013/11/08 22:14:04, sievers wrote: > > > nit: init to |compositor_provider| here > > > > Done. > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_android.cc:1280: void > > RenderWidgetHostViewAndroid::UnobserveCompositor( > > On 2013/11/08 22:14:04, sievers wrote: > > > No need for an extra function. And then it avoids the awkward name too :) > > > > Done. > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_android.cc:1417: return > > new RenderWidgetHostViewAndroid(rwhi, NULL, NULL); > > On 2013/11/08 22:14:04, sievers wrote: > > > Hmm what's the point if we can't create it with the CompositorProvider here? > > > > I don't think this is ever called. Maybe a NOTREACH? > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > File content/browser/renderer_host/render_widget_host_view_android.h (right): > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_android.h:207: virtual > > void ChangeCompositor(Compositor* new_compositor) OVERRIDE; > > On 2013/11/08 22:14:04, sievers wrote: > > > nit: maybe just 'SetCompositor'? > > > > Done. > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > content/browser/renderer_host/render_widget_host_view_android.h:223: void > > SetCompositorProvider(CompositorProvider* compositor_provider); > > On 2013/11/08 22:14:04, sievers wrote: > > > That's weird. Isn't the whole point of the provider interface to have an > > > instance of something that outlives the RWHVA and never changes? And it gets > > > passed into the RWHVA constructor. > > > > I agree this isn't great. Maybe you can suggest a better way? There's a path > > that creates a WebContentViewAndroid and calls WCVA::CreateViewForWidget > before > > WCVA::SetContentViewCore is called. > > Is it possible to create the WebContentsViewAndroid with the CompositorProvider > then? > Maybe it's not even necessary to plumb this native pointer through so many Java > classes. It requires plumbing through all the CreateWebContentsView for WebContentView[Platform] and also WebContentsImpl. Is this ok for something specific to Android? > So RWHVA is created with a null > > CompositorProvider. This method exists so that we can pass the > > CompositorProvider (from ContentViewCore) to RWHVA after it's constructed.
On 2013/11/09 01:15:12, jam wrote: > for the new single-method interfaces, are they going to have more methods in the > future? if no, or you don't know for certain, then use callbacks instead per the > content api convention Will do. Thanks for letting me know.
On 2013/11/11 18:55:41, powei wrote: > On 2013/11/11 18:32:02, sievers wrote: > > On 2013/11/08 22:51:15, powei wrote: > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > File content/browser/renderer_host/render_widget_host_view_android.cc > (right): > > > > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > content/browser/renderer_host/render_widget_host_view_android.cc:120: > > > compositor_provider_(NULL) { > > > On 2013/11/08 22:14:04, sievers wrote: > > > > nit: init to |compositor_provider| here > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > content/browser/renderer_host/render_widget_host_view_android.cc:1280: void > > > RenderWidgetHostViewAndroid::UnobserveCompositor( > > > On 2013/11/08 22:14:04, sievers wrote: > > > > No need for an extra function. And then it avoids the awkward name too :) > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > content/browser/renderer_host/render_widget_host_view_android.cc:1417: > return > > > new RenderWidgetHostViewAndroid(rwhi, NULL, NULL); > > > On 2013/11/08 22:14:04, sievers wrote: > > > > Hmm what's the point if we can't create it with the CompositorProvider > here? > > > > > > I don't think this is ever called. Maybe a NOTREACH? > > > > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > File content/browser/renderer_host/render_widget_host_view_android.h > (right): > > > > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > content/browser/renderer_host/render_widget_host_view_android.h:207: virtual > > > void ChangeCompositor(Compositor* new_compositor) OVERRIDE; > > > On 2013/11/08 22:14:04, sievers wrote: > > > > nit: maybe just 'SetCompositor'? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/26753005/diff/340001/content/browser/renderer... > > > content/browser/renderer_host/render_widget_host_view_android.h:223: void > > > SetCompositorProvider(CompositorProvider* compositor_provider); > > > On 2013/11/08 22:14:04, sievers wrote: > > > > That's weird. Isn't the whole point of the provider interface to have an > > > > instance of something that outlives the RWHVA and never changes? And it > gets > > > > passed into the RWHVA constructor. > > > > > > I agree this isn't great. Maybe you can suggest a better way? There's a > path > > > that creates a WebContentViewAndroid and calls WCVA::CreateViewForWidget > > before > > > WCVA::SetContentViewCore is called. > > > > Is it possible to create the WebContentsViewAndroid with the > CompositorProvider > > then? > > Maybe it's not even necessary to plumb this native pointer through so many > Java > > classes. > > It requires plumbing through all the CreateWebContentsView for > WebContentView[Platform] and also WebContentsImpl. Is this ok for something > specific to Android? > Does this help? // Allows an embedder to return their own WebContentsViewPort implementation. // Return NULL to let the default one for the platform be created. Otherwise // |render_view_host_delegate_view| also needs to be provided, and it is // owned by the embedder. virtual WebContentsViewPort* OverrideCreateWebContentsView( WebContents* web_contents, RenderViewHostDelegateView** render_view_host_delegate_view);
Ready for another look. This went through another redesign after some offline discussion. I'm specifically requesting OWNERS stamps from dtrainor@ : chrome/android/testshell sievers@ : content/browser/renderer_host tedchoc@ : content/public/android, content/public/browser/android, content/shell/android/, ui/base/android Everyone's welcomed to comment as usual of course. Thank you!
Nice, thanks for doing this! https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... content/browser/renderer_host/compositor_impl_android.h:124: ui::WindowAndroid* window_android_; nit: maybe call it |root_window_| or so? https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1293: DCHECK(host_); nit: Can this just become a member variable |using_synchronous_compositor_| that gets initialized in the constructor? https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1307: content_view_core_->GetWindowAndroid()->RemoveObserver(this); RunAckCallbacks() here. And being removed as an observer could actually be implied. https://codereview.chromium.org/26753005/diff/540001/content/public/browser/a... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/26753005/diff/540001/content/public/browser/a... content/public/browser/android/compositor.h:44: ui::WindowAndroid* window_android); nit: |root_window|? Can you also use the common typedef 'NativeWindow' from native_widget_types.h here and everywhere else in content/ just so that it's more obvious what it is? Can you also document that this needs to outlive the compositor? https://codereview.chromium.org/26753005/diff/540001/ui/base/android/window_a... File ui/base/android/window_android_observer.h (right): https://codereview.chromium.org/26753005/diff/540001/ui/base/android/window_a... ui/base/android/window_android_observer.h:19: virtual void OnAttachCompositor(content::Compositor* compositor) = 0; Can you remove the compositor argument from all functions? It's actually unused and unnecessary here.
comments addressed. PTAL. Thanks. +cc: boliu@, jam@ accidentally removed them in updating the reviewer list. https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... content/browser/renderer_host/compositor_impl_android.h:124: ui::WindowAndroid* window_android_; On 2013/11/14 22:01:01, sievers wrote: > nit: maybe call it |root_window_| or so? Done. https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1293: DCHECK(host_); On 2013/11/14 22:01:01, sievers wrote: > nit: Can this just become a member variable |using_synchronous_compositor_| that > gets initialized in the constructor? Done. https://codereview.chromium.org/26753005/diff/540001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1307: content_view_core_->GetWindowAndroid()->RemoveObserver(this); On 2013/11/14 22:01:01, sievers wrote: > RunAckCallbacks() here. > And being removed as an observer could actually be implied. Done. https://codereview.chromium.org/26753005/diff/540001/content/public/browser/a... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/26753005/diff/540001/content/public/browser/a... content/public/browser/android/compositor.h:44: ui::WindowAndroid* window_android); On 2013/11/14 22:01:01, sievers wrote: > nit: |root_window|? Can you also use the common typedef 'NativeWindow' from > native_widget_types.h here and everywhere else in content/ just so that it's > more obvious what it is? > > Can you also document that this needs to outlive the compositor? Done. https://codereview.chromium.org/26753005/diff/540001/ui/base/android/window_a... File ui/base/android/window_android_observer.h (right): https://codereview.chromium.org/26753005/diff/540001/ui/base/android/window_a... ui/base/android/window_android_observer.h:19: virtual void OnAttachCompositor(content::Compositor* compositor) = 0; On 2013/11/14 22:01:01, sievers wrote: > Can you remove the compositor argument from all functions? It's actually unused > and unnecessary here. Done.
https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:50: #include "ui/base/android/window_android_observer.h" nit: already included in the header, since RWHV implements this https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:270: bool UsingSynchronousCompositor() const; remove this https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... File ui/base/android/window_android.h (right): https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... ui/base/android/window_android.h:17: class Compositor; remove this https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... File ui/base/android/window_android_observer.h (right): https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... ui/base/android/window_android_observer.h:11: class Compositor; remove this https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... ui/base/android/window_android_observer.h:19: virtual void OnAttachCompositor() = 0; I don't see anyone call OnAttach/Detach. Can the compositor maybe just call through the window on construction and destruction respectively? Also erase all listeners in the window once it detaches.
new patchset up. thanks. https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:50: #include "ui/base/android/window_android_observer.h" On 2013/11/15 20:15:20, sievers wrote: > nit: already included in the header, since RWHV implements this Done. https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/26753005/diff/690001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:270: bool UsingSynchronousCompositor() const; On 2013/11/15 20:15:20, sievers wrote: > remove this Done. https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... File ui/base/android/window_android.h (right): https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... ui/base/android/window_android.h:17: class Compositor; On 2013/11/15 20:15:20, sievers wrote: > remove this Done. https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... File ui/base/android/window_android_observer.h (right): https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... ui/base/android/window_android_observer.h:11: class Compositor; On 2013/11/15 20:15:20, sievers wrote: > remove this Done. https://codereview.chromium.org/26753005/diff/690001/ui/base/android/window_a... ui/base/android/window_android_observer.h:19: virtual void OnAttachCompositor() = 0; On 2013/11/15 20:15:20, sievers wrote: > I don't see anyone call OnAttach/Detach. > > Can the compositor maybe just call through the window on construction and > destruction respectively? > > Also erase all listeners in the window once it detaches. Done.
LGTM. thanks! https://codereview.chromium.org/26753005/diff/810001/content/browser/android/... File content/browser/android/content_view_render_view.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/android/... content/browser/android/content_view_render_view.cc:46: static jint Init(JNIEnv* env, jobject obj, jlong native_root_window) { nit: we already have |root_window| https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/compositor_impl_android.cc:492: DCHECK(root_window_); nit: we already DCHECK() in the constructor
https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1260: if (content_view_core_ && !using_synchronous_compositor_) Why does value of new content_view_core matter here? Wouldn't you want to remove observer on the old cvc either way? (Just wondering...I'm not familiar with this code. https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:343: bool using_synchronous_compositor_; Can this be const?
lgtm w/ some nits https://codereview.chromium.org/26753005/diff/810001/chrome/android/testshell... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java (right): https://codereview.chromium.org/26753005/diff/810001/chrome/android/testshell... chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:57: protected void onReadyToRender() { -4 indent https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:202: content_view_core_->GetWindowAndroid()->AddObserver(this); -2 indent. In the event content_view_core_ wasn't set when WasShown was called, are we now hosed? https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:216: content_view_core_->GetWindowAndroid()->RemoveObserver(this); -2 indent https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1259: if (content_view_core == NULL) { Shouldn't need the == NULL https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1260: if (content_view_core_ && !using_synchronous_compositor_) On 2013/11/16 00:42:56, boliu wrote: > Why does value of new content_view_core matter here? Wouldn't you want to remove > observer on the old cvc either way? (Just wondering...I'm not familiar with this > code. Yeah, maybe it should be content_view_core != content_view_core_
Addressed the comments. Thanks for the reviews! Will try to commit soon. https://codereview.chromium.org/26753005/diff/810001/chrome/android/testshell... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java (right): https://codereview.chromium.org/26753005/diff/810001/chrome/android/testshell... chrome/android/testshell/java/src/org/chromium/chrome/testshell/TabManager.java:57: protected void onReadyToRender() { On 2013/11/16 01:04:08, Ted C wrote: > -4 indent Done. https://codereview.chromium.org/26753005/diff/810001/content/browser/android/... File content/browser/android/content_view_render_view.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/android/... content/browser/android/content_view_render_view.cc:46: static jint Init(JNIEnv* env, jobject obj, jlong native_root_window) { On 2013/11/15 22:56:19, sievers wrote: > nit: we already have |root_window| can you elaborate? is this about the name |native_root_window|? https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/compositor_impl_android.cc:492: DCHECK(root_window_); On 2013/11/15 22:56:19, sievers wrote: > nit: we already DCHECK() in the constructor Done. https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:202: content_view_core_->GetWindowAndroid()->AddObserver(this); On 2013/11/16 01:04:08, Ted C wrote: > -2 indent. > > In the event content_view_core_ wasn't set when WasShown was called, are we now > hosed? Done for indent. I don't think we're necessarily hosed if there is no |content_view_core_|, just that no frame will rendered, which can't happen anyway without a ContentViewCore. Just being conservative since I'm not sure when WasShown can be called. https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:216: content_view_core_->GetWindowAndroid()->RemoveObserver(this); On 2013/11/16 01:04:08, Ted C wrote: > -2 indent Done. https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1259: if (content_view_core == NULL) { On 2013/11/16 01:04:08, Ted C wrote: > Shouldn't need the == NULL Done. Followed suggestion below. https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1260: if (content_view_core_ && !using_synchronous_compositor_) On 2013/11/16 01:04:08, Ted C wrote: > On 2013/11/16 00:42:56, boliu wrote: > > Why does value of new content_view_core matter here? Wouldn't you want to > remove > > observer on the old cvc either way? (Just wondering...I'm not familiar with > this > > code. > > Yeah, maybe it should be content_view_core != content_view_core_ Done. https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/26753005/diff/810001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:343: bool using_synchronous_compositor_; On 2013/11/16 00:42:56, boliu wrote: > Can this be const? Done.
lgtm
https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... content/browser/renderer_host/render_widget_host_view_android.cc:1258: if (content_view_core != content_view_core_ && content_view_core_ && This still doesn't seem to make sense. If content_view_core == content_view_core_, then RemoveObserver is skipped, but AddObserver below is not, which means the observer wil be added twice? How much of this code can be skipped if new_cvc == old_cvc? Maybe should just early out in that case?
https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... content/browser/renderer_host/render_widget_host_view_android.cc:1258: if (content_view_core != content_view_core_ && content_view_core_ && On 2013/11/18 19:39:25, boliu wrote: > This still doesn't seem to make sense. If content_view_core == > content_view_core_, then RemoveObserver is skipped, but AddObserver below is > not, which means the observer wil be added twice? > > How much of this code can be skipped if new_cvc == old_cvc? Maybe should just > early out in that case? I'm not sure about the other calls so I'm hesitant to early out here, but AddObserver will not add an already observing observer.
https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... content/browser/renderer_host/render_widget_host_view_android.cc:1258: if (content_view_core != content_view_core_ && content_view_core_ && On 2013/11/18 19:44:02, powei wrote: > On 2013/11/18 19:39:25, boliu wrote: > > This still doesn't seem to make sense. If content_view_core == > > content_view_core_, then RemoveObserver is skipped, but AddObserver below is > > not, which means the observer wil be added twice? > > > > How much of this code can be skipped if new_cvc == old_cvc? Maybe should just > > early out in that case? > > I'm not sure about the other calls so I'm hesitant to early out here, but > AddObserver will not add an already observing observer. > Maybe you'd be ok with just: if (content_view_core_ && !using_synchronous_compositor)
On 2013/11/18 19:47:13, powei wrote: > https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... > content/browser/renderer_host/render_widget_host_view_android.cc:1258: if > (content_view_core != content_view_core_ && content_view_core_ && > On 2013/11/18 19:44:02, powei wrote: > > On 2013/11/18 19:39:25, boliu wrote: > > > This still doesn't seem to make sense. If content_view_core == > > > content_view_core_, then RemoveObserver is skipped, but AddObserver below is > > > not, which means the observer wil be added twice? > > > > > > How much of this code can be skipped if new_cvc == old_cvc? Maybe should > just > > > early out in that case? > > > > I'm not sure about the other calls so I'm hesitant to early out here, but > > AddObserver will not add an already observing observer. > > > > Maybe you'd be ok with just: > if (content_view_core_ && !using_synchronous_compositor) Yep. That looks good.
submitting https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/26753005/diff/1110001/content/browser/... content/browser/renderer_host/render_widget_host_view_android.cc:1258: if (content_view_core != content_view_core_ && content_view_core_ && On 2013/11/18 19:47:14, powei wrote: > On 2013/11/18 19:44:02, powei wrote: > > On 2013/11/18 19:39:25, boliu wrote: > > > This still doesn't seem to make sense. If content_view_core == > > > content_view_core_, then RemoveObserver is skipped, but AddObserver below is > > > not, which means the observer wil be added twice? > > > > > > How much of this code can be skipped if new_cvc == old_cvc? Maybe should > just > > > early out in that case? > > > > I'm not sure about the other calls so I'm hesitant to early out here, but > > AddObserver will not add an already observing observer. > > > > Maybe you'd be ok with just: > if (content_view_core_ && !using_synchronous_compositor) > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/26753005/1220001
Failed to apply patch for content/browser/android/content_view_render_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/android/content_view_render_view.cc
Hunk #2 FAILED at 43.
1 out of 3 hunks FAILED -- saving rejects to file
content/browser/android/content_view_render_view.cc.rej
Patch: content/browser/android/content_view_render_view.cc
Index: content/browser/android/content_view_render_view.cc
diff --git a/content/browser/android/content_view_render_view.cc
b/content/browser/android/content_view_render_view.cc
index
05f248272353e63711f8fc6f1dbef3e42dd5c9cf..d5b636bfbc3d7f8a4a3d8d32476706fa19e246c9
100644
--- a/content/browser/android/content_view_render_view.cc
+++ b/content/browser/android/content_view_render_view.cc
@@ -31,8 +31,11 @@ bool
ContentViewRenderView::RegisterContentViewRenderView(JNIEnv* env) {
return RegisterNativesImpl(env);
}
-ContentViewRenderView::ContentViewRenderView(JNIEnv* env, jobject obj)
- : buffers_swapped_during_composite_(false) {
+ContentViewRenderView::ContentViewRenderView(JNIEnv* env,
+ jobject obj,
+ gfx::NativeWindow root_window)
+ : buffers_swapped_during_composite_(false),
+ root_window_(root_window) {
java_obj_.Reset(env, obj);
}
@@ -40,9 +43,11 @@ ContentViewRenderView::~ContentViewRenderView() {
}
// static
-static jint Init(JNIEnv* env, jobject obj) {
+static jint Init(JNIEnv* env, jobject obj, jlong native_root_window) {
+ gfx::NativeWindow root_window =
+ reinterpret_cast<gfx::NativeWindow>(native_root_window);
ContentViewRenderView* content_view_render_view =
- new ContentViewRenderView(env, obj);
+ new ContentViewRenderView(env, obj, root_window);
return reinterpret_cast<jint>(content_view_render_view);
}
@@ -110,7 +115,7 @@ void ContentViewRenderView::OnSwapBuffersCompleted() {
void ContentViewRenderView::InitCompositor() {
if (!compositor_)
- compositor_.reset(Compositor::Create(this));
+ compositor_.reset(Compositor::Create(this, root_window_));
}
} // namespace content
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/26753005/1370001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/26753005/1900003
Message was sent while issue was closed.
Change committed as 236598 |
