Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(205)

Issue 2700493005: Mus Demo: Refactor code to prepare multiple windows in external mode (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 1 week ago by fwang
Modified:
6 months, 3 weeks ago
Reviewers:
kylechar
CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), tonikitoo
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Mus Demo: Refactor code to prepare multiple windows in external mode Support for external mode in Mus Demo was added in [1]. However, the Mus Demo code is currently designed to work with a single WindowTreeData. This CL refactors the code to be able to handle a list of WindowTreeDatas instead. Actual support for multiple windows will be handled in follow-up CLs. BUG=666958 [1] https://codereview.chromium.org/2622103004/ Review-Url: https://codereview.chromium.org/2700493005 Cr-Commit-Position: refs/heads/master@{#452971} Committed: https://chromium.googlesource.com/chromium/src/+/84358c00a81eab2e2ccc5485fbfe44ffc50307dc

Patch Set 1 #

Patch Set 2 : Mus Demo: Open multiple windows in external mode. #

Patch Set 3 : Implement MusDemo::RemoveWindowTreeData #

Patch Set 4 : Rebase and more cleanup to prepare for Antonio's API change. #

Patch Set 5 : Address some feedback from Antonio. #

Total comments: 25

Patch Set 6 : Address Kylechar's comments #

Total comments: 4

Patch Set 7 : Rename MusExternal::tree_client_ & describes the helper functions for WindowTreeData. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -37 lines) Patch
M services/ui/demo/mus_demo.h View 1 2 3 4 5 6 2 chunks +22 lines, -5 lines 0 comments Download
M services/ui/demo/mus_demo.cc View 1 2 3 4 5 3 chunks +25 lines, -7 lines 0 comments Download
M services/ui/demo/mus_demo_external.h View 1 2 3 4 5 6 1 chunk +6 lines, -2 lines 0 comments Download
M services/ui/demo/mus_demo_external.cc View 1 2 3 4 5 6 1 chunk +45 lines, -13 lines 0 comments Download
M services/ui/demo/mus_demo_internal.h View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M services/ui/demo/mus_demo_internal.cc View 1 2 3 5 2 chunks +12 lines, -8 lines 0 comments Download
M services/ui/demo/window_tree_data.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 21 (12 generated)
fwang
PTAL This is only the mus-demo specific code. Multiple windows will likely need more work ...
7 months ago (2017-02-23 08:25:30 UTC) #10
kylechar
https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_demo.cc#newcode60 services/ui/demo/mus_demo.cc:60: window_tree_data_list_.erase(it, it + 1); You can just do: erase(it). ...
7 months ago (2017-02-23 22:45:30 UTC) #11
fwang
Thank you for the feedback. More explanations inline. As you see, there are follow-up work ...
6 months, 4 weeks ago (2017-02-24 10:08:51 UTC) #12
fwang
PTAL https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_demo.cc#newcode60 services/ui/demo/mus_demo.cc:60: window_tree_data_list_.erase(it, it + 1); On 2017/02/24 10:08:50, fwang ...
6 months, 4 weeks ago (2017-02-24 10:40:11 UTC) #13
kylechar
https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_demo.h#newcode47 services/ui/demo/mus_demo.h:47: void AppendWindowTreeData(std::unique_ptr<WindowTreeData> window_tree_data); Thanks for the explanation on how ...
6 months, 4 weeks ago (2017-02-24 21:14:04 UTC) #14
fwang
https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_demo.h#newcode47 services/ui/demo/mus_demo.h:47: void AppendWindowTreeData(std::unique_ptr<WindowTreeData> window_tree_data); On 2017/02/24 21:14:04, kylechar wrote: > ...
6 months, 4 weeks ago (2017-02-24 21:53:45 UTC) #15
kylechar
lgtm
6 months, 4 weeks ago (2017-02-24 21:55:46 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700493005/140001
6 months, 4 weeks ago (2017-02-24 22:02:10 UTC) #18
commit-bot: I haz the power
6 months, 4 weeks ago (2017-02-24 22:52:25 UTC) #21
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/84358c00a81eab2e2ccc5485fbfe...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b