Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1000503002:
Add BeginFrameObserverProxy (Closed)
|
Created:
5 years, 9 months ago by simonhong Modified:
5 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, Ian Vollick, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, cc-bugs_chromium.org, James Su, scheduler-bugs_chromium.org, danakj+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd BeginFrameObserverProxy
This is partial cl from https://codereview.chromium.org/775143003/
for easy review.
In this cl, BeginFrameObserverProxy is implemented to handle
BeginFrame message for RWHV.
In the follow-up patch, RWHVAura will own it and inherit
BeginframeObserverProxyClient.
R=brianderson@chromium.org, danakj@chromium.org
BUG=372086
TEST=compositor_unittests, content_unittests
Committed: https://crrev.com/8af4c8355523e6ddf4d89ba03116b3650234b604
Cr-Commit-Position: refs/heads/master@{#321686}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 13
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : Add BeginFrameManager #
Total comments: 24
Patch Set 12 : #Patch Set 13 : #
Total comments: 26
Patch Set 14 : WIP #Patch Set 15 : #Patch Set 16 : #
Total comments: 5
Patch Set 17 : #
Total comments: 2
Patch Set 18 : #
Total comments: 4
Patch Set 19 : Rename to BeginFrameObserverProxy #Messages
Total messages: 36 (3 generated)
This is separated patch from https://codereview.chromium.org/775143003/. PTAL!
https://codereview.chromium.org/1000503002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/1000503002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.h:482: const cc::BeginFrameArgs& args) override; Accidental change? https://codereview.chromium.org/1000503002/diff/1/ui/compositor/compositor.cc File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/1/ui/compositor/compositor.cc... ui/compositor/compositor.cc:417: void Compositor::SetChildrenNeedBeginFrames(bool need_begin_frames) { Does this need it's own method?
https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... File content/browser/compositor/delegated_frame_host_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:73: bool begin_frame_sended_; sended -> sent https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:114: DCHECK(!client.begin_frame_sended()); EXPECT_TRUE https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:120: DCHECK(client.begin_frame_sended()); EXPECT_TRUE
Brian, all comments are addressed. Thanks for review. https://codereview.chromium.org/1000503002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/1000503002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.h:482: const cc::BeginFrameArgs& args) override; On 2015/03/10 23:36:46, brianderson wrote: > Accidental change? Oops. reverted. https://codereview.chromium.org/1000503002/diff/1/ui/compositor/compositor.cc File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/1/ui/compositor/compositor.cc... ui/compositor/compositor.cc:417: void Compositor::SetChildrenNeedBeginFrames(bool need_begin_frames) { On 2015/03/10 23:36:46, brianderson wrote: > Does this need it's own method? Removed. https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... File content/browser/compositor/delegated_frame_host_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:73: bool begin_frame_sended_; On 2015/03/10 23:45:41, brianderson wrote: > sended -> sent Done. https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:114: DCHECK(!client.begin_frame_sended()); On 2015/03/10 23:45:41, brianderson wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/1000503002/diff/20001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:120: DCHECK(client.begin_frame_sended()); On 2015/03/10 23:45:41, brianderson wrote: > EXPECT_TRUE Done.
Looks like you need to fix a few compile errors related to the removal of forward_begin_frames_to_children?
On 2015/03/11 02:17:57, brianderson wrote: > Looks like you need to fix a few compile errors related to the removal of > forward_begin_frames_to_children? Oops! Fixed. Thanks :)
brianderson@chromium.org changed reviewers: + mithro@mithis.com, sievers@chromium.org
lgtm +sievers for owners review of content +mithro in case he has any general feedback. (The discussion about having one BeginFrameSource before resulted in https://codereview.chromium.org/845393002/, which we'll clean up later.)
Hi Simon, Generally, this CL looks good. Just some comments on the tests involved. Tim https://codereview.chromium.org/1000503002/diff/60001/content/browser/composi... File content/browser/compositor/delegated_frame_host_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/60001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:20: class FakeTaskRunner : public base::SingleThreadTaskRunner { There is a TestSimpleTaskRunner in base/test/test_simple_task_runner.h - I wonder if you can use that here? That TaskRunner actually runs the tasks which might not be what we want here? https://codereview.chromium.org/1000503002/diff/60001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:120: EXPECT_TRUE(client.begin_frame_sent()); Can you check the right value is received somewhere here? https://codereview.chromium.org/1000503002/diff/60001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/60001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:19: class FakeTaskRunner : public base::SingleThreadTaskRunner { We definitely shouldn't be duplicating this FakeTestRunner class. If we can't use the SimpleTestTaskRunner, we should look at getting a FakeTestRunner into the base/test/ directory (or some other common location). https://codereview.chromium.org/1000503002/diff/60001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:39: class TestCompositorBeginFrameObserver : public CompositorBeginFrameObserver { Looking at this, what you should be using gmock here. class MockCompositorBeginFrameObserver : public CompositorBeginFrameObserver { MOCK_METHOD1(OnSendBeginFrame, void(const BeginFrameArgs&)); } MockCompositorBeginFrameObserver obs; EXPECT_CALL(obs, OnSendBeginFrame(args)); my_method(obs); https://code.google.com/p/googlemock/wiki/CheatSheet
https://codereview.chromium.org/1000503002/diff/60001/content/browser/composi... File content/browser/compositor/delegated_frame_host_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/60001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:20: class FakeTaskRunner : public base::SingleThreadTaskRunner { On 2015/03/11 03:42:58, mithro wrote: > There is a TestSimpleTaskRunner in base/test/test_simple_task_runner.h - I > wonder if you can use that here? That TaskRunner actually runs the tasks which > might not be what we want here? > Removed. TestSimpleTaskRunner works well. https://codereview.chromium.org/1000503002/diff/60001/content/browser/composi... content/browser/compositor/delegated_frame_host_unittest.cc:120: EXPECT_TRUE(client.begin_frame_sent()); On 2015/03/11 03:42:58, mithro wrote: > Can you check the right value is received somewhere here? frame_time check is added. https://codereview.chromium.org/1000503002/diff/60001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/60001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:19: class FakeTaskRunner : public base::SingleThreadTaskRunner { On 2015/03/11 03:42:58, mithro wrote: > We definitely shouldn't be duplicating this FakeTestRunner class. If we can't > use the SimpleTestTaskRunner, we should look at getting a FakeTestRunner into > the base/test/ directory (or some other common location). Removed and replaced with TestSimpleTaskRunner. https://codereview.chromium.org/1000503002/diff/60001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:39: class TestCompositorBeginFrameObserver : public CompositorBeginFrameObserver { On 2015/03/11 03:42:58, mithro wrote: > Looking at this, what you should be using gmock here. > > class MockCompositorBeginFrameObserver : public CompositorBeginFrameObserver { > MOCK_METHOD1(OnSendBeginFrame, void(const BeginFrameArgs&)); > } > > MockCompositorBeginFrameObserver obs; > EXPECT_CALL(obs, OnSendBeginFrame(args)); > > my_method(obs); > > https://code.google.com/p/googlemock/wiki/CheatSheet Thanks for suggestion. Looks clear with gmock!
https://codereview.chromium.org/1000503002/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1000503002/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:383: if (state_machine_.children_need_begin_frames()) { If this patch isn't enabling stuff, but it's already removing code, then these removals seem independent and like something you could do in an even smaller CL?
https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.cc:943: void DelegatedFrameHost::OnSendBeginFrame(const cc::BeginFrameArgs& args) { This code seems entirely disconnected from the other responsibilities of DelegatedFrameHost, and has nothing to do with hosting frames. Can we tear this off into a separate class? https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.h (left): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:248: std::vector<base::Closure> on_compositing_did_commit_callbacks_; unrelated move?
https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.h (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:353: // True when unified BeginFrame scheduing is used. nit: typo "scheduling' What's 'unified'? https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:356: // True when RenderWidget needs a BeginFrame. nit: you mean 'need BeginFrame messages' like var name and other function names? https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host_unittest.cc:79: compositor_.reset(); ImageTransportFactory::Terminate(); Esp. so it doesn't leak into other tests.
https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.h (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:353: // True when unified BeginFrame scheduing is used. On 2015/03/13 20:30:54, sievers wrote: > nit: typo "scheduling' > > What's 'unified'? It's the concept of using a common code path for platforms to send the BeginFrame message. Probably don't need to use the word "unified" in the code. How about: // True when BeginFrame messages are used.
I added BeginFrameManager for handling BeginFrame message instead of DelegatedFrameHost. RWHVAura will own it and inherit BeginFrameManagerClient in the next patch. PTAL! https://codereview.chromium.org/1000503002/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1000503002/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:383: if (state_machine_.children_need_begin_frames()) { On 2015/03/13 17:57:11, danakj wrote: > If this patch isn't enabling stuff, but it's already removing code, then these > removals seem independent and like something you could do in an even smaller CL? Yep, this is removed in a separate cl. https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.cc (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.cc:943: void DelegatedFrameHost::OnSendBeginFrame(const cc::BeginFrameArgs& args) { On 2015/03/13 20:04:40, danakj wrote: > This code seems entirely disconnected from the other responsibilities of > DelegatedFrameHost, and has nothing to do with hosting frames. Can we tear this > off into a separate class? Yep, handling BeginFrame message is not related with this class. I created BeginFrameManager class for it. https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.h (left): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:248: std::vector<base::Closure> on_compositing_did_commit_callbacks_; On 2015/03/13 20:04:40, danakj wrote: > unrelated move? Done. https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host.h (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:353: // True when unified BeginFrame scheduing is used. On 2015/03/13 20:53:52, brianderson wrote: > On 2015/03/13 20:30:54, sievers wrote: > > nit: typo "scheduling' > > > > What's 'unified'? > > It's the concept of using a common code path for platforms to send the > BeginFrame message. Probably don't need to use the word "unified" in the code. > > How about: > > // True when BeginFrame messages are used. Done. https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host.h:356: // True when RenderWidget needs a BeginFrame. On 2015/03/13 20:30:54, sievers wrote: > nit: you mean 'need BeginFrame messages' like var name and other function names? Done. https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... File content/browser/compositor/delegated_frame_host_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/140001/content/browser/compos... content/browser/compositor/delegated_frame_host_unittest.cc:79: compositor_.reset(); On 2015/03/13 20:30:54, sievers wrote: > ImageTransportFactory::Terminate(); > > Esp. so it doesn't leak into other tests. Done.
https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager.cc (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:17: void BeginFrameManager::OnSetNeedsBeginFrames(bool needs_begin_frames) { This looks like it should be just "SetNeedsBeginFrames()" https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:35: DCHECK(!compositor_); I'm a little unclear about this DCHECK with the if() early out below. You don't want them setting a compositor more than once. But you want to allow SetCompositor with a null compositor? But ignore it? That seems awkward, can you explain? Can we DCHECK(compositor) as well? Or... Why not just allow null here, and you don't need a separate Reset method? https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:58: last_sent_begin_frame_args_ = args; Would it be equivalent to early out in here instead of passing the last_sent_..._args_ to the AddBeginFrameObserver and having it skip sending? Or can compositor send the same args more than once? Cuz if it's the same, it seems a bit nicer to it locally here rather than passing things around. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:63: compositor_->AddBeginFrameObserver(this, last_sent_begin_frame_args_); can you add some comments explaining the purpose of thie last_sent_..._args_ thing as well? Is it for moving to a new compositor? Or...? What kinda situations will make this used? What does it do or prevent? https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager.h (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.h:25: // This class is used to manage all of the RWHV state and functionality that is can you spell out RenderWidgetHostView here? https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.h:53: // Not owned. I think the ownership is implied by the raw pointer, no comment needed for it. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager_unittest.cc:70: begin_frame_manager.SetCompositor(compositor()); Shall we also test setting the compositor to null and back and verify that it adds/removes itself as an observer of the compositor? https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor.cc:298: if (!begin_frame_observer_list_.might_have_observers()) From the name "might have", this feels like a bug where maybe it doesn't have an observer, but it returns true, so we don't SetChildrenNeedBeginFrames() but we should have. Can you convince my why this will act correctly? https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor.cc:308: if (!begin_frame_observer_list_.might_have_observers()) Dittos https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:90: compositor()->MissedBeginFrameArgsForTesting().frame_time); I don't think you need this check (or the ForTesting method). You care that Compositor sends the args, not how it stores it. https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:97: EXPECT_CALL(test_observer, OnSendBeginFrame(expected_args)); This is the right level of testing here.
https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager.h (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.h:27: class CONTENT_EXPORT BeginFrameManager BeginFrameManager is a somewhat ambiguous name. What about CompositorBeginFrameObserverImpl? https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager_unittest.cc:70: begin_frame_manager.SetCompositor(compositor()); On 2015/03/17 18:56:45, danakj wrote: > Shall we also test setting the compositor to null and back and verify that it > adds/removes itself as an observer of the compositor? +1 https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor.cc:298: if (!begin_frame_observer_list_.might_have_observers()) On 2015/03/17 18:56:45, danakj wrote: > From the name "might have", this feels like a bug where maybe it doesn't have an > observer, but it returns true, so we don't SetChildrenNeedBeginFrames() but we > should have. > > Can you convince my why this will act correctly? Looks like might_have_observers() can return the incorrect value if observers have been removed while an iterator exists (in order to avoid having to invalidate iterators when elements are removed): https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list... https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list... https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list... Since this isn't called when an iterator exists, it should return the correct value. That's a lot of mental overhead to verify correctness though. It also means we don't really need the added functionality of an ObserverList. Should we just use a std::list instead of an ObserverList?
https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager.cc (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:17: void BeginFrameManager::OnSetNeedsBeginFrames(bool needs_begin_frames) { On 2015/03/17 18:56:45, danakj wrote: > This looks like it should be just "SetNeedsBeginFrames()" Done. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:35: DCHECK(!compositor_); On 2015/03/17 18:56:45, danakj wrote: > I'm a little unclear about this DCHECK with the if() early out below. > > You don't want them setting a compositor more than once. But you want to allow > SetCompositor with a null compositor? But ignore it? That seems awkward, can you > explain? Can we DCHECK(compositor) as well? > > Or... Why not just allow null here, and you don't need a separate Reset method? DCHECK(compositor) is added. I think using Reset method can make more easier and readable code. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:58: last_sent_begin_frame_args_ = args; On 2015/03/17 18:56:45, danakj wrote: > Would it be equivalent to early out in here instead of passing the > last_sent_..._args_ to the AddBeginFrameObserver and having it skip sending? Or > can compositor send the same args more than once? Cuz if it's the same, it seems > a bit nicer to it locally here rather than passing things around. Hmm, this is not for checking sending same args twice. It helps Compositor. Please check below comment. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.cc:63: compositor_->AddBeginFrameObserver(this, last_sent_begin_frame_args_); On 2015/03/17 18:56:45, danakj wrote: > can you add some comments explaining the purpose of thie last_sent_..._args_ > thing as well? Is it for moving to a new compositor? Or...? What kinda > situations will make this used? What does it do or prevent? I commented its purpose in header file. It is for two situation. One is moving to a new compositor like you said when tab is detached and attached. The Other is needs_begin_frame_ is changed from false to true. Passing this value can help Compositor whether Compositor passing args immediately or not. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager.h (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.h:25: // This class is used to manage all of the RWHV state and functionality that is On 2015/03/17 18:56:45, danakj wrote: > can you spell out RenderWidgetHostView here? Done. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.h:27: class CONTENT_EXPORT BeginFrameManager On 2015/03/17 22:29:32, brianderson wrote: > BeginFrameManager is a somewhat ambiguous name. What about > CompositorBeginFrameObserverImpl? Done. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager.h:53: // Not owned. On 2015/03/17 18:56:45, danakj wrote: > I think the ownership is implied by the raw pointer, no comment needed for it. Removed. https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... File content/browser/renderer_host/begin_frame_manager_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/200001/content/browser/render... content/browser/renderer_host/begin_frame_manager_unittest.cc:70: begin_frame_manager.SetCompositor(compositor()); On 2015/03/17 22:29:32, brianderson wrote: > On 2015/03/17 18:56:45, danakj wrote: > > Shall we also test setting the compositor to null and back and verify that it > > adds/removes itself as an observer of the compositor? > > +1 Done. https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor.cc:298: if (!begin_frame_observer_list_.might_have_observers()) On 2015/03/17 18:56:45, danakj wrote: > From the name "might have", this feels like a bug where maybe it doesn't have an > observer, but it returns true, so we don't SetChildrenNeedBeginFrames() but we > should have. > > Can you convince my why this will act correctly? I used it to check whether the list is empty or not because ObserverList provides only that method. It will work as I expected as brian explained. Anyway, I changed to use std::list instead of using ObserverList as brian suggested. https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/200001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:90: compositor()->MissedBeginFrameArgsForTesting().frame_time); On 2015/03/17 18:56:45, danakj wrote: > I don't think you need this check (or the ForTesting method). You care that > Compositor sends the args, not how it stores it. Removed.
https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.cc (right): https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.cc:42: less whitespace? https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.cc:53: dittos https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.h (right): https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.h:20: class CompositorBeginFrameObserverImplClient { I think it's a layering weird thing to put a CompositorFooImpl over here in renderer_host, since it's not tightly coupled to the interface it's implementing. I was fine with the old name, but maybe you can get something brian would be happy with that describes what it does rather than using the InterfaceNameImpl? https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl_unittest.cc:103: EXPECT_TRUE(compositor()->BeginFrameObserverListForTesting().empty()); you could do this without exposing compositor implementation details by just checking that it does call the observer on SendBeingFrames, and stopped after Reset? https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:291: // If |last_begin_frame_args_| is still effective, send it to the new what does "still effective" mean? You're checking that it's new here? This doesn't clarify for me why Compositor is doing this check instead of the observer.. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:301: std::list<CompositorBeginFrameObserver*>::iterator it = do all of this inside the DCHECK please, temp vars are good but keep DCHECK-only code in DCHECKs https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:304: DCHECK(begin_frame_observer_list_.end() == it); DCHECK(it != end) Like we do foo != 5 not 5 != foo. end is the "fixed" thing here. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:314: DCHECK(begin_frame_observer_list_.end() != it); DCHECK(it != end) https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:386: while(it != begin_frame_observer_list_.end()) { for (auto* observer : begin_..._list_)
https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:293: if (last_begin_frame_args_sent_to_observer.frame_time != Now that there's a shared CompositorBeginFrameObserverImpl we can use, should the BeginFrame de-dupe logic and test move there? https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:295: observer->OnSendBeginFrame(missed_begin_frame_args_); Then the call to OnSendBeginFrame can move to below. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:299: host_->SetChildrenNeedBeginFrames(true); else if (missed_begin_frame_args_.IsValid()) observer->OnSendBeginFrame(missed_begin_frame_args_); We should also make missed_begin_frame_args_ invalid after calling SetChildrenNeedBeginFrames(false) to avoid sending a reallly old missed begin frame from a previous active period. Can you add a test case for that? I think this patch will fail that test. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:301: std::list<CompositorBeginFrameObserver*>::iterator it = auto? Here and elsewhere. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:318: host_->SetChildrenNeedBeginFrames(false); To make missed_begin_frame_args_ invalid: missed_begin_frame_args_ = BeginFrameArgs();
All comments are addressed. brian, dana gave an opinion about the name of CompositorBeginFrameObserverImpl. Making a class name is still difficult.. I made below candidates. BeginFrameMessageHandler BeginFrameMessageRouter BeginFrameHelper BeginFrameObserverImpl https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.cc (right): https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.cc:42: On 2015/03/19 22:36:40, danakj wrote: > less whitespace? Done. https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.cc:53: On 2015/03/19 22:36:40, danakj wrote: > dittos Done. https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl_unittest.cc:103: EXPECT_TRUE(compositor()->BeginFrameObserverListForTesting().empty()); On 2015/03/19 22:36:40, danakj wrote: > you could do this without exposing compositor implementation details by just > checking that it does call the observer on SendBeingFrames, and stopped after > Reset? Yep, that way can verify. I added that in the above test case. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:291: // If |last_begin_frame_args_| is still effective, send it to the new On 2015/03/19 22:36:41, danakj wrote: > what does "still effective" mean? You're checking that it's new here? This > doesn't clarify for me why Compositor is doing this check instead of the > observer.. Ah... I got your meaning now. You mean that making Compositor just OnSendBeginFrame(missed_begin_frame_) to newly added observer and let observer verify about it whether or not to send to child scheduler. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:293: if (last_begin_frame_args_sent_to_observer.frame_time != On 2015/03/19 22:48:59, brianderson wrote: > Now that there's a shared CompositorBeginFrameObserverImpl we can use, should > the BeginFrame de-dupe logic and test move there? Yep, moved to there. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:295: observer->OnSendBeginFrame(missed_begin_frame_args_); On 2015/03/19 22:48:59, brianderson wrote: > Then the call to OnSendBeginFrame can move to below. Done. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:299: host_->SetChildrenNeedBeginFrames(true); On 2015/03/19 22:48:59, brianderson wrote: > else if (missed_begin_frame_args_.IsValid()) > observer->OnSendBeginFrame(missed_begin_frame_args_); > > We should also make missed_begin_frame_args_ invalid after calling > SetChildrenNeedBeginFrames(false) to avoid sending a reallly old missed begin > frame from a previous active period. > > Can you add a test case for that? I think this patch will fail that test. > Done. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:301: std::list<CompositorBeginFrameObserver*>::iterator it = On 2015/03/19 22:36:41, danakj wrote: > do all of this inside the DCHECK please, temp vars are good but keep DCHECK-only > code in DCHECKs Done. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:304: DCHECK(begin_frame_observer_list_.end() == it); On 2015/03/19 22:36:41, danakj wrote: > DCHECK(it != end) > > Like we do foo != 5 not 5 != foo. end is the "fixed" thing here. Done. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:314: DCHECK(begin_frame_observer_list_.end() != it); On 2015/03/19 22:36:40, danakj wrote: > DCHECK(it != end) Done. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:318: host_->SetChildrenNeedBeginFrames(false); On 2015/03/19 22:48:59, brianderson wrote: > To make missed_begin_frame_args_ invalid: > missed_begin_frame_args_ = BeginFrameArgs(); Done. https://codereview.chromium.org/1000503002/diff/240001/ui/compositor/composit... ui/compositor/compositor.cc:386: while(it != begin_frame_observer_list_.end()) { On 2015/03/19 22:36:41, danakj wrote: > for (auto* observer : begin_..._list_) Done.
https://codereview.chromium.org/1000503002/diff/300001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.cc (right): https://codereview.chromium.org/1000503002/diff/300001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.cc:57: if (last_sent_begin_frame_args_.frame_time != args.frame_time) Ya this is what I mean :) https://codereview.chromium.org/1000503002/diff/300001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.h (right): https://codereview.chromium.org/1000503002/diff/300001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.h:49: // Pass |last_sent_begin_frame_args_| to compositor when |this| is added as a Update this comment? https://codereview.chromium.org/1000503002/diff/300001/ui/compositor/composit... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/300001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:96: EXPECT_FALSE(compositor()->MissedBeginFrameArgsForTesting().IsValid()); This is testing internal implementation details still. I think what you want to test is that: If you add 2 observers, and remove one, then add it back, it gets called with the last args. If you remove both observers, then add one back, it does not get called with the last args. Ya?
On 2015/03/20 16:07:36, simonhong wrote: > All comments are addressed. > > brian, dana gave an opinion about the name of CompositorBeginFrameObserverImpl. > > Making a class name is still difficult.. > > I made below candidates. > BeginFrameMessageHandler > BeginFrameMessageRouter > BeginFrameHelper > BeginFrameObserverImpl In the past we've used Proxy to talk about a class that passes things though it. I don't have strong feelings, I just don't think it should be called <Interface>Impl when it's not tied closely to that interface (ie is not the only implementation by design).
I think BeginFrameProxy sounds good :) https://codereview.chromium.org/1000503002/diff/300001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.h (right): https://codereview.chromium.org/1000503002/diff/300001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.h:49: // Pass |last_sent_begin_frame_args_| to compositor when |this| is added as a On 2015/03/20 16:46:11, danakj wrote: > Update this comment? Done. https://codereview.chromium.org/1000503002/diff/300001/ui/compositor/composit... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/300001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:96: EXPECT_FALSE(compositor()->MissedBeginFrameArgsForTesting().IsValid()); On 2015/03/20 16:46:11, danakj wrote: > This is testing internal implementation details still. I think what you want to > test is that: > > If you add 2 observers, and remove one, then add it back, it gets called with > the last args. > > If you remove both observers, then add one back, it does not get called with the > last args. > > Ya? Yep, Thanks!
LGTM https://codereview.chromium.org/1000503002/diff/320001/ui/compositor/composit... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/320001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:113: compositor()->RemoveBeginFrameObserver(&test_observer); it's already removed here right?
https://codereview.chromium.org/1000503002/diff/320001/ui/compositor/composit... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1000503002/diff/320001/ui/compositor/composit... ui/compositor/compositor_unittest.cc:113: compositor()->RemoveBeginFrameObserver(&test_observer); On 2015/03/20 17:28:05, danakj wrote: > it's already removed here right? Eh. there is my mistake. This should be test_observer2 and above at line 110 and 111.
On Fri, Mar 20, 2015 at 10:28 AM, <danakj@chromium.org> wrote: > LGTM > % naming it something different To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
BeginFrameObserverProxy, then lgtm!
lgtm https://codereview.chromium.org/1000503002/diff/340001/content/browser/render... File content/browser/renderer_host/compositor_begin_frame_observer_impl.h (right): https://codereview.chromium.org/1000503002/diff/340001/content/browser/render... content/browser/renderer_host/compositor_begin_frame_observer_impl.h:32: virtual ~CompositorBeginFrameObserverImpl(); nit: override instead of virtual https://codereview.chromium.org/1000503002/diff/340001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1000503002/diff/340001/content/content_browse... content/content_browser.gypi:1896: 'browser/renderer_host/compositor_begin_frame_observer_impl.cc', nit: Does this fit better in the section in line 1943 where we exclude render_widget_host_view_aura.cc and other files for !aura? https://codereview.chromium.org/1000503002/diff/340001/content/content_tests.... File content/content_tests.gypi (right): https://codereview.chromium.org/1000503002/diff/340001/content/content_tests.... content/content_tests.gypi:1137: 'browser/renderer_host/compositor_begin_frame_observer_impl_unittest.cc', nit: same here... use_aura!=1 would be more correct than android.
https://codereview.chromium.org/1000503002/diff/340001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1000503002/diff/340001/content/content_browse... content/content_browser.gypi:1896: 'browser/renderer_host/compositor_begin_frame_observer_impl.cc', On 2015/03/20 22:45:21, sievers wrote: > nit: Does this fit better in the section in line 1943 where we exclude > render_widget_host_view_aura.cc and other files for !aura? I added here because this class will also used for Mac in the follow up patch. If then, is this the right place?
On 2015/03/20 23:01:48, simonhong wrote: > https://codereview.chromium.org/1000503002/diff/340001/content/content_browse... > File content/content_browser.gypi (right): > > https://codereview.chromium.org/1000503002/diff/340001/content/content_browse... > content/content_browser.gypi:1896: > 'browser/renderer_host/compositor_begin_frame_observer_impl.cc', > On 2015/03/20 22:45:21, sievers wrote: > > nit: Does this fit better in the section in line 1943 where we exclude > > render_widget_host_view_aura.cc and other files for !aura? > > I added here because this class will also used for Mac in the follow up patch. > If then, is this the right place? Ok, sounds good then.
The CQ bit was checked by simonhong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, sievers@chromium.org, brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1000503002/#ps350001 (title: "Rename to BeginFrameObserverProxy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1000503002/350001
Message was sent while issue was closed.
Committed patchset #19 (id:350001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/8af4c8355523e6ddf4d89ba03116b3650234b604 Cr-Commit-Position: refs/heads/master@{#321686} |
