Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807653003:
Move Work From CompositorFrameSinkSupport() To Init() (Closed)
|
Created:
3 years, 8 months ago by Alex Z. Modified:
3 years, 8 months ago CC:
cc-bugs_chromium.org, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, miu+watch_chromium.org, nasko+codewatch_chromium.org, piman+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove Work From CompositorFrameSinkSupport() To Init()
SurfaceManager::RegisterSurfaceFactoryClient calls
SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Calling
virtual methods on an object during its construction in the constructor is against the
style guide.
Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into
Init(). Init() is called right after constructing CompositorFrameSinkSupport
everywhere.
BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2807653003
Cr-Commit-Position: refs/heads/master@{#463853}
Committed: https://chromium.googlesource.com/chromium/src/+/962f6109508a91c8110ac855d018d334b274913e
Patch Set 1 #Patch Set 2 : Call Init In CompositorFrameSinkSupportTest #Patch Set 3 : Fix Android #Patch Set 4 : Call CompositorFrameSinkSupport::Init() in SynchronosCompositorFrameSink #Patch Set 5 : More Init #
Total comments: 2
Patch Set 6 : Contruct SurfaceFactory In Init() #Patch Set 7 : Set needs_sync_points #
Total comments: 2
Patch Set 8 : Initialize surface_manager_ To nullptr #Patch Set 9 : Add CompositorFrameSinkSupport::Create #Messages
Total messages: 92 (52 generated)
Description was changed from ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 ========== to ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
staraz@chromium.org changed reviewers: + fsamuel@chromium.org
PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM but I bet you have some android files you need to update.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
staraz@chromium.org changed reviewers: + boliu@chromium.org, danakj@chromium.org, nasko@chromium.org, reveman@chromium.org, sky@chromium.org, xlai@chromium.org
xlai@chromium.org: Please review changes in offscreen_canvas* sky@chromium.org: Please review changes in ash/ boliu@chromium.org: Please review changes in *android nasko@chromium.org: Please review changes in *frame_host reveman@chromium.org: Please review changes in exo danakj@chromium.org: Please review changes in cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
I don't fully understand what's the problem here. It seemed to worked fine so far, so why change it?
On 2017/04/08 16:47:40, Saman Sami wrote: > I don't fully understand what's the problem here. It seemed to worked fine so > far, so why change it? I agree with Saman here, Alex. Please change the description of this issue to reflect WHY you're making this change in addition to what is changing. What that said, this change is happening because calling virtual methods on an object during its construction in the constructor is against the style guide. This is because C++'s behavior is unintuitive in this situation. Suppose we wish to subclass CompositorFrameSinkSupport and override a virtual method in the subclass. The base class WILL NOT call the derived classes overridden method because the base class's constructor is called BEFORE the derived class's and so since the derived has not been constructed yet, we use the base's vtable for virtual methods calls. http://stackoverflow.com/questions/962132/calling-virtual-functions-inside-co... Virtual method calls should simply be avoided in the constructor. It was my bad that I accidentally introduced this.
On 2017/04/08 17:39:15, Fady Samuel wrote: > On 2017/04/08 16:47:40, Saman Sami wrote: > > I don't fully understand what's the problem here. It seemed to worked fine so > > far, so why change it? > > I agree with Saman here, Alex. Please change the description of this issue to > reflect WHY you're making this change in addition to what is changing. > > What that said, this change is happening because calling virtual methods on an > object during its construction in the constructor is against the style guide. > This is because C++'s behavior is unintuitive in this situation. Suppose we wish > to subclass CompositorFrameSinkSupport and override a virtual method in the > subclass. The base class WILL NOT call the derived classes overridden method > because the base class's constructor is called BEFORE the derived class's and so > since the derived has not been constructed yet, we use the base's vtable for > virtual methods calls. > http://stackoverflow.com/questions/962132/calling-virtual-functions-inside-co... > > Virtual method calls should simply be avoided in the constructor. It was my bad > that I accidentally introduced this. Does the style guide allow calling virtual methods marked final from the ctor/dtor? (Asking out of curiosity, I am not suggesting that's what should be done here)
On 2017/04/08 18:21:00, sadrul wrote: > On 2017/04/08 17:39:15, Fady Samuel wrote: > > On 2017/04/08 16:47:40, Saman Sami wrote: > > > I don't fully understand what's the problem here. It seemed to worked fine > so > > > far, so why change it? > > > > I agree with Saman here, Alex. Please change the description of this issue to > > reflect WHY you're making this change in addition to what is changing. > > > > What that said, this change is happening because calling virtual methods on an > > object during its construction in the constructor is against the style guide. > > This is because C++'s behavior is unintuitive in this situation. Suppose we > wish > > to subclass CompositorFrameSinkSupport and override a virtual method in the > > subclass. The base class WILL NOT call the derived classes overridden method > > because the base class's constructor is called BEFORE the derived class's and > so > > since the derived has not been constructed yet, we use the base's vtable for > > virtual methods calls. > > > http://stackoverflow.com/questions/962132/calling-virtual-functions-inside-co... > > > > Virtual method calls should simply be avoided in the constructor. It was my > bad > > that I accidentally introduced this. > > Does the style guide allow calling virtual methods marked final from the > ctor/dtor? (Asking out of curiosity, I am not suggesting that's what should be > done here) https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors It doesn't look like it. With that said, I do agree that the final keyword (meaning the method cannot/will not be override) would alleviate the concerns with using a virtual method call in the constructor. danakj@ probably has more insight. Given this is not specifically discussed in the style guide, I would suggest we don't block on this discussion here. I think this CL is generally taking us in a good direction.
Thank you for the explanation. I don't know what the style guide says about an init method but I would argue it should be avoided if possible because that's what the constructor is for. I'm not sure why you mentioned the possibility of deriving from CompositorFrameSinkSupport. If there are any plans for doing so, please let me know. Otherwise, I think it's better not to change the code until we actually have a reason to derive from CompositorFrameSinkSupport. Maybe we should just mark SetBeginFrameSource or the whole class as final so someone doesn't override / derive from them by mistake.
I know the style guide says don't call virtual methods in the constructor, but is it really virtual if you are in the derived class and the method is marked final?
On 2017/04/08 18:53:50, Saman Sami wrote: > I know the style guide says don't call virtual methods in the constructor, but > is it really virtual if you are in the derived class and the method is marked > final? As I said, I actually agree with Sadrul that maybe this is OK if this method is final BUT we actually maybe don't want it to be final: https://codereview.chromium.org/2795683003/ In this patch, staraz@ had wanted to use gmock in patchset #10 to intercept SetBeginFrameSource and do something. This didn't work because SetBeginFrameSource was getting called from CompositorFrameSinkSupport's constructor. Dana suggested that we fix this by introducing an Init method. I don't want to block progress here on style guide discussions. There is no statement about final methods from constructors AFAIK today, so let's assume they're not allowed for now and proceed forward and revisit separately.
components/exo lgtm
On 2017/04/08 18:59:05, Fady Samuel wrote: > On 2017/04/08 18:53:50, Saman Sami wrote: > > I know the style guide says don't call virtual methods in the constructor, but > > is it really virtual if you are in the derived class and the method is marked > > final? > > As I said, I actually agree with Sadrul that maybe this is OK if this method is > final BUT we actually maybe don't want it to be final: > > https://codereview.chromium.org/2795683003/ > > In this patch, staraz@ had wanted to use gmock in patchset #10 to intercept > SetBeginFrameSource and do something. This didn't work because > SetBeginFrameSource was getting called from CompositorFrameSinkSupport's > constructor. > > Dana suggested that we fix this by introducing an Init method. I don't want to > block progress here on style guide discussions. There is no statement about > final methods from constructors AFAIK today, so let's assume they're not allowed > for now and proceed forward and revisit separately. If we really needed to intercept SetBeginFrameSource (meaning don't call CFSS::SetBeginFrameSource, do something else entirely), then this would make sense, but it seems like all we want is accessing begin_frame_source_? In that case there are better solutions such as making begin_frame_source_ protected or making the test class a friend.
Description was changed from ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Calling virtual methods on an object during its construction in the constructor is against the style guide. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/08 18:59:05, Fady Samuel wrote: > On 2017/04/08 18:53:50, Saman Sami wrote: > > I know the style guide says don't call virtual methods in the constructor, but > > is it really virtual if you are in the derived class and the method is marked > > final? > > As I said, I actually agree with Sadrul that maybe this is OK if this method is > final BUT we actually maybe don't want it to be final: > > https://codereview.chromium.org/2795683003/ > > In this patch, staraz@ had wanted to use gmock in patchset #10 to intercept > SetBeginFrameSource and do something. This didn't work because > SetBeginFrameSource was getting called from CompositorFrameSinkSupport's > constructor. > > Dana suggested that we fix this by introducing an Init method. I don't want to > block progress here on style guide discussions. There is no statement about > final methods from constructors AFAIK today, so let's assume they're not allowed > for now and proceed forward and revisit separately. This just come to me: after the SurfaceFactoryClient interface is removed in the near future, SetBeginFrameSource would become a non-vurtual private function and the test class wouldn't be able to access it unless it's a friend of CompositorFrameSinkSupport. Therefore, we wouldn't need the Init() at that point. Is this change still worth doing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/08 19:56:00, Saman Sami wrote: > On 2017/04/08 18:59:05, Fady Samuel wrote: > > On 2017/04/08 18:53:50, Saman Sami wrote: > > > I know the style guide says don't call virtual methods in the constructor, > but > > > is it really virtual if you are in the derived class and the method is > marked > > > final? > > > > As I said, I actually agree with Sadrul that maybe this is OK if this method > is > > final BUT we actually maybe don't want it to be final: > > > > https://codereview.chromium.org/2795683003/ > > > > In this patch, staraz@ had wanted to use gmock in patchset #10 to intercept > > SetBeginFrameSource and do something. This didn't work because > > SetBeginFrameSource was getting called from CompositorFrameSinkSupport's > > constructor. > > > > Dana suggested that we fix this by introducing an Init method. I don't want to > > block progress here on style guide discussions. There is no statement about > > final methods from constructors AFAIK today, so let's assume they're not > allowed > > for now and proceed forward and revisit separately. > > If we really needed to intercept SetBeginFrameSource (meaning don't call > CFSS::SetBeginFrameSource, do something else entirely), then this would make > sense, but it seems like all we want is accessing begin_frame_source_? In that > case there are better solutions such as making begin_frame_source_ protected or > making the test class a friend. According to danakj@ from my other CL (https://codereview.chromium.org/2795683003/): We want to verify that CompositorFrameSinkSupport::SetBeginFrameSource() gets called. We shouldn't need to peek at internal states to write a test. Invoking virtual methods in constructors in dangerous (although this would soon become irrelevant once I delete the SurfaceFactoryClient interface).
> According to danakj@ from my other CL > (https://codereview.chromium.org/2795683003/): > We want to verify that CompositorFrameSinkSupport::SetBeginFrameSource() gets > called. We can verify that by looking at begin_frame_source_ right? > We shouldn't need to peek at internal states to write a test. But sometimes it's better than the alternative, and really I don't see the downside of a test class having access to members of the class it's testing, and it's a relatively common pattern. > Invoking virtual methods in constructors in dangerous Not when the method is final I guess? We are adding an unnecessary complication for every instantiation of CompositorFrameSinkSupport throughout the codebase and I don't see a good justification for it. The code looks uglier to me this way, and there is a possibility of someone introducing a bug by forgetting to call the Init method (it's a reasonable assumption that the constructor will initialize the object, right?) As I said before, it’s better to just make the test class a friend and make the controversial method final (remember that we never actually want to derive from CompositorFrameSinkSupport, this is all just because of some test), but this is just my opinion and if Fady and Dana are not convinced then I don't object.
ash LGTM
https://codereview.chromium.org/2807653003/diff/80001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2807653003/diff/80001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:40: void Init(); it's usually better to have a public factory method that calls Init, and make the constructor private. does that work here?
https://codereview.chromium.org/2807653003/diff/80001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2807653003/diff/80001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:40: void Init(); On 2017/04/10 17:02:23, boliu wrote: > it's usually better to have a public factory method that calls Init, and make > the constructor private. does that work here? We're still in discussion about the necessity of the Init since SurfaceFactory and SurfaceFactoryClient are soon to be removed. I will get back to you on the factory method when that's settled.
On 2017/04/10 14:18:46, Saman Sami wrote: > > According to danakj@ from my other CL > > (https://codereview.chromium.org/2795683003/): > > We want to verify that CompositorFrameSinkSupport::SetBeginFrameSource() gets > > called. > We can verify that by looking at begin_frame_source_ right? > > > We shouldn't need to peek at internal states to write a test. > But sometimes it's better than the alternative, and really I don't see the > downside of a test class having access to members of the class it's testing, and > it's a relatively common pattern. > > > Invoking virtual methods in constructors in dangerous > Not when the method is final I guess? > > We are adding an unnecessary complication for every instantiation of > CompositorFrameSinkSupport throughout the codebase and I don't see a good > justification for it. The code looks uglier to me this way, and there is a > possibility of someone introducing a bug by forgetting to call the Init method > (it's a reasonable assumption that the constructor will initialize the object, > right?) As I said before, it’s better to just make the test class a friend and > make the controversial method final (remember that we never actually want to > derive from CompositorFrameSinkSupport, this is all just because of some test), > but this is just my opinion and if Fady and Dana are not convinced then I don't > object. I don't think final is in general a correct solution here. Anyone may remove the final in the future and then the problem is re-introduced. It was mentioned that the interface is going away? But I think this is pointing at a situation where we can improve the code design. It's not expected that a constructor does so many things. This would also have the constructor calling AddObserver on the BeginFrameSource, which would lead to more code running. Constructors running complex logic makes testing the object harder, you can't skip an object's constructor if you want to mock out/change some behaviour in tests. This is an opportunity to make the constructor just construct and not run complex logic. I'd suggest that the Init method takes a SurfaceManager* and constructs the SurfaceFactory. WRT forgetting to call Init, you'd have a null SurfaceFactory. If that's not enough for you, there's at least 2 options: A factory method and make the constructor private as boliu suggests (this means you must allocate on the heap tho), or a bool that DCHECKs that init is called when other methods are used.
Message was sent while issue was closed.
On 2017/04/11 13:50:16, danakj (out sick) wrote: > On 2017/04/10 14:18:46, Saman Sami wrote: > > > According to danakj@ from my other CL > > > (https://codereview.chromium.org/2795683003/): > > > We want to verify that CompositorFrameSinkSupport::SetBeginFrameSource() > gets > > > called. > > We can verify that by looking at begin_frame_source_ right? > > > > > We shouldn't need to peek at internal states to write a test. > > But sometimes it's better than the alternative, and really I don't see the > > downside of a test class having access to members of the class it's testing, > and > > it's a relatively common pattern. > > > > > Invoking virtual methods in constructors in dangerous > > Not when the method is final I guess? > > > > We are adding an unnecessary complication for every instantiation of > > CompositorFrameSinkSupport throughout the codebase and I don't see a good > > justification for it. The code looks uglier to me this way, and there is a > > possibility of someone introducing a bug by forgetting to call the Init method > > (it's a reasonable assumption that the constructor will initialize the object, > > right?) As I said before, it’s better to just make the test class a friend and > > make the controversial method final (remember that we never actually want to > > derive from CompositorFrameSinkSupport, this is all just because of some > test), > > but this is just my opinion and if Fady and Dana are not convinced then I > don't > > object. > > I don't think final is in general a correct solution here. Anyone may remove the > final in the future and then the problem is re-introduced. > > It was mentioned that the interface is going away? But I think this is pointing > at a situation where we can improve the code design. It's not expected that a > constructor does so many things. This would also have the constructor calling > AddObserver on the BeginFrameSource, which would lead to more code running. > Constructors running complex logic makes testing the object harder, you can't > skip an object's constructor if you want to mock out/change some behaviour in > tests. This is an opportunity to make the constructor just construct and not run > complex logic. I'd suggest that the Init method takes a SurfaceManager* and > constructs the SurfaceFactory. > > WRT forgetting to call Init, you'd have a null SurfaceFactory. If that's not > enough for you, there's at least 2 options: A factory method and make the > constructor private as boliu suggests (this means you must allocate on the heap > tho), or a bool that DCHECKs that init is called when other methods are used. I'm fine with either the factory method or DCHECKs. Since we've decided to make CompositorFrameSinkSupport's SurfaceFactory implementation public, the removal of SurfaceFactory and SurfaceFactoryClient is no longer blocked by this CL. I'd like to postpone this change until the removal is finished.
> I'm fine with either the factory method or DCHECKs. Since we've decided to make > CompositorFrameSinkSupport's SurfaceFactory implementation public, the removal > of SurfaceFactory and SurfaceFactoryClient is no longer blocked by this CL. I'd > like to postpone this change until the removal is finished. Never mind that. We still need to move work out of constructor to mock SetBeginFrameSource.
staraz@chromium.org changed reviewers: - nasko@chromium.org
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
On 2017/04/11 16:53:43, Alex Z. wrote: > > I'm fine with either the factory method or DCHECKs. Since we've decided to > make > > CompositorFrameSinkSupport's SurfaceFactory implementation public, the removal > > > of SurfaceFactory and SurfaceFactoryClient is no longer blocked by this CL. > I'd > > like to postpone this change until the removal is finished. > > Never mind that. We still need to move work out of constructor to mock > SetBeginFrameSource. CompositorFrameSinkSupport now constructs SurfaceFactory in Init.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
staraz@chromium.org changed reviewers: + piman@chromium.org
piman@: Please review changes in *frame_host
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2807653003/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2807653003/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:53: bool needs_sync_points) { Hmmm, do I miss anything here? Your Init() function does not make use of the input parameter "needs_sync_points" at all.
https://codereview.chromium.org/2807653003/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2807653003/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:53: bool needs_sync_points) { On 2017/04/11 19:31:03, xlai (Olivia) wrote: > Hmmm, do I miss anything here? Your Init() function does not make use of the > input parameter "needs_sync_points" at all. Yea, it looks like you lost a line of code.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2807653003/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2807653003/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:53: bool needs_sync_points) { On 2017/04/11 19:35:07, Fady Samuel wrote: > On 2017/04/11 19:31:03, xlai (Olivia) wrote: > > Hmmm, do I miss anything here? Your Init() function does not make use of the > > input parameter "needs_sync_points" at all. > > Yea, it looks like you lost a line of code. My bad. It's fixed now.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> I don't think final is in general a correct solution here. Anyone may remove the > final in the future and then the problem is re-introduced. If the method ever needs to be non-final, then we can think about a solution, but now that's not the case. We can add a comment explaining why it's final so someone doesn't randomly remove it. Anyway, the method apparently is going to be non-virtual in the future so maybe there is no discussion needed here. > It was mentioned that the interface is going away? But I think this is pointing > at a situation where we can improve the code design. It's not expected that a > constructor does so many things. This would also have the constructor calling > AddObserver on the BeginFrameSource, which would lead to more code running. > Constructors running complex logic makes testing the object harder, you can't > skip an object's constructor if you want to mock out/change some behaviour in > tests. This is an opportunity to make the constructor just construct and not run > complex logic. I'd suggest that the Init method takes a SurfaceManager* and > constructs the SurfaceFactory. Maybe if we call the method something other than Init then it feels more right. To me constructor = Init. However, even if we find a nicer name, the fact that we have to call the method for every instantiation in the same exact way still feels wrong. > WRT forgetting to call Init, you'd have a null SurfaceFactory. If that's not > enough for you, there's at least 2 options: A factory method and make the > constructor private as boliu suggests (this means you must allocate on the heap > tho), or a bool that DCHECKs that init is called when other methods are used. Forgetting to call Init is not my biggest concern. I was mostly concerned about unneeded complexity for instantiation.
On 2017/04/11 20:30:16, Saman Sami wrote: > > I don't think final is in general a correct solution here. Anyone may remove > the > > final in the future and then the problem is re-introduced. > > If the method ever needs to be non-final, then we can think about a solution, > but now that's not the case. We can add a comment explaining why it's final so > someone doesn't randomly remove it. Anyway, the method apparently is going to be > non-virtual in the future so maybe there is no discussion needed here. > > > It was mentioned that the interface is going away? But I think this is > pointing > > at a situation where we can improve the code design. It's not expected that a > > constructor does so many things. This would also have the constructor calling > > AddObserver on the BeginFrameSource, which would lead to more code running. > > Constructors running complex logic makes testing the object harder, you can't > > skip an object's constructor if you want to mock out/change some behaviour in > > tests. This is an opportunity to make the constructor just construct and not > run > > complex logic. I'd suggest that the Init method takes a SurfaceManager* and > > constructs the SurfaceFactory. > > Maybe if we call the method something other than Init then it feels more right. > To me constructor = Init. However, even if we find a nicer name, the fact that > we have to call the method for every instantiation in the same exact way still > feels wrong. > > > WRT forgetting to call Init, you'd have a null SurfaceFactory. If that's not > > enough for you, there's at least 2 options: A factory method and make the > > constructor private as boliu suggests (this means you must allocate on the > heap > > tho), or a bool that DCHECKs that init is called when other methods are used. > > Forgetting to call Init is not my biggest concern. I was mostly concerned about > unneeded complexity for instantiation. The biggest reason for making this change for me is that, as danakj@ suggests, the constructor is doing too much work. The immediate problem is that I cannot mock CompositorFrameSinkSupport and use EXPECT_CALL on SetBeginFrameSource because it's called in the constructor. We have separate constructor and init else where, e.g. PlatformDisplay: https://cs.chromium.org/chromium/src/services/ui/ws/display.cc?l=77.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
my parts lgtm https://codereview.chromium.org/2807653003/diff/160001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2807653003/diff/160001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:28: surface_manager_(surface_manager), this still needs to be initialized to null, though you are free to do that in the header instead
lgtm for offscreen_canvas* On Tue, Apr 11, 2017 at 3:38 PM, <staraz@chromium.org> wrote: > > https://codereview.chromium.org/2807653003/diff/120001/cc/ > surfaces/compositor_frame_sink_support.cc > File cc/surfaces/compositor_frame_sink_support.cc (right): > > https://codereview.chromium.org/2807653003/diff/120001/cc/ > surfaces/compositor_frame_sink_support.cc#newcode53 > cc/surfaces/compositor_frame_sink_support.cc:53: bool needs_sync_points) > { > On 2017/04/11 19:35:07, Fady Samuel wrote: > > On 2017/04/11 19:31:03, xlai (Olivia) wrote: > > > Hmmm, do I miss anything here? Your Init() function does not make > use of the > > > input parameter "needs_sync_points" at all. > > > > Yea, it looks like you lost a line of code. > > My bad. It's fixed now. > > https://codereview.chromium.org/2807653003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2807653003/diff/160001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2807653003/diff/160001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:28: surface_manager_(surface_manager), On 2017/04/11 20:49:13, boliu wrote: > this still needs to be initialized to null, though you are free to do that in > the header instead Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. A common pattern is to have a static unique_ptr<A> Create(param, param, ...); that does construction + Init. This helps making sure you're not forgetting things (doubly so if you can make constructor and Init private).
On 2017/04/11 21:37:46, piman wrote: > LGTM. > > A common pattern is to have a static unique_ptr<A> Create(param, param, ...); > that does construction + Init. This helps making sure you're not forgetting > things (doubly so if you can make constructor and Init private). I am a little bit saddened that we're doing more heap allocations, but I guess we don't create CompositorFrameSinks all that often so it's not the end of the world.
On 2017/04/11 21:49:05, Fady Samuel wrote: > On 2017/04/11 21:37:46, piman wrote: > > LGTM. > > > > A common pattern is to have a static unique_ptr<A> Create(param, param, ...); > > that does construction + Init. This helps making sure you're not forgetting > > things (doubly so if you can make constructor and Init private). > > I am a little bit saddened that we're doing more heap allocations, but I guess > we don't create CompositorFrameSinks all that often so it's not the end of the > world. If the class is movable (or copyable I guess), it's ok also to have static A Create(param, param, ...);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/11 21:51:42, piman wrote: > On 2017/04/11 21:49:05, Fady Samuel wrote: > > On 2017/04/11 21:37:46, piman wrote: > > > LGTM. > > > > > > A common pattern is to have a static unique_ptr<A> Create(param, param, > ...); > > > that does construction + Init. This helps making sure you're not forgetting > > > things (doubly so if you can make constructor and Init private). > > > > I am a little bit saddened that we're doing more heap allocations, but I guess > > we don't create CompositorFrameSinks all that often so it's not the end of the > > world. > > If the class is movable (or copyable I guess), it's ok also to have static A > Create(param, param, ...); I've added the Create method.
On 2017/04/11 22:37:49, Alex Z. wrote: > On 2017/04/11 21:51:42, piman wrote: > > On 2017/04/11 21:49:05, Fady Samuel wrote: > > > On 2017/04/11 21:37:46, piman wrote: > > > > LGTM. > > > > > > > > A common pattern is to have a static unique_ptr<A> Create(param, param, > > ...); > > > > that does construction + Init. This helps making sure you're not > forgetting > > > > things (doubly so if you can make constructor and Init private). > > > > > > I am a little bit saddened that we're doing more heap allocations, but I > guess > > > we don't create CompositorFrameSinks all that often so it's not the end of > the > > > world. > > > > If the class is movable (or copyable I guess), it's ok also to have static A > > Create(param, param, ...); > > I've added the Create method. I also left the constructor as protected instead of private so SurfaceManagerTest can still mock Support in the near future.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, reveman@chromium.org, sky@chromium.org, boliu@chromium.org, xlai@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2807653003/#ps200001 (title: "Add CompositorFrameSinkSupport::Create")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491950318901180,
"parent_rev": "f1a29ebf18d1e876df019c862c9e4f6719aff64f", "commit_rev":
"933f528626b564f9b2df1e02b60feac5dfc5e07c"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491950318901180,
"parent_rev": "ae0304082bf05eeb62bd74b4ba9141f26cf1accb", "commit_rev":
"962f6109508a91c8110ac855d018d334b274913e"}
Message was sent while issue was closed.
Description was changed from ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Calling virtual methods on an object during its construction in the constructor is against the style guide. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move Work From CompositorFrameSinkSupport() To Init() SurfaceManager::RegisterSurfaceFactoryClient calls SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Calling virtual methods on an object during its construction in the constructor is against the style guide. Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into Init(). Init() is called right after constructing CompositorFrameSinkSupport everywhere. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2807653003 Cr-Commit-Position: refs/heads/master@{#463853} Committed: https://chromium.googlesource.com/chromium/src/+/962f6109508a91c8110ac855d018... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/962f6109508a91c8110ac855d018...
Message was sent while issue was closed.
tyvm for solving this. LGTM (and fwiw Init() the name is specifically suggested in the google style guide so I think it was the right choice here) |
