Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684933003:
Move frame_sink_id management to framesink_manager.cc/h from (Closed)
|
Created:
3 years, 10 months ago by k.devara Modified:
3 years, 9 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, rjkroege, Ian Vollick, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove frame_sink_id management to framesink_manager.cc/h from
surface_manager.cc/h and modify calls to surface_manager accoridingly.
BUG=
R=fsamuel@chromium.org
R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2684933003
Cr-Commit-Position: refs/heads/master@{#459703}
Committed: https://chromium.googlesource.com/chromium/src/+/7ee8c7800de63a7616c093f6d2582c1b90aa1cf0
Patch Set 1 #
Total comments: 10
Patch Set 2 : Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls … #Patch Set 3 : Addressed core-review comments from patchset1, added an accessor fro valid_frame_sink_ids #Patch Set 4 : added surface_factory null check as the reference is a weak_ptr. #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Patch Set 10 : fixed an issue with declaring Test class as friend class #Patch Set 11 : fixed ui/aur/demo_main.cc #Patch Set 12 : fixed some errors in compositor_unittests #Patch Set 13 : fixed ash/laser/laser_pointer_view.cc #Patch Set 14 : fixed some unit tests #Patch Set 15 : Rebase #Patch Set 16 : Unit tests fro android neded fixing #Patch Set 17 : Changed the patch to call frmesink_manager from surface_manager #
Total comments: 6
Patch Set 18 : Addressed CR comments #
Messages
Total messages: 115 (94 generated)
Description was changed from ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org ========== to ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
I think you'll need to rebase. Once you rebase and the bots are happy, then I'll l-g-t-m it. https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/direct_composit... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/direct_composit... cc/surfaces/direct_compositor_frame_sink.cc:82: display_->Initialize(this, surface_manager_, framesink_manager_); Does Display do anything useful with SurfaceManager? https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/framesink_manag... File cc/surfaces/framesink_manager.h (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/framesink_manag... cc/surfaces/framesink_manager.h:38: void RegisterFrameSinkId(const FrameSinkId& frame_sink_id); Ahh I see you've moved this here. I think you left dead code in SurfaceManager then. https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:279: // TODO(kavithadevara): commented out following bcz of moving frame_sink_ids Can you restore this change? https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.h:81: void RegisterFrameSinkId(const FrameSinkId& frame_sink_id); It seems strange to leave this here. What about moving it to FrameSinkManager? https://codereview.chromium.org/2684933003/diff/1/components/display_composit... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2684933003/diff/1/components/display_composit... components/display_compositor/gpu_compositor_frame_sink.cc:29: framesink_manager_(framesink_manager), What's the point of this member variable?
https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/direct_composit... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/direct_composit... cc/surfaces/direct_compositor_frame_sink.cc:82: display_->Initialize(this, surface_manager_, framesink_manager_); On 2017/02/12 19:04:07, Fady Samuel wrote: > Does Display do anything useful with SurfaceManager? surface_manager itself is used to get surface_id for aggregator, root_surface and to pass itself to surface_aggregator. But I needed o pass framesink_manager to display->Initialize()bcz of RegisterBeginFrameSource(frame_sink_id,begin_frame_source), a function that I moved to framesink_manager.cc from surface_manager.cc If frame_sink_id & begin_frame_source can be moved out of Display, we can avoid having framesink_manager altogether in display.cc, which seems to make sense. It will be better to have the Display deal with surface_id than frame_sink_id. and leave all frame_sink_id references in the corresponding xyz_compositor_frame_sinks. Any ideas on this? I will submit a separate CL on this (making Display, free frame_sink_id & begin_frame_source)
https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.h:81: void RegisterFrameSinkId(const FrameSinkId& frame_sink_id); On 2017/02/12 19:04:08, Fady Samuel wrote: > It seems strange to leave this here. What about moving it to FrameSinkManager? Yes- sorry I think the dfn still stayed back in surface_manager too - I will clean it up. https://codereview.chromium.org/2684933003/diff/1/components/display_composit... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2684933003/diff/1/components/display_composit... components/display_compositor/gpu_compositor_frame_sink.cc:29: framesink_manager_(framesink_manager), On 2017/02/12 19:04:08, Fady Samuel wrote: > What's the point of this member variable? You are right- the argument is just a pass-thru I don't need to store it in the member variable. I will clean this up.
https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/framesink_manag... File cc/surfaces/framesink_manager.h (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/framesink_manag... cc/surfaces/framesink_manager.h:38: void RegisterFrameSinkId(const FrameSinkId& frame_sink_id); On 2017/02/12 19:04:07, Fady Samuel wrote: > Ahh I see you've moved this here. I think you left dead code in SurfaceManager > then. Yes, I will remove it from surface_manager.h, looks like it was left behind, somehow not caught by presubmit-checks! https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2684933003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:279: // TODO(kavithadevara): commented out following bcz of moving frame_sink_ids On 2017/02/12 19:04:07, Fady Samuel wrote: > Can you restore this change? I was trying to avoid having to expose valid_frame_sink_ids from framesink_manager.cc - I guess I cant avoid it, OK, I will find a way to pass the valid_framesink_ids to Surface().
The CQ bit was checked by k.devara@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by k.devara@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by fsamuel@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: The author k.devara@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
lgtm once the bots are happy.
The CQ bit was checked by fsamuel@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: The author k.devara@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by a.suchit@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: The author k.devara@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by k.devara@samsung.com 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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_...)
Description was changed from ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by k.devara@samsung.com 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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by k.devara@samsung.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by k.devara@samsung.com 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_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 k.devara@samsung.com 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_chromium_tsan_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 k.devara@samsung.com 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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 k.devara@samsung.com 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_chromium_chromeos_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 k.devara@samsung.com 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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 k.devara@samsung.com 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_chromium_chromeos_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 k.devara@samsung.com 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_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 k.devara@samsung.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by k.devara@samsung.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by k.devara@samsung.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by k.devara@samsung.com 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by k.devara@samsung.com 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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by k.devara@samsung.com 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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/02/16 11:04:23, Fady Samuel wrote: > lgtm once the bots are happy. I changed the patch significantly from the original one in the following respect: previously I separated framesink_manager functionality completely from surface_manager and have all clients register with surface_manager & framesink_manager individually for the respective functionality. My goal was to keep the frameisnk concepts close to the Compositing clients and move the surface_manager concepts close to the display & surface. but I noticed the compositing clients are many and along with the associated unit tests it was bloating up the patch and failing the Trybots, due to many files touching the CompostorFrameSink APIs. In the last patch, what I did was the following: 1. Keep the new changes in framesink_manager that are pulled out from surface_manager, 2. make all calls to framesink_manager from within surface_manager. 3. Obvously this left all unit tests and compositing clients intact while separating functionality into framesink_manager. I'm thinking, with other work on surfaces in progress, the surface_manager should become leaner still, and later perhaps we can revisiting the CompositorFrameSinkAPIs as they are called from the compositing clients & unit tests. Kindly review the patch now, and see if LGTM is OK, for this patch.
This approach looks fine to me % fsamuel's review as well. I think this is a good way to stage the changes with a cleanup patch after. Please also format your patch description like this: https://chris.beams.io/posts/git-commit/#seven-rules https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/framesink_... File cc/surfaces/framesink_manager.cc (right): https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/framesink_... cc/surfaces/framesink_manager.cc:10: #include <queue> Are all of these includes used? https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/framesink_... cc/surfaces/framesink_manager.cc:16: #if DCHECK_IS_ON() This doesn't appear used either. https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/framesink_... File cc/surfaces/framesink_manager.h (right): https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/framesink_... cc/surfaces/framesink_manager.h:21: #if DCHECK_IS_ON() These don't appear to be used in this header file. https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/framesink_... cc/surfaces/framesink_manager.h:64: // Associates a |source| with a particular namespace. That namespace and Might be good to clean up this antiquated namespace wording here. I think namespace => frame sink id? https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:178: FrameSinkManager* framesink_manager_; Can this just be a member directly instead of a raw pointer?
I generally like this as well. https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:178: FrameSinkManager* framesink_manager_; On 2017/03/20 18:28:53, enne wrote: > Can this just be a member directly instead of a raw pointer? +1
The CQ bit was checked by k.devara@samsung.com 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/03/20 18:36:30, Fady Samuel wrote: > I generally like this as well. > > https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/surface_ma... > File cc/surfaces/surface_manager.h (right): > > https://codereview.chromium.org/2684933003/diff/320001/cc/surfaces/surface_ma... > cc/surfaces/surface_manager.h:178: FrameSinkManager* framesink_manager_; > On 2017/03/20 18:28:53, enne wrote: > > Can this just be a member directly instead of a raw pointer? > > +1 Hello, Thank u for the review. It passed all the trybots now. Previously failing linux_asan bots also succeeded, thanks to declaring teh framesink_manager_ directly instead of as pointer. Could you give a LGTM? Thank you,
Could you also format your patch description like this: https://chris.beams.io/posts/git-commit/#seven-rules
lgtm % fsamuel, and making the patch description better
This is cool! Thanks! LGTM
The CQ bit was checked by k.devara@samsung.com
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": 340001, "attempt_start_ts": 1490585640532760,
"parent_rev": "f12c9e76dcb16276ed02a33be7270f4dc12f63b5", "commit_rev":
"7ee8c7800de63a7616c093f6d2582c1b90aa1cf0"}
Message was sent while issue was closed.
Description was changed from ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move frame_sink_id management to framesink_manager.cc/h from surface_manager.cc/h and modify calls to surface_manager accoridingly. BUG= R=fsamuel@chromium.org R=enne@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2684933003 Cr-Commit-Position: refs/heads/master@{#459703} Committed: https://chromium.googlesource.com/chromium/src/+/7ee8c7800de63a7616c093f6d258... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/7ee8c7800de63a7616c093f6d258...
Message was sent while issue was closed.
On 2017/03/24 16:49:11, enne wrote: > lgtm % fsamuel, and making the patch description better My apologies- I did not notice the comment regarding the patch description. Patch already landed. Is there a simple way to fix the commit message?
Message was sent while issue was closed.
On 2017/03/24 16:49:11, enne wrote: > lgtm % fsamuel, and making the patch description better My apologies- I did not notice the comment regarding the patch description. Patch already landed. Is there a simple way to fix the commit message?
Message was sent while issue was closed.
Message was sent while issue was closed.
No, there's not, sorry. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
