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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by fwang
Modified:
4 months, 1 week 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
4 months, 2 weeks ago (2017-02-14 16:21:09 UTC) #1
tonikitoo
lgtm
4 months, 2 weeks 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. ...
4 months, 2 weeks 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: > ...
4 months, 2 weeks 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 ...
4 months, 2 weeks 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 ...
4 months, 2 weeks 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: > ...
4 months, 1 week 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 ...
4 months, 1 week 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: ...
4 months, 1 week 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 ...
4 months, 1 week 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
4 months, 1 week 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
4 months, 1 week 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 ...
4 months, 1 week 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. ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-15 18:27:06 UTC) #31
fwang
4 months, 1 week 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/
Sign in to reply to this message.

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