Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2638833002:
Moving temporary reference logic to SurfaceManager (Closed)
|
Created:
3 years, 11 months ago by Saman Sami Modified:
3 years, 11 months ago CC:
chromium-reviews, rjkroege, cc-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMoving temporary reference logic to SurfaceManager
This CL moves the logic of temporary surface references from
DisplayCompositor into SurfaceManager in order to centralize
the management of SurfaceReferences and to make the code available
outside of display compositor.
BUG=676384, 670454
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2638833002
Cr-Commit-Position: refs/heads/master@{#444486}
Committed: https://chromium.googlesource.com/chromium/src/+/f32903d5d3d0ca5dd5a45c3110f95f47cac02c86
Patch Set 1 #Patch Set 2 : c #
Total comments: 5
Patch Set 3 : c #
Total comments: 16
Patch Set 4 : c #
Total comments: 2
Patch Set 5 : c #
Total comments: 38
Patch Set 6 : c #
Total comments: 2
Patch Set 7 : c #Patch Set 8 : c #
Messages
Total messages: 56 (35 generated)
Description was changed from ========== Moving temporary reference logic to SurfaceManager BUG=676384 ========== to ========== Moving temporary reference logic to SurfaceManager BUG=676384 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@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...
Description was changed from ========== Moving temporary reference logic to SurfaceManager BUG=676384 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moving temporary reference logic to SurfaceManager BUG=676384 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org
samans@chromium.org changed reviewers: + kylechar@chromium.org
The CQ bit was checked by samans@chromium.org to run a CQ dry run
fsamuel and kylechar: Please review all files.
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.
lgtm
Please pass this along to jbauman@ or danakj@ for cc OWNER review.
https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:177: DCHECK(thread_checker_.CalledOnValidThread()); The DCHECK + if statements should go on the public method. We want to check there is nothing weird is happening before doing work. https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:48: SurfaceId CreateSurface(uint32_t client_id, This approach won't work after you fix the AddSurfaceReference() method, since SurfaceManager won't know about these surfaces. You'll need to use CreateSurface method above.
The CQ bit was checked by samans@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...
samans@chromium.org changed reviewers: + danakj@chromium.org, sky@chromium.org
PTAL sky: Please review services/ danakj: Please review cc/ https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:177: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/01/17 15:04:19, kylechar wrote: > The DCHECK + if statements should go on the public method. We want to check > there is nothing weird is happening before doing work. Done. https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:48: SurfaceId CreateSurface(uint32_t client_id, On 2017/01/17 15:04:19, kylechar wrote: > This approach won't work after you fix the AddSurfaceReference() method, since > SurfaceManager won't know about these surfaces. You'll need to use CreateSurface > method above. Could you elaborate? This method internally uses the method above. I didn't get any errors while running the tests.
https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:48: SurfaceId CreateSurface(uint32_t client_id, On 2017/01/17 15:32:25, Saman Sami wrote: > On 2017/01/17 15:04:19, kylechar wrote: > > This approach won't work after you fix the AddSurfaceReference() method, since > > SurfaceManager won't know about these surfaces. You'll need to use > CreateSurface > > method above. > > Could you elaborate? This method internally uses the method above. I didn't get > any errors while running the tests. Oh I missed that it used the method above! That's fine then. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:194: DCHECK(thread_checker_.CalledOnValidThread()); The private method doesn't also need a DCHECK(thread_checker_.CalledOnValidThread()) since the public method has one too. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:574: if (lifetime_type_ == LifetimeType::REFERENCES) { There was a comment that went with this code in DisplayCompositor that should be moved/modified for here. Otherwise it's a bit of a mystery what is happening. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:15: #include "cc/surfaces/surface_info.h" Is this used? https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:21: constexpr FrameSinkId kFrameSink1(1, 0); You should leave constants/helper classes/helper functions in the anonymous namespace. Only SurfaceManagerRefTest can't be in it, since you make it a friend of SurfaceManager. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:273: EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 1u); Can you also check this is true before AddSurfaceReference is called with bad arguments? https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:294: EXPECT_EQ(std::vector<LocalFrameId>{surface_id.local_frame_id()}, The tests become pretty difficult to read. Changing the helper method names and using matchers might be helpful. Also improving the comments. Here is a rough suggestion for something that seems clearer to me but feel free to modify it as you feel see fit. const SurfaceId surface_id = CreateSurface(kFrameSink2, kLocalFrame1); EXPECT_THAT(GetTempReferencesFor(kFrameSink2), ElementsAre(kLocalFrame1)); EXPECT_THAT(GetReferencesFor(manager().GetRootSurfaceId()), ElementsAre(surface_id)); const SurfaceId parent_id = CreateSurface(kFrameSink1, kLocalFrame1); manager().AddSurfaceReference(parent_id, surface_id); ... https://codereview.chromium.org/2638833002/diff/40001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.h (right): https://codereview.chromium.org/2638833002/diff/40001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.h:41: namespace test { Remove the forward def for the test. https://codereview.chromium.org/2638833002/diff/40001/services/ui/ws/BUILD.gn File services/ui/ws/BUILD.gn (right): https://codereview.chromium.org/2638833002/diff/40001/services/ui/ws/BUILD.gn... services/ui/ws/BUILD.gn:261: deps += [ "//services/ui/surfaces" ] You can remove this dep now too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by samans@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...
PTAL I addressed the comments. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:194: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/01/17 16:15:35, kylechar wrote: > The private method doesn't also need a > DCHECK(thread_checker_.CalledOnValidThread()) since the public method has one > too. Done. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:574: if (lifetime_type_ == LifetimeType::REFERENCES) { On 2017/01/17 16:15:35, kylechar wrote: > There was a comment that went with this code in DisplayCompositor that should be > moved/modified for here. Otherwise it's a bit of a mystery what is happening. Done. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:15: #include "cc/surfaces/surface_info.h" On 2017/01/17 16:15:35, kylechar wrote: > Is this used? Done. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:21: constexpr FrameSinkId kFrameSink1(1, 0); On 2017/01/17 16:15:35, kylechar wrote: > You should leave constants/helper classes/helper functions in the anonymous > namespace. Only SurfaceManagerRefTest can't be in it, since you make it a friend > of SurfaceManager. Done. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:273: EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 1u); On 2017/01/17 16:15:35, kylechar wrote: > Can you also check this is true before AddSurfaceReference is called with bad > arguments? Done. https://codereview.chromium.org/2638833002/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:294: EXPECT_EQ(std::vector<LocalFrameId>{surface_id.local_frame_id()}, On 2017/01/17 16:15:35, kylechar wrote: > The tests become pretty difficult to read. Changing the helper method names and > using matchers might be helpful. Also improving the comments. Here is a rough > suggestion for something that seems clearer to me but feel free to modify it as > you feel see fit. > > const SurfaceId surface_id = CreateSurface(kFrameSink2, kLocalFrame1); > > EXPECT_THAT(GetTempReferencesFor(kFrameSink2), ElementsAre(kLocalFrame1)); > EXPECT_THAT(GetReferencesFor(manager().GetRootSurfaceId()), > ElementsAre(surface_id)); > > const SurfaceId parent_id = CreateSurface(kFrameSink1, kLocalFrame1); > manager().AddSurfaceReference(parent_id, surface_id); > > ... Neat. https://codereview.chromium.org/2638833002/diff/40001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.h (right): https://codereview.chromium.org/2638833002/diff/40001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.h:41: namespace test { On 2017/01/17 16:15:35, kylechar wrote: > Remove the forward def for the test. Done. https://codereview.chromium.org/2638833002/diff/40001/services/ui/ws/BUILD.gn File services/ui/ws/BUILD.gn (right): https://codereview.chromium.org/2638833002/diff/40001/services/ui/ws/BUILD.gn... services/ui/ws/BUILD.gn:261: deps += [ "//services/ui/surfaces" ] On 2017/01/17 16:15:36, kylechar wrote: > You can remove this dep now too. Done.
lgtm, you might want to add crbug.com/670454 to the bug line as this will resolve it as well. https://codereview.chromium.org/2638833002/diff/60001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:17: #include "testing/gmock/include/gmock/gmock-generated-matchers.h" I think you're just supposed to include the main gmock.h file, eg: #include "testing/gmock/include/gmock/gmock.h"
Description was changed from ========== Moving temporary reference logic to SurfaceManager BUG=676384 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moving temporary reference logic to SurfaceManager BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2638833002/diff/60001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:17: #include "testing/gmock/include/gmock/gmock-generated-matchers.h" On 2017/01/17 18:44:02, kylechar wrote: > I think you're just supposed to include the main gmock.h file, eg: > > #include "testing/gmock/include/gmock/gmock.h" Done.
The CQ bit was checked by samans@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...
This CL description is extremely terse. Can you explain motivations, what is moving and why and such in the CL description?
Description was changed from ========== Moving temporary reference logic to SurfaceManager BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moving temporary reference logic to SurfaceManager This CL moves the logic of temporary surface references from DisplayCompositor into SurfaceManager in order to centralize the management of SurfaceReferences and to make the code available outside of display compositor. BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Moving temporary reference logic to SurfaceManager This CL moves the logic of temporary surface references from DisplayCompositor into SurfaceManager in order to centralize the management of SurfaceReferences and to make the code available outside of display compositor. BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moving temporary reference logic to SurfaceManager This CL moves the logic of temporary surface references from DisplayCompositor into SurfaceManager in order to centralize the management of SurfaceReferences and to make the code available outside of display compositor. BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice tests :) Few comments mostly about comments https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:48: for (auto& map_entry : temp_references_) { const auto&? https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:50: for (auto& local_frame_id : map_entry.second) { same? https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:132: LOG(ERROR) << "Cannot add self reference for " << parent_id.ToString(); aside these LOGs should be DLOGs. no need to ship these strings to billions of people. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:144: auto vector_iter = temp_references_.find(child_id.frame_sink_id()); can this have more context in its name explaining its purpose? this is an iterator pointing to the set of local frame ids for a child's frame sink id. something about that should be in the name ideally. vector_iter seems a bit misleading it reads like an iterator of a vector - in this case its an iterator of an unordered_map https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:153: // Get the vector<LocalFrameId> for the appropriate FrameSinkId and look for Change this comment to say less "what", comments should be "why". IOW It can just explain that it's looking for a temporary reference to the |child_id|. The rest is repeating the code I think? https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:165: // All surfaces get a temporary reference to the top level root. If the parent I think we can word this a bit more clearly. I suggest "Temporary references are implemented by holding a reference from the top level root to the child. If the parent...Otherwise move the reference from the top level root to the |parent_id|." https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:576: // destroyed and then if there are no references it will be deleted during there's no destruction in this method, can you maybe point where destruction occurs? https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:101: std::vector<LocalFrameId> GetTempReferencesFor( nit: I would maybe name these methods Get*ReferencesFrom() instead of __For() as they are finding references from the input parent to the children (in my mind.. maybe u disagree) https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:110: for (auto& map_entry : manager().temp_references_) this for loop has a body larger than a single line so needs {} https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:303: EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 1u); I'm struggling with why a temp reference would exist after calling AddSurfaceReference. Can you explain? I feel like it would "replace" the temp reference with the real reference, and would drop the reference from temp_references_, but may be I am misreading? https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:317: // Temporary Reference Tests this comment is going to be wrong in no time, i would remove it. if u want to document it, have it in the test names https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:324: EXPECT_THAT(GetAllTempReferences(), ElementsAre(surface_id)); Oh this is some fancy gtesting, cool https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:384: // client submits two CFs before parent submits a new CF. s/CFs/CompositorFrames/. CFs will never be found if we rename and isn't clear to the reader necessarily. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:386: const SurfaceId surface_id2 = CreateSurface(2, 1, 2); maybe don't make them sorted by the local id (ie give the 2nd one a lower id, or test both, perhaps with 3 ids or a 2nd step), cuz the SurfaceManager doesnt (shouldnt) depend on that right? https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:396: // Create parent_id and add a reference from it to surface_id2 which was use bars for variable names, like |parent_id| and |surface_id2| https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:411: // client submits two CFs before parent submits a new CF. CFs->CompositorFrames https://codereview.chromium.org/2638833002/diff/80001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2638833002/diff/80001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:51: for (auto& reference : references) {} https://codereview.chromium.org/2638833002/diff/80001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:62: for (auto& reference : references) {}
https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:296: // Add a self reference. // Try to add a self reference, this should fail. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:303: EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 1u); On 2017/01/17 21:56:42, danakj (slow) wrote: > I'm struggling with why a temp reference would exist after calling > AddSurfaceReference. Can you explain? I feel like it would "replace" the temp > reference with the real reference, and would drop the reference from > temp_references_, but may be I am misreading? Adding a reference to itself fails is why.
https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:296: // Add a self reference. On 2017/01/18 15:36:44, kylechar wrote: > // Try to add a self reference, this should fail. Ah ok ya this would help :)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:48: for (auto& map_entry : temp_references_) { On 2017/01/17 21:56:41, danakj (slow) wrote: > const auto&? Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:50: for (auto& local_frame_id : map_entry.second) { On 2017/01/17 21:56:41, danakj (slow) wrote: > same? Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:132: LOG(ERROR) << "Cannot add self reference for " << parent_id.ToString(); On 2017/01/17 21:56:41, danakj (slow) wrote: > aside these LOGs should be DLOGs. no need to ship these strings to billions of > people. Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:144: auto vector_iter = temp_references_.find(child_id.frame_sink_id()); On 2017/01/17 21:56:42, danakj (slow) wrote: > can this have more context in its name explaining its purpose? this is an > iterator pointing to the set of local frame ids for a child's frame sink id. > something about that should be in the name ideally. > > vector_iter seems a bit misleading it reads like an iterator of a vector - in > this case its an iterator of an unordered_map It's hard to pick a good name. I'm just gonna call it refs_iter because refs is what the vector is called below, to avoid the confusion about vector iterator. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:153: // Get the vector<LocalFrameId> for the appropriate FrameSinkId and look for On 2017/01/17 21:56:41, danakj (slow) wrote: > Change this comment to say less "what", comments should be "why". IOW It can > just explain that it's looking for a temporary reference to the |child_id|. The > rest is repeating the code I think? Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:165: // All surfaces get a temporary reference to the top level root. If the parent On 2017/01/17 21:56:42, danakj (slow) wrote: > I think we can word this a bit more clearly. I suggest "Temporary references are > implemented by holding a reference from the top level root to the child. If the > parent...Otherwise move the reference from the top level root to the > |parent_id|." Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:576: // destroyed and then if there are no references it will be deleted during On 2017/01/17 21:56:41, danakj (slow) wrote: > there's no destruction in this method, can you maybe point where destruction > occurs? Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:101: std::vector<LocalFrameId> GetTempReferencesFor( On 2017/01/17 21:56:42, danakj (slow) wrote: > nit: I would maybe name these methods Get*ReferencesFrom() instead of __For() as > they are finding references from the input parent to the children (in my mind.. > maybe u disagree) For temp references, the name is correct, but GetReferencesFor should be renamed. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:110: for (auto& map_entry : manager().temp_references_) On 2017/01/17 21:56:42, danakj (slow) wrote: > this for loop has a body larger than a single line so needs {} Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:296: // Add a self reference. On 2017/01/18 15:36:44, kylechar wrote: > // Try to add a self reference, this should fail. Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:317: // Temporary Reference Tests On 2017/01/17 21:56:42, danakj (slow) wrote: > this comment is going to be wrong in no time, i would remove it. if u want to > document it, have it in the test names Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:384: // client submits two CFs before parent submits a new CF. On 2017/01/17 21:56:42, danakj (slow) wrote: > s/CFs/CompositorFrames/. CFs will never be found if we rename and isn't clear to > the reader necessarily. Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:386: const SurfaceId surface_id2 = CreateSurface(2, 1, 2); On 2017/01/17 21:56:42, danakj (slow) wrote: > maybe don't make them sorted by the local id (ie give the 2nd one a lower id, or > test both, perhaps with 3 ids or a 2nd step), cuz the SurfaceManager doesnt > (shouldnt) depend on that right? Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:396: // Create parent_id and add a reference from it to surface_id2 which was On 2017/01/17 21:56:42, danakj (slow) wrote: > use bars for variable names, like |parent_id| and |surface_id2| Done. https://codereview.chromium.org/2638833002/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:411: // client submits two CFs before parent submits a new CF. On 2017/01/17 21:56:42, danakj (slow) wrote: > CFs->CompositorFrames Done. https://codereview.chromium.org/2638833002/diff/80001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2638833002/diff/80001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:51: for (auto& reference : references) On 2017/01/17 21:56:42, danakj (slow) wrote: > {} Done. https://codereview.chromium.org/2638833002/diff/80001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:62: for (auto& reference : references) On 2017/01/17 21:56:42, danakj (slow) wrote: > {} Done.
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.
Thanks, LGTM! https://codereview.chromium.org/2638833002/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:166: // nothing because the reference already exists. nit: don't linebreak after a period
https://codereview.chromium.org/2638833002/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2638833002/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:166: // nothing because the reference already exists. On 2017/01/18 19:53:11, danakj (slow) wrote: > nit: don't linebreak after a period Done.
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, sky@chromium.org, kylechar@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2638833002/#ps120001 (title: "c")
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
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-...) 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...)
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, fsamuel@chromium.org, kylechar@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2638833002/#ps140001 (title: "c")
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": 140001, "attempt_start_ts": 1484770091645570,
"parent_rev": "77003d9bba2369d252fd5ba985bbae40c817a977", "commit_rev":
"f32903d5d3d0ca5dd5a45c3110f95f47cac02c86"}
Message was sent while issue was closed.
Description was changed from ========== Moving temporary reference logic to SurfaceManager This CL moves the logic of temporary surface references from DisplayCompositor into SurfaceManager in order to centralize the management of SurfaceReferences and to make the code available outside of display compositor. BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moving temporary reference logic to SurfaceManager This CL moves the logic of temporary surface references from DisplayCompositor into SurfaceManager in order to centralize the management of SurfaceReferences and to make the code available outside of display compositor. BUG=676384,670454 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2638833002 Cr-Commit-Position: refs/heads/master@{#444486} Committed: https://chromium.googlesource.com/chromium/src/+/f32903d5d3d0ca5dd5a45c3110f9... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/f32903d5d3d0ca5dd5a45c3110f9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
