Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
720883003:
Adds a CloneAndAnimate function to WindowManagerInternalClient (Closed)
|
Created:
6 years, 1 month ago by sky Modified:
6 years, 1 month ago Reviewers:
msw CC:
mojo-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Project:
mojo Visibility:
Public. |
DescriptionAdds a CloneAndAnimate function to WindowManagerInternalClient
The idea is to clone an existing tree of views reusing the surface id
and run the animation on it. By doing this we effectively have a (visual) copy of the views that we can then run an animation on. Eg fade out, apply a transform... If reusing surface ids becomes problematic we could instead go with creating a pixel copy of the underyling surface and change the cloning not to recurse. That's an implementation detail that could be changed later on.
I don't think cloning is useful outside of animations and really only makes sense at the WM level. Hence going with a narrow API for now.
There are a number of other pieces that need to be implemented before this can be used, but this is a good starting point.
R=msw@chromium.org
BUG=434429
Committed: https://chromium.googlesource.com/external/mojo/+/364f9e4767069aed16ac1cd87b490ddbbdf0f047
Patch Set 1 #Patch Set 2 : tweaks #
Total comments: 94
Patch Set 3 : feedback #
Total comments: 5
Patch Set 4 : moar feedback #Patch Set 5 : comment #Patch Set 6 : Add GetView(ClonedViewId) coverage #Messages
Total messages: 9 (0 generated)
Sorry this took me a while... https://codereview.chromium.org/720883003/diff/20001/mojo/services/public/int... File mojo/services/public/interfaces/window_manager/window_manager_internal.mojom (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/window_manager/window_manager_internal.mojom:36: CloneAndAnimate(uint32 view_id) => (bool success); q: is the success response here valuable? (would the ViewManager do something different based on the success or failure? or is it useful as a callback for when the animation completes?) It seems odd if that has more value than the success or failure of the functions above. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:26: ServerView* clone = new ServerView(delegate, ClonedViewId()); q: should the clone copy any/all ServerView::properties()? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:33: // are nit: fix line break https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:35: void CloneAllViews(const ServerView* parent, nit: CloneViewTree? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:39: for (const ServerView* to_clone : parent->GetChildren()) { q: will cloned views preserve the ordering of the original views? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:51: void ReparentClonedViews(ServerView* add_to, nit: s/add_to/new_parent/? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:69: void DeleteAllViews(ServerView* view) { nit: DeleteViewTree? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:303: if (!in_destructor_ && root_->Contains(view) && view != root_.get() && Add a comment here explaining why this is useful, like "Preserve cloned views within the view tree for animations on destruction." https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:306: ReparentClonedViews(view->parent(), &parent_above, view); I worry a bit about potential performance implications here. If a view tree is being destroyed, will the cloned views slowly bubble up the tree in a naive fashion? If so, is that okay (for now with a TODO)? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:306: ReparentClonedViews(view->parent(), &parent_above, view); Add a comment here explaining why this is useful, like OnWillChangeViewVisibility. Perhaps "Preserve cloned views within the view tree for animations on destruction." https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:323: ReparentClonedViews(view->parent(), &parent_above, view); I'm confused; does this reparent cloned views to |view|'s old parent? Why wouldn't they also move to the new parent, and why do they need to bubble up the tree at all? Perhaps add a comment here like in OnWillChangeViewVisibility. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:427: callback.Run(CloneAndAnimate(ViewIdFromTransportId(transport_view_id))); optional nit: it might be more straightforward to inline the helper. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:427: callback.Run(CloneAndAnimate(ViewIdFromTransportId(transport_view_id))); Add a comment describing the expected lifetime of cloned views (here?). https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.h:121: // Implementation of WindowManagerInternalClient functions. See mojom for nit: "WindowManagerInternalClient implementation helper(s); see mojom for details." https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/display_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/display_manager.cc:57: sqs->opacity = view->opacity(); q: will applying the same opacity throughout the view tree in your stub animation make nested views render progressively more transparent? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/server_view.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/server_view.cc:130: delegate_->OnScheduleViewPaint(this); nit q: should 0 opactiy trigger OnWillChangeViewVisibility? (probably not) https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/server_view.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/server_view.h:91: float opacity_; nit q: can you replace |visible_| with opacity_ > 0? (maybe not, if SetVisible shouldn't clobber a tangential opacity) https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_apptest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:191: vm->CreateView(BuildViewId(0, 1), nit: Use RootViewId() (or why not InvalidViewId()) https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:199: if (views[i].view_id == BuildViewId(kInvalidConnectionId, 1)) nit: use ClonedViewId(). https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:1367: // Attempt to create a bad view so we know all messages have been received nit: is WaitForAllMessages's impl worth noting? If so, add a trailing period. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:1380: // No one should be able to see the cloned tree. Hmm, I don't know if I fully understand the implications of this policy. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_impl.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_impl.h:83: void GetViewTree(const ViewId& view_id, nit: match the ordering of the mojom? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_impl.h:84: std::vector<const ServerView*>* views); nit: return an std::vector<const ServerView*>? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:239: const ServerView* GetFirstCloned(const ServerView* view) { nit: move to anon namespace above? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:249: // 1,1 nit: annotate this view (this comment block is great!) https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:254: void SetUpAnimate1(ViewManagerServiceTest* test, ViewId* embed_view_id) { ditto nit: move to anon namespace above? (nix '1' from function name?) https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:260: std::string(), *embed_view_id, InterfaceRequest<ServiceProvider>())); For my clarity, is this embedding a connection with nothing on the other end? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:262: test->connection_manager()->GetConnectionWithRoot(*embed_view_id); Harking back to my confusion about view manager connections. Does "GetConnectionWithRoot" return the connection embeded within the argument view? We created embed_view_id wm_connection's id (1, right?), but this returns connection1 (with id 2, right?), which is not wm_connection. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:300: EXPECT_EQ(gfx::Rect(2, 3, 6, 7), cloned_view->bounds()); nit: compare to v2 and v3 bounds instead of copied literals? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. Should this check GetFirstCloned() on the results of GetView() or GetViewTree(), instead of solely relying on the direct contents of GetViewTree()? (ie. ensure those ServerViews don't return cloned views via GetChildren()?) https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:350: // Because the cloned view changed parents it's bounds should have changed. nit: "its" without apostrophe. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:363: // Hide the parent of the cloned view, should force the cloned view to become nit: view, which should https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:377: // Clone and animate on tree with more depth. Basicly that of SetUpAnimate1() nit: "a tree", also "Basically" https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:380: const ViewId embed_view_id(wm_connection()->id(), 1); nit: Should SetUpAnimate1 be split into two steps? One would setup the tree, (which can be re-used here), and another step to CloneAndAnimate (2,2) and verify the expectations. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:414: // |cloned_view| should have a child and it's child should have a child. nit: "its" without apostrophe. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:417: EXPECT_EQ(1u, cloned_view_child->GetChildren().size()); nit: ensure the cloned_view_child has ClonedViewId() https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... File mojo/services/window_manager/window_manager_app.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... mojo/services/window_manager/window_manager_app.cc:33: void EmptyCallback(bool value) { nit: I feel like there ought to be a better way to do this... can you just pass a just pass an anonymous base::Callback<void(bool)>()? https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... File mojo/services/window_manager/window_manager_app.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... mojo/services/window_manager/window_manager_app.h:86: // Calls through to WindowManagerInternalClient::CloneAndAnimate(); see mojom nit "to WindowManagerInternalClient; see " https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... mojo/services/window_manager/window_manager_app.h:88: void CloneAndAnimate(Id view); Is this actually called somewhere?
New patch uploaded https://codereview.chromium.org/720883003/diff/20001/mojo/services/public/int... File mojo/services/public/interfaces/window_manager/window_manager_internal.mojom (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/window_manager/window_manager_internal.mojom:36: CloneAndAnimate(uint32 view_id) => (bool success); On 2014/11/18 23:37:41, msw wrote: > q: is the success response here valuable? (would the ViewManager do something > different based on the success or failure? or is it useful as a callback for > when the animation completes?) It seems odd if that has more value than the > success or failure of the functions above. I'm not sure if I'll end up needing the callback, so I nuked it. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:26: ServerView* clone = new ServerView(delegate, ClonedViewId()); On 2014/11/18 23:37:42, msw wrote: > q: should the clone copy any/all ServerView::properties()? Yes, I missed opacity. No need to visibility as we only clone visible views. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:33: // are On 2014/11/18 23:37:42, msw wrote: > nit: fix line break Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:35: void CloneAllViews(const ServerView* parent, On 2014/11/18 23:37:42, msw wrote: > nit: CloneViewTree? Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:39: for (const ServerView* to_clone : parent->GetChildren()) { On 2014/11/18 23:37:42, msw wrote: > q: will cloned views preserve the ordering of the original views? Yes. I added a comment indicating this. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:51: void ReparentClonedViews(ServerView* add_to, On 2014/11/18 23:37:41, msw wrote: > nit: s/add_to/new_parent/? Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:69: void DeleteAllViews(ServerView* view) { On 2014/11/18 23:37:41, msw wrote: > nit: DeleteViewTree? Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:306: ReparentClonedViews(view->parent(), &parent_above, view); On 2014/11/18 23:37:42, msw wrote: > I worry a bit about potential performance implications here. If a view tree is > being destroyed, will the cloned views slowly bubble up the tree in a naive > fashion? If so, is that okay (for now with a TODO)? The bubbling at most happens for the depth of the tree, which I suspect won't be vary deep. If there is anything bad here it's having to search through the view tree every time. I'm hesitant to try to optimize this unless it shows up. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:323: ReparentClonedViews(view->parent(), &parent_above, view); On 2014/11/18 23:37:42, msw wrote: > I'm confused; does this reparent cloned views to |view|'s old parent? Why > wouldn't they also move to the new parent, and why do they need to bubble up the > tree at all? Perhaps add a comment here like in OnWillChangeViewVisibility. Added similar comment to above, let me know if still unclear. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:427: callback.Run(CloneAndAnimate(ViewIdFromTransportId(transport_view_id))); On 2014/11/18 23:37:42, msw wrote: > optional nit: it might be more straightforward to inline the helper. This code is all going to change drastically (see all the todos). I'm going to leave it like this for now. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.h:121: // Implementation of WindowManagerInternalClient functions. See mojom for On 2014/11/18 23:37:42, msw wrote: > nit: "WindowManagerInternalClient implementation helper(s); see mojom for > details." Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/display_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/display_manager.cc:57: sqs->opacity = view->opacity(); On 2014/11/18 23:37:42, msw wrote: > q: will applying the same opacity throughout the view tree in your stub > animation make nested views render progressively more transparent? Yes. Opacity is inherited. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/server_view.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/server_view.cc:130: delegate_->OnScheduleViewPaint(this); On 2014/11/18 23:37:42, msw wrote: > nit q: should 0 opactiy trigger OnWillChangeViewVisibility? (probably not) We don't do that in the aura/views side, which doesn't mean it's the right thing. I'm going to leave it as is for now. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/server_view.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/server_view.h:91: float opacity_; On 2014/11/18 23:37:42, msw wrote: > nit q: can you replace |visible_| with opacity_ > 0? > (maybe not, if SetVisible shouldn't clobber a tangential opacity) Indeed. They are two separate things. Opacity isn't configurable yet, but no doubt that'll change. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_apptest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:191: vm->CreateView(BuildViewId(0, 1), On 2014/11/18 23:37:42, msw wrote: > nit: Use RootViewId() (or why not InvalidViewId()) Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:199: if (views[i].view_id == BuildViewId(kInvalidConnectionId, 1)) On 2014/11/18 23:37:42, msw wrote: > nit: use ClonedViewId(). Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:1367: // Attempt to create a bad view so we know all messages have been received On 2014/11/18 23:37:42, msw wrote: > nit: is WaitForAllMessages's impl worth noting? If so, add a trailing period. Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:1380: // No one should be able to see the cloned tree. On 2014/11/18 23:37:42, msw wrote: > Hmm, I don't know if I fully understand the implications of this policy. Indeed. This is the most restrictive we can be. Hopefully we don't need to open it up. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_impl.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_impl.h:83: void GetViewTree(const ViewId& view_id, On 2014/11/18 23:37:43, msw wrote: > nit: match the ordering of the mojom? Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_impl.h:84: std::vector<const ServerView*>* views); On 2014/11/18 23:37:43, msw wrote: > nit: return an std::vector<const ServerView*>? Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:239: const ServerView* GetFirstCloned(const ServerView* view) { On 2014/11/18 23:37:43, msw wrote: > nit: move to anon namespace above? I placed it here to reinforce it's for the clone tests. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:249: // 1,1 On 2014/11/18 23:37:43, msw wrote: > nit: annotate this view (this comment block is great!) Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:254: void SetUpAnimate1(ViewManagerServiceTest* test, ViewId* embed_view_id) { On 2014/11/18 23:37:43, msw wrote: > ditto nit: move to anon namespace above? (nix '1' from function name?) Same comment about wanting to keep it close to animation related tests. I see this file as getting other tests, not just animation related. I did stick this (and GetFirstCloned) into an anonymous namespace though. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:260: std::string(), *embed_view_id, InterfaceRequest<ServiceProvider>())); On 2014/11/18 23:37:43, msw wrote: > For my clarity, is this embedding a connection with nothing on the other end? Depends upon what you mean by nothing on the other end. Embed is routed through COnnectionManagerDelegate::CreateClientConnectionForEmbedAtView, which for these tests is TestConnectionManagerDelegate::CreateClientConnectionForEmbedAtView, which doesn't create a real connection. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:262: test->connection_manager()->GetConnectionWithRoot(*embed_view_id); On 2014/11/18 23:37:43, msw wrote: > Harking back to my confusion about view manager connections. And my overloading of 'connection'. I made it worse with ClientConnection. Sorry. > Does > "GetConnectionWithRoot" return the connection embeded within the argument view? That's right. This returns the ViewManagerServiceImpl that was created by the Embed call on 259. This ViewManagerServiceImpl has *embed_view_id as it's root. > We created embed_view_id wm_connection's id (1, right?), but this returns > connection1 (with id 2, right?), which is not wm_connection. That's right. Every call to Embed creates a new ViewManagerServiceImpl whose id is one greater than the last. I suspect we'll want some randomness here, but that's how it is now. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:300: EXPECT_EQ(gfx::Rect(2, 3, 6, 7), cloned_view->bounds()); On 2014/11/18 23:37:43, msw wrote: > nit: compare to v2 and v3 bounds instead of copied literals? Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. On 2014/11/18 23:37:43, msw wrote: > Should this check GetFirstCloned() on the results of GetView() or GetViewTree(), > instead of solely relying on the direct contents of GetViewTree()? (ie. ensure > those ServerViews don't return cloned views via GetChildren()?) I'm not sure I get what you're asking. SetUpAnimate1 does test views trees. Is that what you mean? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:350: // Because the cloned view changed parents it's bounds should have changed. On 2014/11/18 23:37:43, msw wrote: > nit: "its" without apostrophe. Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:363: // Hide the parent of the cloned view, should force the cloned view to become On 2014/11/18 23:37:43, msw wrote: > nit: view, which should Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:377: // Clone and animate on tree with more depth. Basicly that of SetUpAnimate1() On 2014/11/18 23:37:43, msw wrote: > nit: "a tree", also "Basically" Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:380: const ViewId embed_view_id(wm_connection()->id(), 1); On 2014/11/18 23:37:43, msw wrote: > nit: Should SetUpAnimate1 be split into two steps? One would setup the tree, > (which can be re-used here), and another step to CloneAndAnimate (2,2) and > verify the expectations. I kind of like it this way, but I of course may change that as I need more tests. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:414: // |cloned_view| should have a child and it's child should have a child. On 2014/11/18 23:37:43, msw wrote: > nit: "its" without apostrophe. Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:417: EXPECT_EQ(1u, cloned_view_child->GetChildren().size()); On 2014/11/18 23:37:43, msw wrote: > nit: ensure the cloned_view_child has ClonedViewId() Done. https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... File mojo/services/window_manager/window_manager_app.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... mojo/services/window_manager/window_manager_app.cc:33: void EmptyCallback(bool value) { On 2014/11/18 23:37:43, msw wrote: > nit: I feel like there ought to be a better way to do this... can you just pass > a just pass an anonymous base::Callback<void(bool)>()? No longer applicable as I removed the callback. https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... File mojo/services/window_manager/window_manager_app.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/window_man... mojo/services/window_manager/window_manager_app.h:88: void CloneAndAnimate(Id view); On 2014/11/18 23:37:44, msw wrote: > Is this actually called somewhere? I was adding it now to cut down on size of future patches. But I nuked it for now.
https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:26: ServerView* clone = new ServerView(delegate, ClonedViewId()); On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > q: should the clone copy any/all ServerView::properties()? > > Yes, I missed opacity. No need to visibility as we only clone visible views. Good catch, but I was wondering about the std::map<std::string, std::vector<uint8_t>> ServerView::properties_ https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:306: ReparentClonedViews(view->parent(), &parent_above, view); On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > I worry a bit about potential performance implications here. If a view tree is > > being destroyed, will the cloned views slowly bubble up the tree in a naive > > fashion? If so, is that okay (for now with a TODO)? > > The bubbling at most happens for the depth of the tree, which I suspect won't be > vary deep. If there is anything bad here it's having to search through the view > tree every time. I'm hesitant to try to optimize this unless it shows up. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:323: ReparentClonedViews(view->parent(), &parent_above, view); On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > I'm confused; does this reparent cloned views to |view|'s old parent? Why > > wouldn't they also move to the new parent, and why do they need to bubble up > the > > tree at all? Perhaps add a comment here like in OnWillChangeViewVisibility. > > Added similar comment to above, let me know if still unclear. I guess it makes sense, re-parenting an ancestor of the the original view tree is likely a precursor to changing its bounds, and the cloned views shouldn't be part of that... I feel like there may be cases where this doesn't make sense, like if you're closing a view within a view that's moving/closing? https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:427: callback.Run(CloneAndAnimate(ViewIdFromTransportId(transport_view_id))); On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > optional nit: it might be more straightforward to inline the helper. > > This code is all going to change drastically (see all the todos). I'm going to > leave it like this for now. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/display_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/display_manager.cc:57: sqs->opacity = view->opacity(); On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > q: will applying the same opacity throughout the view tree in your stub > > animation make nested views render progressively more transparent? > > Yes. Opacity is inherited. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/server_view.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/server_view.cc:130: delegate_->OnScheduleViewPaint(this); On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > nit q: should 0 opactiy trigger OnWillChangeViewVisibility? (probably not) > > We don't do that in the aura/views side, which doesn't mean it's the right > thing. I'm going to leave it as is for now. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/server_view.h (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/server_view.h:91: float opacity_; On 2014/11/19 00:49:39, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > nit q: can you replace |visible_| with opacity_ > 0? > > (maybe not, if SetVisible shouldn't clobber a tangential opacity) > > Indeed. They are two separate things. Opacity isn't configurable yet, but no > doubt that'll change. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_apptest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:1380: // No one should be able to see the cloned tree. On 2014/11/19 00:49:40, sky wrote: > On 2014/11/18 23:37:42, msw wrote: > > Hmm, I don't know if I fully understand the implications of this policy. > > Indeed. This is the most restrictive we can be. Hopefully we don't need to open > it up. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:239: const ServerView* GetFirstCloned(const ServerView* view) { On 2014/11/19 00:49:40, sky wrote: > On 2014/11/18 23:37:43, msw wrote: > > nit: move to anon namespace above? > > I placed it here to reinforce it's for the clone tests. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:260: std::string(), *embed_view_id, InterfaceRequest<ServiceProvider>())); On 2014/11/19 00:49:40, sky wrote: > On 2014/11/18 23:37:43, msw wrote: > > For my clarity, is this embedding a connection with nothing on the other end? > > Depends upon what you mean by nothing on the other end. Embed is routed through > COnnectionManagerDelegate::CreateClientConnectionForEmbedAtView, which for these > tests is TestConnectionManagerDelegate::CreateClientConnectionForEmbedAtView, > which doesn't create a real connection. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. On 2014/11/19 00:49:40, sky wrote: > On 2014/11/18 23:37:43, msw wrote: > > Should this check GetFirstCloned() on the results of GetView() or > GetViewTree(), > > instead of solely relying on the direct contents of GetViewTree()? (ie. ensure > > those ServerViews don't return cloned views via GetChildren()?) > > I'm not sure I get what you're asking. SetUpAnimate1 does test views trees. Is > that what you mean? Ah, so really this is a test that *just the vector returned* by GetViewTree doesn't return cloned views, right? Explicitly using GetView or checking the members of that vector for their children can expose cloned views? Maybe I'm still misunderstanding... https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:380: const ViewId embed_view_id(wm_connection()->id(), 1); On 2014/11/19 00:49:40, sky wrote: > On 2014/11/18 23:37:43, msw wrote: > > nit: Should SetUpAnimate1 be split into two steps? One would setup the tree, > > (which can be re-used here), and another step to CloneAndAnimate (2,2) and > > verify the expectations. > > I kind of like it this way, but I of course may change that as I need more > tests. Acknowledged. https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.cc (right): https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:390: // hide to. Reparent so that animations are still visible. nit: "hide too" https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.h (right): https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.h:121: // WindowManagerInternalClient implementation helper(s); see mojom for nit: sorry I didn't meant to include the parens around 's', I meant either "helper;" or "helpers;" https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_apptest.cc (right): https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:174: void CloneAndAnimate(WindowManagerInternalClient* wm, Id view_id) { optional nit: remove this helper for now?
New patch uploaded. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:26: ServerView* clone = new ServerView(delegate, ClonedViewId()); On 2014/11/19 01:27:51, msw wrote: > On 2014/11/19 00:49:39, sky wrote: > > On 2014/11/18 23:37:42, msw wrote: > > > q: should the clone copy any/all ServerView::properties()? > > > > Yes, I missed opacity. No need to visibility as we only clone visible views. > > Good catch, but I was wondering about the std::map<std::string, > std::vector<uint8_t>> ServerView::properties_ In theory those shouldn't matter as the only possible use of them is the window manager, which doesn't see the cloned views anyway. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:323: ReparentClonedViews(view->parent(), &parent_above, view); On 2014/11/19 01:27:51, msw wrote: > On 2014/11/19 00:49:39, sky wrote: > > On 2014/11/18 23:37:42, msw wrote: > > > I'm confused; does this reparent cloned views to |view|'s old parent? Why > > > wouldn't they also move to the new parent, and why do they need to bubble up > > the > > > tree at all? Perhaps add a comment here like in OnWillChangeViewVisibility. > > > > Added similar comment to above, let me know if still unclear. > > I guess it makes sense, re-parenting an ancestor of the the original view tree > is likely a precursor to changing its bounds, and the cloned views shouldn't be > part of that... I feel like there may be cases where this doesn't make sense, > like if you're closing a view within a view that's moving/closing? I think we'll just have to see how this pans out. I tend to think what I have here is the right thing, but we'll evaluate as we start adding actual animations. No doubt a bunch of this will need to change. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. On 2014/11/19 01:27:51, msw wrote: > On 2014/11/19 00:49:40, sky wrote: > > On 2014/11/18 23:37:43, msw wrote: > > > Should this check GetFirstCloned() on the results of GetView() or > > GetViewTree(), > > > instead of solely relying on the direct contents of GetViewTree()? (ie. > ensure > > > those ServerViews don't return cloned views via GetChildren()?) > > > > I'm not sure I get what you're asking. SetUpAnimate1 does test views trees. Is > > that what you mean? > > Ah, so really this is a test that *just the vector returned* by GetViewTree > doesn't return cloned views, right? Explicitly using GetView or checking the > members of that vector for their children can expose cloned views? Maybe I'm > still misunderstanding... You got it. GetViewTree is what is passed to clients in some circumstances, which is why it's important it doesn't contain the cloned views. Actually, if a client ends up seeing the cloned view it isn't the end of the world, perhaps just a bit unexpected. https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.h (right): https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.h:121: // WindowManagerInternalClient implementation helper(s); see mojom for On 2014/11/19 01:27:51, msw wrote: > nit: sorry I didn't meant to include the parens around 's', I meant either > "helper;" or "helpers;" Done. https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_apptest.cc (right): https://codereview.chromium.org/720883003/diff/40001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_apptest.cc:174: void CloneAndAnimate(WindowManagerInternalClient* wm, Id view_id) { On 2014/11/19 01:27:51, msw wrote: > optional nit: remove this helper for now? Done.
Just one point left to discuss, sorry. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/connection_manager.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:26: ServerView* clone = new ServerView(delegate, ClonedViewId()); On 2014/11/19 03:28:17, sky wrote: > On 2014/11/19 01:27:51, msw wrote: > > On 2014/11/19 00:49:39, sky wrote: > > > On 2014/11/18 23:37:42, msw wrote: > > > > q: should the clone copy any/all ServerView::properties()? > > > > > > Yes, I missed opacity. No need to visibility as we only clone visible views. > > > > Good catch, but I was wondering about the std::map<std::string, > > std::vector<uint8_t>> ServerView::properties_ > > In theory those shouldn't matter as the only possible use of them is the window > manager, which doesn't see the cloned views anyway. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/connection_manager.cc:323: ReparentClonedViews(view->parent(), &parent_above, view); On 2014/11/19 03:28:17, sky wrote: > On 2014/11/19 01:27:51, msw wrote: > > On 2014/11/19 00:49:39, sky wrote: > > > On 2014/11/18 23:37:42, msw wrote: > > > > I'm confused; does this reparent cloned views to |view|'s old parent? Why > > > > wouldn't they also move to the new parent, and why do they need to bubble > up > > > the > > > > tree at all? Perhaps add a comment here like in > OnWillChangeViewVisibility. > > > > > > Added similar comment to above, let me know if still unclear. > > > > I guess it makes sense, re-parenting an ancestor of the the original view tree > > is likely a precursor to changing its bounds, and the cloned views shouldn't > be > > part of that... I feel like there may be cases where this doesn't make sense, > > like if you're closing a view within a view that's moving/closing? > > I think we'll just have to see how this pans out. I tend to think what I have > here is the right thing, but we'll evaluate as we start adding actual > animations. No doubt a bunch of this will need to change. Acknowledged. https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. On 2014/11/19 03:28:17, sky wrote: > On 2014/11/19 01:27:51, msw wrote: > > On 2014/11/19 00:49:40, sky wrote: > > > On 2014/11/18 23:37:43, msw wrote: > > > > Should this check GetFirstCloned() on the results of GetView() or > > > GetViewTree(), > > > > instead of solely relying on the direct contents of GetViewTree()? (ie. > > ensure > > > > those ServerViews don't return cloned views via GetChildren()?) > > > > > > I'm not sure I get what you're asking. SetUpAnimate1 does test views trees. > Is > > > that what you mean? > > > > Ah, so really this is a test that *just the vector returned* by GetViewTree > > doesn't return cloned views, right? Explicitly using GetView or checking the > > members of that vector for their children can expose cloned views? Maybe I'm > > still misunderstanding... > > You got it. GetViewTree is what is passed to clients in some circumstances, > which is why it's important it doesn't contain the cloned views. Actually, if a > client ends up seeing the cloned view it isn't the end of the world, perhaps > just a bit unexpected. I feel like the name of the test and comments here are somewhat misleading in that regard. Can you try to clarify those? Also, perhaps we should have explicit tests (beyond SetUpAnimate1) that: (1) Use ViewManagerServiceImpl::GetView() to directly access cloned views (perhaps that's not possible if the clone IDs are all the same?) (2) Use ServerView::GetChildren() on views returned by ViewManagerServiceImpl::GetView() to access cloned views. (3) Use ServerView::GetChildren() on views returned by ViewManagerServiceImpl::GetViewTree() to access cloned views. (4) Access cloned views from a connection in other ways?
https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. On 2014/11/19 18:18:29, msw wrote: > On 2014/11/19 03:28:17, sky wrote: > > On 2014/11/19 01:27:51, msw wrote: > > > On 2014/11/19 00:49:40, sky wrote: > > > > On 2014/11/18 23:37:43, msw wrote: > > > > > Should this check GetFirstCloned() on the results of GetView() or > > > > GetViewTree(), > > > > > instead of solely relying on the direct contents of GetViewTree()? (ie. > > > ensure > > > > > those ServerViews don't return cloned views via GetChildren()?) > > > > > > > > I'm not sure I get what you're asking. SetUpAnimate1 does test views > trees. > > Is > > > > that what you mean? > > > > > > Ah, so really this is a test that *just the vector returned* by GetViewTree > > > doesn't return cloned views, right? Explicitly using GetView or checking the > > > members of that vector for their children can expose cloned views? Maybe I'm > > > still misunderstanding... > > > > You got it. GetViewTree is what is passed to clients in some circumstances, > > which is why it's important it doesn't contain the cloned views. Actually, if > a > > client ends up seeing the cloned view it isn't the end of the world, perhaps > > just a bit unexpected. > > I feel like the name of the test and comments here are somewhat misleading in > that regard. Can you try to clarify those? Also, perhaps we should have explicit > tests (beyond SetUpAnimate1) that: > (1) Use ViewManagerServiceImpl::GetView() to directly access cloned views > (perhaps that's not possible if the clone IDs are all the same?) This isn't really interesting as cloned views aren't owned by a connection. > (2) Use ServerView::GetChildren() on views returned by > ViewManagerServiceImpl::GetView() to access cloned views. > (3) Use ServerView::GetChildren() on views returned by > ViewManagerServiceImpl::GetViewTree() to access cloned views. GetFirstCloned() effectively tests these. Assertions for it are covered in SetupAndAnimate1. > (4) Access cloned views from a connection in other ways? These are really the only ways to iterate the tree. The reason GetViewTree is important to test is that it directly corresponds to GetViewTree as defined in the mojom. It's the one case where we want to hide cloned view. ServerView and accessing it is really an implementation of the server. I added the following comment for this test: // Verifies ViewManagerService::GetViewTree() doesn't return cloned views. Any better?
lgtm https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... File mojo/services/view_manager/view_manager_service_unittest.cc (right): https://codereview.chromium.org/720883003/diff/20001/mojo/services/view_manag... mojo/services/view_manager/view_manager_service_unittest.cc:315: // Verify the root doesn't see any cloned views. On 2014/11/19 18:34:53, sky wrote: > On 2014/11/19 18:18:29, msw wrote: > > On 2014/11/19 03:28:17, sky wrote: > > > On 2014/11/19 01:27:51, msw wrote: > > > > On 2014/11/19 00:49:40, sky wrote: > > > > > On 2014/11/18 23:37:43, msw wrote: > > > > > > Should this check GetFirstCloned() on the results of GetView() or > > > > > GetViewTree(), > > > > > > instead of solely relying on the direct contents of GetViewTree()? > (ie. > > > > ensure > > > > > > those ServerViews don't return cloned views via GetChildren()?) > > > > > > > > > > I'm not sure I get what you're asking. SetUpAnimate1 does test views > > trees. > > > Is > > > > > that what you mean? > > > > > > > > Ah, so really this is a test that *just the vector returned* by > GetViewTree > > > > doesn't return cloned views, right? Explicitly using GetView or checking > the > > > > members of that vector for their children can expose cloned views? Maybe > I'm > > > > still misunderstanding... > > > > > > You got it. GetViewTree is what is passed to clients in some circumstances, > > > which is why it's important it doesn't contain the cloned views. Actually, > if > > a > > > client ends up seeing the cloned view it isn't the end of the world, perhaps > > > just a bit unexpected. > > > > I feel like the name of the test and comments here are somewhat misleading in > > that regard. Can you try to clarify those? Also, perhaps we should have > explicit > > tests (beyond SetUpAnimate1) that: > > (1) Use ViewManagerServiceImpl::GetView() to directly access cloned views > > (perhaps that's not possible if the clone IDs are all the same?) > > This isn't really interesting as cloned views aren't owned by a connection. > > > (2) Use ServerView::GetChildren() on views returned by > > ViewManagerServiceImpl::GetView() to access cloned views. > > (3) Use ServerView::GetChildren() on views returned by > > ViewManagerServiceImpl::GetViewTree() to access cloned views. > > > GetFirstCloned() effectively tests these. Assertions for it are covered in > SetupAndAnimate1. > > > (4) Access cloned views from a connection in other ways? > > These are really the only ways to iterate the tree. > > The reason GetViewTree is important to test is that it directly corresponds to > GetViewTree as defined in the mojom. It's the one case where we want to hide > cloned view. ServerView and accessing it is really an implementation of the > server. > > I added the following comment for this test: > > // Verifies ViewManagerService::GetViewTree() doesn't return cloned views. > > Any better? That, combined with my new understanding (clients can't use GetView and only get view IDs from GetViewTree) is all I needed; thanks!
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 364f9e4767069aed16ac1cd87b490ddbbdf0f047 (presubmit successful). |
