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

Issue 2694843005: Mus Demo: Move definition of WindowTreeData into separate files. (Closed)

Created:
10 months ago by fwang
Modified:
10 months ago
Reviewers:
kylechar, tonikitoo
CC:
chromium-reviews, rjkroege
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Mus Demo: Move definition of WindowTreeData into mus_demo.h In follow-up CLs, MusDemo will be split into two derived classes demonstrating external and internal modes. These classes will handle instances of WindowTreeData and for that purpose this CL moves the definition of WindowTreeData into separate files. We also adjust a bit the Constructor/Init to help in future CLs. BUG=666958 R=kylechar@chromium.org Review-Url: https://codereview.chromium.org/2694843005 Cr-Commit-Position: refs/heads/master@{#450726} Committed: https://chromium.googlesource.com/chromium/src/+/da4637b0c789f6360dacdea216dd5316972862ee

Patch Set 1 #

Total comments: 7

Patch Set 2 : Address review comments ; adjust Init for future refactoring #

Total comments: 6

Patch Set 3 : Address review comments #

Total comments: 2

Patch Set 4 : tonikitoo's comments #

Patch Set 5 : Back to signed int again for square size. #

Messages

Total messages: 35 (19 generated)
fwang
PTAL
10 months ago (2017-02-14 16:21:09 UTC) #1
tonikitoo
lgtm
10 months ago (2017-02-14 16:55:19 UTC) #6
kylechar
https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h#newcode54 services/ui/demo/mus_demo.h:54: class WindowTreeData { This is a pretty big class. ...
10 months ago (2017-02-14 16:58:10 UTC) #7
fwang
https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h#newcode54 services/ui/demo/mus_demo.h:54: class WindowTreeData { On 2017/02/14 16:58:10, kylechar wrote: > ...
10 months ago (2017-02-14 17:20:26 UTC) #8
kylechar
Is there a new patch coming? https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h#newcode88 services/ui/demo/mus_demo.h:88: std::unique_ptr<WindowTreeData> window_tree_data_; On ...
10 months ago (2017-02-14 18:27:00 UTC) #9
fwang
On 2017/02/14 18:27:00, kylechar wrote: > Is there a new patch coming? > I won't ...
10 months ago (2017-02-14 20:39:34 UTC) #10
fwang
https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h#newcode54 services/ui/demo/mus_demo.h:54: class WindowTreeData { On 2017/02/14 16:58:10, kylechar wrote: > ...
10 months ago (2017-02-15 13:45:06 UTC) #12
kylechar
lgtm once comments are fixed. https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window_tree_data.cc File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window_tree_data.cc#newcode121 services/ui/demo/window_tree_data.cc:121: } // namespace aura ...
10 months ago (2017-02-15 14:35:06 UTC) #16
fwang
https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window_tree_data.cc File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window_tree_data.cc#newcode121 services/ui/demo/window_tree_data.cc:121: } // namespace aura On 2017/02/15 14:35:06, kylechar wrote: ...
10 months ago (2017-02-15 15:02:53 UTC) #17
tonikitoo
informal l g t m (minor comments, feel free to address if you wish). https://codereview.chromium.org/2694843005/diff/40001/services/ui/demo/window_tree_data.h ...
10 months ago (2017-02-15 15:23:33 UTC) #20
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/2694843005/60001
10 months ago (2017-02-15 15:33:06 UTC) #25
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/da4637b0c789f6360dacdea216dd5316972862ee
10 months ago (2017-02-15 17:01:55 UTC) #28
findit-for-me
FYI: Findit identified this CL at revision 450726 as the culprit for failures in the ...
10 months ago (2017-02-15 18:17:25 UTC) #29
bruthig
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2694413003/ by bruthig@chromium.org. ...
10 months ago (2017-02-15 18:22:04 UTC) #30
tonikitoo
On 2017/02/15 18:17:25, findit-for-me wrote: > FYI: Findit identified this CL at revision 450726 as ...
10 months ago (2017-02-15 18:27:06 UTC) #31
fwang
10 months ago (2017-02-16 06:24:27 UTC) #35
Message was sent while issue was closed.
On 2017/02/15 18:27:06, tonikitoo wrote:
> So the "use of unsigned" actually caused it. I think this is just a bogus
> compiler, but well.

OK, I'm not sure what's the proper way of relanding in Chromium. But I guess
it's safer to open a new CL instead of re-opening that one. Moved to
https://codereview.chromium.org/2697013004/

Powered by Google App Engine
This is Rietveld 0eb02b776