Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1943963003:
WIP Handle AwContents needing multiple live functors. (Closed)
|
Created:
4 years, 7 months ago by Tobias Sargeant Modified:
4 years, 7 months ago Reviewers:
boliu CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWIP Handle AwContents needing multiple live functors.
AwContents now needs to keep a functor per view. Either functor may be
asked to draw its current frame at any time. New frames produced by
the compositor go to the current RTM, and the BVR is interested in
the geometry of the current RTM. Trim memory signal must go to all
RTMs, and all RTMs must be able to hand back resources to the right
compositor at any time, and not be forced to do so when a different
RTM becomes current.
BUG=597167
Committed: https://crrev.com/6f2a192ab2b7fe5bb6bdb7f58e0d0015816ba121
Cr-Commit-Position: refs/heads/master@{#394387}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 30
Patch Set 4 : two functors may draw to the same GLSurfaceView during tests #Patch Set 5 : native code comments; inform old producer when taking ownership of a consumer. #Patch Set 6 : #Patch Set 7 : #
Total comments: 14
Patch Set 8 : assert only non current consumers can be removed #Patch Set 9 : remainder of comments from PS3 #Patch Set 10 : PS7 comments #
Total comments: 9
Patch Set 11 : remove AwGLFunctor lifetime objects from AwContents destroy runnable #Patch Set 12 : #Patch Set 13 : comments addressed; unittests TODO #
Total comments: 4
Patch Set 14 : Testing framework changes to support testing multiple RenderThreadManager instances. #
Total comments: 33
Patch Set 15 : #
Total comments: 15
Patch Set 16 : Comments from PS15 #Patch Set 17 : Destroy HardwareRenderer when detached and not awaiting a callback. #
Total comments: 14
Patch Set 18 : Comments from PS17 #Messages
Total messages: 37 (7 generated)
Description was changed from ========== WIP Handle AwContents needing multiple live functors. AwContents now needs to keep a functor per view. Either functor may be asked to draw its current frame at any time. New frames produced by the compositor go to the current RTM, and the BVR is interested in the geometry of the current RTM. Trim memory signal must go to all RTMs, and all RTMs must be able to hand back resources to the right compositor at any time, and not be forced to do so when they are no longer the current RTM. BUG=597167 ========== to ========== WIP Handle AwContents needing multiple live functors. AwContents now needs to keep a functor per view. Either functor may be asked to draw its current frame at any time. New frames produced by the compositor go to the current RTM, and the BVR is interested in the geometry of the current RTM. Trim memory signal must go to all RTMs, and all RTMs must be able to hand back resources to the right compositor at any time, and not be forced to do so when a different RTM becomes current. BUG=597167 ==========
Patchset #4 (id:60001) has been deleted
tobiasjs@chromium.org changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:96: compositor_frame_consumers_(), not necessary https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:119: void BrowserViewRenderer::SetCompositorFrameConsumer( Feels like "AddToSet" and "MakeCurrent" should be two different calls. This feels kinda sketchy implicitly doing both. If we always do them at the same time, then at least rename this method to say it does those two things. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:124: current_compositor_frame_consumer_ = compositor_frame_consumer; should update parent draw constraints if the current consumer changed https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:128: compositor_frame_consumers_.insert(current_compositor_frame_consumer_); dcheck for duplicates? https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:270: current_compositor_frame_consumer_->GetParentDrawConstraintsOnUI(); what if compositor_frame_consumer != current? https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:276: DCHECK(compositor_frame_consumers_.count(compositor_frame_consumer) == 1); == 1 part is redundant https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:279: current_compositor_frame_consumer_ = nullptr; SetCompositorFrameConsumer(nullptr), maybe that one should be renamed to the "update current" one https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:280: LOG(WARNING) << "XXX Setting current_compositor_frame_consumer_ to null"; So yeah, what's responsible for setting the current consumer now? https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:456: if (compositor_frame_consumer_) { make sure null is never inserted instead of having a check here https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:263: private AwGLFunctor mFullScreenFunctor; // Only non-null when in fullscreen mode. style: two spaces before // https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:913: // Explicitly destroy the full screen functor now. not really "destroying" functor https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1020: private void makeFunctorCurrent(AwGLFunctor functor) { only one caller, so don't need a separate method https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:96: public void deleteHardwareRenderer() { maybe this should be renamed something about trim, since that's the only caller now
unit tests for the native changes? then you could do the java changes in a separate patch, or keep them together, should be fine either way
https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:119: void BrowserViewRenderer::SetCompositorFrameConsumer( On 2016/05/04 16:58:52, boliu wrote: > Feels like "AddToSet" and "MakeCurrent" should be two different calls. This > feels kinda sketchy implicitly doing both. > > If we always do them at the same time, then at least rename this method to say > it does those two things. I thought about breaking it into two methods, but I think there's always going to be impedance mismatch between this and java AwContents that manages the current functor. All AwContents wants to do is set the current functor. We need to keep a set of consumers associated with the BVR, so that we can clean them up even if they are not current. But other than that, and the implied constraint that you can't make a functor current until it has been removed from any other BVR it is associated with, the set is a private implementation detail. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:124: current_compositor_frame_consumer_ = compositor_frame_consumer; On 2016/05/04 16:58:52, boliu wrote: > should update parent draw constraints if the current consumer changed Done. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:128: compositor_frame_consumers_.insert(current_compositor_frame_consumer_); On 2016/05/04 16:58:52, boliu wrote: > dcheck for duplicates? Insertion of the same value may happen if (for example) when we exit full screen mode, and the initial functor is set as the current functor again. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:270: current_compositor_frame_consumer_->GetParentDrawConstraintsOnUI(); On 2016/05/04 16:58:52, boliu wrote: > what if compositor_frame_consumer != current? Should be NOP in that case. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:276: DCHECK(compositor_frame_consumers_.count(compositor_frame_consumer) == 1); On 2016/05/04 16:58:52, boliu wrote: > == 1 part is redundant Removed.
https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:280: LOG(WARNING) << "XXX Setting current_compositor_frame_consumer_ to null"; On 2016/05/04 16:58:52, boliu wrote: > So yeah, what's responsible for setting the current consumer now? Didn't really resolve this one https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:128: if (current_compositor_frame_consumer_->GetCompositorFrameProducer()) { this can't happen? we destroy the previous BVR before the new BVR is attached in the pop up flow. just DCHECK somewhere that this can't happen https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:272: DCHECK(compositor_frame_consumer); DCHECK compositor frame is in set https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:273: if (compositor_frame_consumer == current_compositor_frame_consumer_) { nit: we usually prefer if (foo) return; since that avoids indenting the whole method https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... File android_webview/browser/compositor_frame_consumer.h (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/compositor_frame_consumer.h:38: virtual CompositorFrameProducer* GetCompositorFrameProducer() = 0; make sure this is removed when the caller is removed https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... File android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java:185: long viewContext = 0; you can follow the same pattern as above, final with no assignment https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java:212: DrawGL.drawGL(mDrawGL, viewContext, width, height, mCommittedScrollX, so mViewContext wasn't thread safe is mCommittedScrollX/Y safe? Looks like it's only used in this method, so should just be converted to a local var
https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:96: compositor_frame_consumers_(), On 2016/05/04 16:58:52, boliu wrote: > not necessary Removed. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:279: current_compositor_frame_consumer_ = nullptr; On 2016/05/04 16:58:52, boliu wrote: > SetCompositorFrameConsumer(nullptr), maybe that one should be renamed to the > "update current" one Removed the need for this. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:280: LOG(WARNING) << "XXX Setting current_compositor_frame_consumer_ to null"; On 2016/05/05 14:09:34, boliu wrote: > On 2016/05/04 16:58:52, boliu wrote: > > So yeah, what's responsible for setting the current consumer now? > > Didn't really resolve this one I think it's actually reasonable to enforce that you can't remove the current consumer. When switching back from full screen, we set the view functor to current, and then destroy the RTM (which causes the BVR to remove it). That's the only way that this case occurs. Otherwise, we're destroying the whole BVR, in which case we can just clear the current consumer before removing all of them. https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:456: if (compositor_frame_consumer_) { On 2016/05/04 16:58:52, boliu wrote: > make sure null is never inserted instead of having a check here Done. https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:263: private AwGLFunctor mFullScreenFunctor; // Only non-null when in fullscreen mode. On 2016/05/04 16:58:52, boliu wrote: > style: two spaces before // Done. https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:913: // Explicitly destroy the full screen functor now. On 2016/05/04 16:58:52, boliu wrote: > not really "destroying" functor Reworded. https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1020: private void makeFunctorCurrent(AwGLFunctor functor) { On 2016/05/04 16:58:52, boliu wrote: > only one caller, so don't need a separate method Done. https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:96: public void deleteHardwareRenderer() { On 2016/05/04 16:58:52, boliu wrote: > maybe this should be renamed something about trim, since that's the only caller > now Maybe this should be done in BVR::TrimMemory for all consumers?
https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:128: if (current_compositor_frame_consumer_->GetCompositorFrameProducer()) { On 2016/05/05 14:09:34, boliu wrote: > this can't happen? we destroy the previous BVR before the new BVR is attached in > the pop up flow. just DCHECK somewhere that this can't happen You're right. I've cleaned this up. https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:272: DCHECK(compositor_frame_consumer); On 2016/05/05 14:09:34, boliu wrote: > DCHECK compositor frame is in set Don't need to, because being equal to the current consumer implies that. https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:273: if (compositor_frame_consumer == current_compositor_frame_consumer_) { On 2016/05/05 14:09:34, boliu wrote: > nit: we usually prefer > > if (foo) > return; > > since that avoids indenting the whole method Done. https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... File android_webview/browser/compositor_frame_consumer.h (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/compositor_frame_consumer.h:38: virtual CompositorFrameProducer* GetCompositorFrameProducer() = 0; On 2016/05/05 14:09:34, boliu wrote: > make sure this is removed when the caller is removed Done. https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... File android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java:185: long viewContext = 0; On 2016/05/05 14:09:35, boliu wrote: > you can follow the same pattern as above, final with no assignment Done. https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java:212: DrawGL.drawGL(mDrawGL, viewContext, width, height, mCommittedScrollX, On 2016/05/05 14:09:34, boliu wrote: > so mViewContext wasn't thread safe The move to two functors/view contexts made it unsafe, so it was ok before this... > is mCommittedScrollX/Y safe? Looks like it's only used in this method, so should > just be converted to a local var mCommitted* are set when draw is true, but used when either draw or process is true, so the values from a previous call can be used. I don't think these can be converted into locals.
https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:280: LOG(WARNING) << "XXX Setting current_compositor_frame_consumer_ to null"; On 2016/05/05 18:36:56, Tobias Sargeant wrote: > On 2016/05/05 14:09:34, boliu wrote: > > On 2016/05/04 16:58:52, boliu wrote: > > > So yeah, what's responsible for setting the current consumer now? > > > > Didn't really resolve this one > > I think it's actually reasonable to enforce that you can't remove the current > consumer. When switching back from full screen, we set the view functor to > current, and then destroy the RTM (which causes the BVR to remove it). That's > the only way that this case occurs. Otherwise, we're destroying the whole BVR, > in which case we can just clear the current consumer before removing all of > them. I'm not suggesting don't allow destroying the current consumer. I'm asking exactly what I asked. What is responsible for setting the new current consumer *after* this. Like what code guarantees OnDrawHardware won't be called until after the current consumer is set again? https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:96: public void deleteHardwareRenderer() { On 2016/05/05 18:36:56, Tobias Sargeant wrote: > On 2016/05/04 16:58:52, boliu wrote: > > maybe this should be renamed something about trim, since that's the only > caller > > now > > Maybe this should be done in BVR::TrimMemory for all consumers? I don't like that. That assumes (correctly for now) that BVR::TrimMemory will cover *all* consumer instances. But what if sometime in the future we decides to change that? No one is going to remember about trim. I think a better way would be to move the whole trim registration logic from AwContents to here. Then each AwGLFunctor registers for its own callback. https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... File android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/test/s... android_webview/test/shell/src/org/chromium/android_webview/test/AwTestContainerView.java:212: DrawGL.drawGL(mDrawGL, viewContext, width, height, mCommittedScrollX, On 2016/05/05 19:06:46, Tobias Sargeant wrote: > On 2016/05/05 14:09:34, boliu wrote: > > so mViewContext wasn't thread safe > > The move to two functors/view contexts made it unsafe, so it was ok before > this... > > > is mCommittedScrollX/Y safe? Looks like it's only used in this method, so > should > > just be converted to a local var > > mCommitted* are set when draw is true, but used when either draw or process is > true, so the values from a previous call can be used. I don't think these can be > converted into locals. Oh good point. And mCommittedScrollX/Y are save because it's only used in this method, ie on the render thread. Should add a comment about this next to where they are declared. https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:278: DCHECK(current_compositor_frame_consumer_ != compositor_frame_consumer); Err, yeah, my comment was not about adding this DCHECK, but rather what ensures there will be a current consumer next time OnDraw/OnParentDrawConstraintsUpdated is called? https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:127: EXPECT_TRUE(DrawConstraintsEquals(constraints, initial_constraints_)); iirc leaving this out was a deliberate choice, this test does not ensure what the state of parent constraint is at the beginning https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/render_thread_manager.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/render_thread_manager.cc:365: if (compositor_frame_producer == compositor_frame_producer_) { instead of the early out, relax the DCHECK instead? https://codereview.chromium.org/1943963003/diff/200001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:915: // render thread callback is the only thing that will continue to keep it alive). way too much detail, will be out of date in a week, just say drop the reference and AwGLFunctor is responsible for cleaning up itself.. Actually, now that we no longer require RTM outlive BVR, maybe get rid of this whole "NativeLifetimeObject" business to begin with.
https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:278: DCHECK(current_compositor_frame_consumer_ != compositor_frame_consumer); On 2016/05/05 21:40:32, boliu wrote: > Err, yeah, my comment was not about adding this DCHECK, but rather what ensures > there will be a current consumer next time OnDraw/OnParentDrawConstraintsUpdated > is called? In answer to your question, AwContents::SetAwGLFunctor is the only place we set up a new current functor. Because BVR::OnDrawHardware always comes via java AwContents calling mCurrentFunctor.requestDrawGL, it will never be null in OnDraw (assuming we don't mess up the mCurrentFunctor accounting). OnParentDrawConstraintsUpdated is more difficult to reason about, although because we do it synthetically when we change current functors, I would assume that ignoring the case where the current consumer is null is ok, because we'll recover as soon as that isn't the case. https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:127: EXPECT_TRUE(DrawConstraintsEquals(constraints, initial_constraints_)); On 2016/05/05 21:40:32, boliu wrote: > iirc leaving this out was a deliberate choice, this test does not ensure what > the state of parent constraint is at the beginning We can remove the EXPECT_TRUE, however this CL adds in an OnParentDrawConstraintsUpdated as soon as the producer is connected to a consumer, so we get this call now on frame 0, whereas we didn't before, so the case 0u: needs to be here because otherwuse we hit the default: FAIL(). https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/render_thread_manager.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/render_thread_manager.cc:365: if (compositor_frame_producer == compositor_frame_producer_) { On 2016/05/05 21:40:32, boliu wrote: > instead of the early out, relax the DCHECK instead? Done. https://codereview.chromium.org/1943963003/diff/200001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:915: // render thread callback is the only thing that will continue to keep it alive). On 2016/05/05 21:40:32, boliu wrote: > way too much detail, will be out of date in a week, just say drop the reference > and AwGLFunctor is responsible for cleaning up itself.. > > Actually, now that we no longer require RTM outlive BVR, maybe get rid of this > whole "NativeLifetimeObject" business to begin with. Done.
https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:96: public void deleteHardwareRenderer() { On 2016/05/05 21:40:31, boliu wrote: > On 2016/05/05 18:36:56, Tobias Sargeant wrote: > > On 2016/05/04 16:58:52, boliu wrote: > > > maybe this should be renamed something about trim, since that's the only > > caller > > > now > > > > Maybe this should be done in BVR::TrimMemory for all consumers? > > I don't like that. That assumes (correctly for now) that BVR::TrimMemory will > cover *all* consumer instances. But what if sometime in the future we decides to > change that? No one is going to remember about trim. > > I think a better way would be to move the whole trim registration logic from > AwContents to here. Then each AwGLFunctor registers for its own callback. Not done yet? Separate CL? https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:272: DCHECK(compositor_frame_consumer); On 2016/05/05 19:06:45, Tobias Sargeant wrote: > On 2016/05/05 14:09:34, boliu wrote: > > DCHECK compositor frame is in set > > Don't need to, because being equal to the current consumer implies that. Well, you are DCHECK-ing that guarantee. https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:278: DCHECK(current_compositor_frame_consumer_ != compositor_frame_consumer); On 2016/05/06 12:41:09, Tobias Sargeant wrote: > On 2016/05/05 21:40:32, boliu wrote: > > Err, yeah, my comment was not about adding this DCHECK, but rather what > ensures > > there will be a current consumer next time > OnDraw/OnParentDrawConstraintsUpdated > > is called? > > In answer to your question, AwContents::SetAwGLFunctor is the only place we set > up a new current functor. Because BVR::OnDrawHardware always comes via java > AwContents calling mCurrentFunctor.requestDrawGL, it will never be null in > OnDraw (assuming we don't mess up the mCurrentFunctor accounting). This belongs to a comment somewhere, that BVR::Current and AwContents.mCurrent is the same one. > OnParentDrawConstraintsUpdated is more difficult to reason about, although > because we do it synthetically when we change current functors, I would assume > that ignoring the case where the current consumer is null is ok, because we'll > recover as soon as that isn't the case. Ignoring null is ok because we update draw constraints when the current pointer changes. https://codereview.chromium.org/1943963003/diff/260001/android_webview/browse... File android_webview/browser/render_thread_manager.cc (right): https://codereview.chromium.org/1943963003/diff/260001/android_webview/browse... android_webview/browser/render_thread_manager.cc:365: DCHECK(compositor_frame_producer == compositor_frame_producer_ || DCHECK is also a stream (like LOG), pipe in compositor_frame_producer and compositor_frame_producer_ so it prints out which case failed if this does fail https://codereview.chromium.org/1943963003/diff/260001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/260001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:263: private AwGLFunctor mFullScreenFunctor; // Only non-null when in fullscreen mode. wait.. space is gone again..? did clang format this or something? https://codereview.chromium.org/1943963003/diff/260001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:432: private AwContentsDestroyRunnable mAwContentsDestroyRunnable; don't need this anymore? https://codereview.chromium.org/1943963003/diff/260001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/260001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:42: private final Object mLifetimeObject; remove this too
https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:456: for (auto compositor_frame_consumer_ : compositor_frame_consumers_) { no _ suffix for local variables https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/browser_view_renderer.h:62: CompositorFrameConsumer* current_compositor_frame_consumer() { not used? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:255: public: are these new public calls needed? Are there subclasses? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:91: void FakeWindow::InvokeFunctorOnRT(Functor* functor, why have both |functor_| and all these methods that take another functor? that feels wrong https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:205: : render_thread_manager_(render_thread_manager_factory.Run(this)) { Hmm, is this safe? Is the object constructed far enough to be used as RTMClient? Also factory callback seems overkill. Just have like a separate Init object that sets the things that depend on this. You already have Attach, so why not replace that with Init(RTM, FakeWindow)? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:221: base::WaitableEvent* sync, keep the sync event logic in FakeWindow https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:10: #include "android_webview/browser/render_thread_manager.h" forward declare? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:73: void CheckCurrentlyOnRT(); style: keep blank line below https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:91: Functor* functor_; there is nothing really guaranteeing lifetime between functor and fakewindow, is it safe to use the other from their destructor for example? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:98: class Functor : public RenderThreadManagerClient { TestFunctor or something that implies it's not a production object https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:104: Functor(RenderThreadManagerFactory render_thread_manager_factory); explicit, const& https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/rendering_test.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/rendering_test.cc:69: return new RenderThreadManager(client, base::ThreadTaskRunnerHandle::Get()); hmm, dcheck this is the ui thread? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/rendering_test.h (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/rendering_test.h:68: virtual RenderThreadManager* CreateRenderThreadManager( return unique_ptr to imply passing ownership https://codereview.chromium.org/1943963003/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:432: private AwContentsDestroyRunnable mAwContentsDestroyRunnable; again, don't need this anymore
https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:91: void FakeWindow::InvokeFunctorOnRT(Functor* functor, On 2016/05/10 15:18:19, boliu wrote: > why have both |functor_| and all these methods that take another functor? that > feels wrong I guess functor_ should really be current_functor_; is only used to determine where OnDrawHardware "draws" to. Invokes and explicit draws know which functor they're associated to, which is the reason for the functor parameters here. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:205: : render_thread_manager_(render_thread_manager_factory.Run(this)) { On 2016/05/10 15:18:19, boliu wrote: > Hmm, is this safe? Is the object constructed far enough to be used as RTMClient? This is effectively the pattern we use with AwGLFunctor (and elsewhere), so it should be just as OK here as it is there. > Also factory callback seems overkill. Just have like a separate Init object that > sets the things that depend on this. You already have Attach, so why not replace > that with Init(RTM, FakeWindow)? We assume that RenderThreadManagerClient pointer is available when RTM is constructed, and stays constant (client_ should be const in RTM, I guess) so I don't think that we can Init with an existing RTM* without changing that. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:221: base::WaitableEvent* sync, On 2016/05/10 15:18:19, boliu wrote: > keep the sync event logic in FakeWindow Could do, but then callback_ has to be available to FakeWindow. It seemed more consistent to have it all in Functor or all in FakeWindow, but doing the committed_location_ setting argued against having Sync in FakeWindow.
https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:91: void FakeWindow::InvokeFunctorOnRT(Functor* functor, On 2016/05/10 16:01:17, Tobias Sargeant wrote: > On 2016/05/10 15:18:19, boliu wrote: > > why have both |functor_| and all these methods that take another functor? that > > feels wrong > > I guess functor_ should really be current_functor_; is only used to determine > where OnDrawHardware "draws" to. > > Invokes and explicit draws know which functor they're associated to, which is > the reason for the functor parameters here. But look like this method. You have |functor| arg, but using |functor_|. Which one are you supposed to use? Just get rid of this whole thing. Either pass it in, or have it be a member, don't mix them. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:205: : render_thread_manager_(render_thread_manager_factory.Run(this)) { On 2016/05/10 16:01:17, Tobias Sargeant wrote: > On 2016/05/10 15:18:19, boliu wrote: > > Hmm, is this safe? Is the object constructed far enough to be used as > RTMClient? > > This is effectively the pattern we use with AwGLFunctor (and elsewhere), so it > should be just as OK here as it is there. It's a technical concern. The object may not be fully constructed. So what happens if RTM wants to use RTMClient in the constructor? RTMClient isn't fully constructed, so could have corrupt members and whatnot. Or maybe vtable is not set up (totally made that up) > > > Also factory callback seems overkill. Just have like a separate Init object > that > > sets the things that depend on this. You already have Attach, so why not > replace > > that with Init(RTM, FakeWindow)? > > We assume that RenderThreadManagerClient pointer is available when RTM is > constructed, and stays constant (client_ should be const in RTM, I guess) so I > don't think that we can Init with an existing RTM* without changing that. No that's fine? I mean like: functor = new FUnctor() rtm = new RTM(functor) functor.init(rtm) https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:221: base::WaitableEvent* sync, On 2016/05/10 16:01:17, Tobias Sargeant wrote: > On 2016/05/10 15:18:19, boliu wrote: > > keep the sync event logic in FakeWindow > > Could do, but then callback_ has to be available to FakeWindow. Hmm, don't see why that's needed? > It seemed more > consistent to have it all in Functor or all in FakeWindow, but doing the > committed_location_ setting argued against having Sync in FakeWindow.
Patchset #15 (id:300001) has been deleted
https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:456: for (auto compositor_frame_consumer_ : compositor_frame_consumers_) { On 2016/05/10 15:18:18, boliu wrote: > no _ suffix for local variables Acknowledged. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/browser_view_renderer.h:62: CompositorFrameConsumer* current_compositor_frame_consumer() { On 2016/05/10 15:18:19, boliu wrote: > not used? Removed. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:255: public: On 2016/05/10 15:18:19, boliu wrote: > are these new public calls needed? Are there subclasses? No they don't need to be; I was just aligning these test classes with the form of others higher up in the file. Removed. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:91: void FakeWindow::InvokeFunctorOnRT(Functor* functor, On 2016/05/10 16:37:53, boliu wrote: > On 2016/05/10 16:01:17, Tobias Sargeant wrote: > > On 2016/05/10 15:18:19, boliu wrote: > > > why have both |functor_| and all these methods that take another functor? > that > > > feels wrong > > > > I guess functor_ should really be current_functor_; is only used to determine > > where OnDrawHardware "draws" to. > > > > Invokes and explicit draws know which functor they're associated to, which is > > the reason for the functor parameters here. > > But look like this method. You have |functor| arg, but using |functor_|. Which > one are you supposed to use? Just get rid of this whole thing. Either pass it > in, or have it be a member, don't mix them. Ok. It's always passed in now, but as far as I can tell that means needing another method on WindowHooks to get the current functor (which is what I did). https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.cc:205: : render_thread_manager_(render_thread_manager_factory.Run(this)) { On 2016/05/10 16:37:53, boliu wrote: > On 2016/05/10 16:01:17, Tobias Sargeant wrote: > > On 2016/05/10 15:18:19, boliu wrote: > > > Hmm, is this safe? Is the object constructed far enough to be used as > > RTMClient? > > > > This is effectively the pattern we use with AwGLFunctor (and elsewhere), so it > > should be just as OK here as it is there. > > It's a technical concern. The object may not be fully constructed. So what > happens if RTM wants to use RTMClient in the constructor? RTMClient isn't fully > constructed, so could have corrupt members and whatnot. Or maybe vtable is not > set up (totally made that up) > > > > > > Also factory callback seems overkill. Just have like a separate Init object > > that > > > sets the things that depend on this. You already have Attach, so why not > > replace > > > that with Init(RTM, FakeWindow)? > > > > We assume that RenderThreadManagerClient pointer is available when RTM is > > constructed, and stays constant (client_ should be const in RTM, I guess) so I > > don't think that we can Init with an existing RTM* without changing that. > > No that's fine? I mean like: > functor = new FUnctor() > rtm = new RTM(functor) > functor.init(rtm) Took this approach. I can't see any reason why this was any more dangerous than the way we construct AwGLFunctor (and AwContents) where we pass in this as a *Client pointer to a member that's being constructed. I agree that that means that you probably don't want to use the client pointer in the constructor of RenderThreadManager or BrowserViewRenderer though. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:10: #include "android_webview/browser/render_thread_manager.h" On 2016/05/10 15:18:19, boliu wrote: > forward declare? Done. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:73: void CheckCurrentlyOnRT(); On 2016/05/10 15:18:19, boliu wrote: > style: keep blank line below Done. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:91: Functor* functor_; On 2016/05/10 15:18:19, boliu wrote: > there is nothing really guaranteeing lifetime between functor and fakewindow, is > it safe to use the other from their destructor for example? True. It's slightly better now that FakeWindow doesn't have a Functor pointer, but there's still the reverse direction. RenderingTest manages both, and enforces (at least for the default single functor) that it is destroyed before the FakwWindow instance. Maybe this can be cleaned up some more? https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:98: class Functor : public RenderThreadManagerClient { On 2016/05/10 15:18:19, boliu wrote: > TestFunctor or something that implies it's not a production object Done. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/fake_window.h:104: Functor(RenderThreadManagerFactory render_thread_manager_factory); On 2016/05/10 15:18:19, boliu wrote: > explicit, const& Took your Init suggestion, no need for this constructor now. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/rendering_test.cc (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/rendering_test.cc:69: return new RenderThreadManager(client, base::ThreadTaskRunnerHandle::Get()); On 2016/05/10 15:18:19, boliu wrote: > hmm, dcheck this is the ui thread? This got removed. https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... File android_webview/browser/test/rendering_test.h (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/browse... android_webview/browser/test/rendering_test.h:68: virtual RenderThreadManager* CreateRenderThreadManager( On 2016/05/10 15:18:20, boliu wrote: > return unique_ptr to imply passing ownership Removed; don't need the factory any more. https://codereview.chromium.org/1943963003/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:432: private AwContentsDestroyRunnable mAwContentsDestroyRunnable; On 2016/05/10 15:18:20, boliu wrote: > again, don't need this anymore Removed.
test look mostly fine now remember the issue on M: we can't rely gc to clean up frames https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:355: std::unique_ptr<RenderThreadManager>(new RenderThreadManager( use base::WrapUnique https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.cc:128: DCHECK(functor); fwiw, production code should handle the case where no functor is called after OnDraw. doesn't need to change this now, but we could unit test that as well https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.cc:235: sync->Signal(); My earlier comment about not having sync objects in FakeFunctor is still not addressed. Conceptually, functor only cares about functor being called, and it's some external thing that's guaranteeing any synchronization behavior. both sync->Signal call is the last thing in the method, so why not just hoist that up to the caller from FakeWindow? https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.h:38: virtual FakeFunctor* GetFunctorForView(BrowserViewRenderer* view) = 0; nothing uses the parameter afaict, or maybe the new test *should* use the param? also could just merge it into DidOnDraw, I'll leave that up to you https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.h:104: using RenderThreadManagerFactory = remove https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.h:111: void Init(FakeWindow* window, nit: Move Init up https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/rendering_test.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/rendering_test.cc:64: std::unique_ptr<RenderThreadManager>(new RenderThreadManager( base::WrapUnique https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/rendering_test.cc:86: void RenderingTest::Attach() { remove
https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.cc:235: sync->Signal(); On 2016/05/13 16:09:01, boliu wrote: > My earlier comment about not having sync objects in FakeFunctor is still not > addressed. > > Conceptually, functor only cares about functor being called, and it's some > external thing that's guaranteeing any synchronization behavior. > > both sync->Signal call is the last thing in the method, so why not just hoist > that up to the caller from FakeWindow? Ahh, OK, I misunderstood what you meant the first time around. I thought you meant Sync as in kModeSync, not the WaitableEvent objects. Makes much more sense now.
https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:355: std::unique_ptr<RenderThreadManager>(new RenderThreadManager( On 2016/05/13 16:09:01, boliu wrote: > use base::WrapUnique Done. https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.cc:128: DCHECK(functor); On 2016/05/13 16:09:02, boliu wrote: > fwiw, production code should handle the case where no functor is called after > OnDraw. doesn't need to change this now, but we could unit test that as well Seems like we could simulate this by returning nullptr from GetFunctorForView. Modified to allow this. https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.cc:235: sync->Signal(); On 2016/05/13 16:23:17, Tobias Sargeant wrote: > On 2016/05/13 16:09:01, boliu wrote: > > My earlier comment about not having sync objects in FakeFunctor is still not > > addressed. > > > > Conceptually, functor only cares about functor being called, and it's some > > external thing that's guaranteeing any synchronization behavior. > > > > both sync->Signal call is the last thing in the method, so why not just hoist > > that up to the caller from FakeWindow? > > Ahh, OK, I misunderstood what you meant the first time around. I thought you > meant Sync as in kModeSync, not the WaitableEvent objects. Makes much more > sense now. Done. https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.h:104: using RenderThreadManagerFactory = On 2016/05/13 16:09:02, boliu wrote: > remove Done. https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/fake_window.h:111: void Init(FakeWindow* window, On 2016/05/13 16:09:02, boliu wrote: > nit: Move Init up Done. https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... File android_webview/browser/test/rendering_test.cc (right): https://codereview.chromium.org/1943963003/diff/320001/android_webview/browse... android_webview/browser/test/rendering_test.cc:64: std::unique_ptr<RenderThreadManager>(new RenderThreadManager( On 2016/05/13 16:09:03, boliu wrote: > base::WrapUnique Done.
Ok, just need to handle the issue on M and this should be good
https://codereview.chromium.org/1943963003/diff/360001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:355: std::unique_ptr<RenderThreadManager>(new RenderThreadManager( again, base::WrapUnique https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:3135: mCurrentFunctor.pinHardwareRenderer(); but this above the early return https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:48: private int mPinCount; maybe just call this refcount, and make the method increase ref count and decrease ref count or something like that? https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:76: ThreadUtils.runOnUiThread(new Runnable() { why post this? https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:95: pinHardwareRenderer(); pin only if requestDrawGL returns true, or if you really want to be safe, pin before, and make sure to unpin if requestDrawGL returns false..
https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:3135: mCurrentFunctor.pinHardwareRenderer(); On 2016/05/17 15:00:10, boliu wrote: > but this above the early return Oh. Didn't see that. Thanks. https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:48: private int mPinCount; On 2016/05/17 15:00:10, boliu wrote: > maybe just call this refcount, and make the method increase ref count and > decrease ref count or something like that? I felt like this was overloading the phrase refcount - but if you prefer that, sure. https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:76: ThreadUtils.runOnUiThread(new Runnable() { On 2016/05/17 15:00:10, boliu wrote: > why post this? The callback runs on the render thread, but we can only call deleteHardwareRenderer on the UI thread. The unpin might come from either the UI thread or the render thread, depending on the caller.
https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:76: ThreadUtils.runOnUiThread(new Runnable() { On 2016/05/17 15:07:36, Tobias Sargeant wrote: > On 2016/05/17 15:00:10, boliu wrote: > > why post this? > > The callback runs on the render thread, but we can only call > deleteHardwareRenderer on the UI thread. The unpin might come from either the UI > thread or the render thread, depending on the caller. wait...... really?? that wasn't explicitly stated...
https://codereview.chromium.org/1943963003/diff/360001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:355: std::unique_ptr<RenderThreadManager>(new RenderThreadManager( On 2016/05/17 15:00:10, boliu wrote: > again, base::WrapUnique Done. https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:3135: mCurrentFunctor.pinHardwareRenderer(); On 2016/05/17 15:07:36, Tobias Sargeant wrote: > On 2016/05/17 15:00:10, boliu wrote: > > but this above the early return > > Oh. Didn't see that. Thanks. Moved up, and also in onDetachedFromWindow, so that the location is consistent. https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java (right): https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:48: private int mPinCount; On 2016/05/17 15:07:36, Tobias Sargeant wrote: > On 2016/05/17 15:00:10, boliu wrote: > > maybe just call this refcount, and make the method increase ref count and > > decrease ref count or something like that? > > I felt like this was overloading the phrase refcount - but if you prefer that, > sure. Done. https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:76: ThreadUtils.runOnUiThread(new Runnable() { On 2016/05/17 15:09:43, boliu wrote: > On 2016/05/17 15:07:36, Tobias Sargeant wrote: > > On 2016/05/17 15:00:10, boliu wrote: > > > why post this? > > > > The callback runs on the render thread, but we can only call > > deleteHardwareRenderer on the UI thread. The unpin might come from either the > UI > > thread or the render thread, depending on the caller. > > wait...... really?? that wasn't explicitly stated... Checked, and the callback is run on the thread it is added on, so no need for posts or synchronization. Hooray. https://codereview.chromium.org/1943963003/diff/360001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwGLFunctor.java:95: pinHardwareRenderer(); On 2016/05/17 15:00:11, boliu wrote: > pin only if requestDrawGL returns true, or if you really want to be safe, pin > before, and make sure to unpin if requestDrawGL returns false.. Done.
lgtm I guess need to manually test and make sure nothing blows up.
On 2016/05/17 17:25:45, boliu wrote: > lgtm > > I guess need to manually test and make sure nothing blows up. Manually tested gmail, and transitioning to/from fullscreen in multiwindow mode with webview shell. Eveything looks ok.
The CQ bit was checked by tobiasjs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943963003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943963003/380001
Message was sent while issue was closed.
Description was changed from ========== WIP Handle AwContents needing multiple live functors. AwContents now needs to keep a functor per view. Either functor may be asked to draw its current frame at any time. New frames produced by the compositor go to the current RTM, and the BVR is interested in the geometry of the current RTM. Trim memory signal must go to all RTMs, and all RTMs must be able to hand back resources to the right compositor at any time, and not be forced to do so when a different RTM becomes current. BUG=597167 ========== to ========== WIP Handle AwContents needing multiple live functors. AwContents now needs to keep a functor per view. Either functor may be asked to draw its current frame at any time. New frames produced by the compositor go to the current RTM, and the BVR is interested in the geometry of the current RTM. Trim memory signal must go to all RTMs, and all RTMs must be able to hand back resources to the right compositor at any time, and not be forced to do so when a different RTM becomes current. BUG=597167 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== WIP Handle AwContents needing multiple live functors. AwContents now needs to keep a functor per view. Either functor may be asked to draw its current frame at any time. New frames produced by the compositor go to the current RTM, and the BVR is interested in the geometry of the current RTM. Trim memory signal must go to all RTMs, and all RTMs must be able to hand back resources to the right compositor at any time, and not be forced to do so when a different RTM becomes current. BUG=597167 ========== to ========== WIP Handle AwContents needing multiple live functors. AwContents now needs to keep a functor per view. Either functor may be asked to draw its current frame at any time. New frames produced by the compositor go to the current RTM, and the BVR is interested in the geometry of the current RTM. Trim memory signal must go to all RTMs, and all RTMs must be able to hand back resources to the right compositor at any time, and not be forced to do so when a different RTM becomes current. BUG=597167 Committed: https://crrev.com/6f2a192ab2b7fe5bb6bdb7f58e0d0015816ba121 Cr-Commit-Position: refs/heads/master@{#394387} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/6f2a192ab2b7fe5bb6bdb7f58e0d0015816ba121 Cr-Commit-Position: refs/heads/master@{#394387}
Message was sent while issue was closed.
forgot to update CL description..
Message was sent while issue was closed.
On 2016/05/18 13:52:13, boliu wrote: > forgot to update CL description.. Yeah, I know. :( |
