Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693923004:
Mus Demo: Extract code specific to internal mode into a separate class (Closed)
DescriptionMus Demo: Extract code specific to internal mode into a separate class
This CL extracts code from MusDemo into a derived class MusDemoInternal
demonstrating internal mode. In a follow-up CL, another derived class
MusDemoExternal will demonstrate external mode and will be used for
non-ChromeOS Ozone builds. Some helper functions are added to try and
have consistency between internal/external implementations.
BUG=666958
R=kylechar@chromium.org
Review-Url: https://codereview.chromium.org/2693923004
Cr-Commit-Position: refs/heads/master@{#451472}
Committed: https://chromium.googlesource.com/chromium/src/+/88bda6e478572197c7c1dcf6bc6fb0b7b3f1e586
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address review comments and more refactoring #
Total comments: 4
Patch Set 3 : Fix wrong namespace comments #
Dependent Patchsets: Messages
Total messages: 36 (18 generated)
The CQ bit was checked by fwang@igalia.com 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...
fwang@igalia.com changed reviewers: + sky@chromium.org
PTAL
tonikitoo@igalia.com changed reviewers: + skym@chromium.org, tonikitoo@igalia.com - sky@chromium.org
most of the CL l g t m (informally). A piece that I am unsure is whether manifest change (remane to -internal) is ideal. also same with main.cc -> main_internal rename. I will defer comment to others.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_u... File services/ui/demo/mus_demo_unittests.cc (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_u... services/ui/demo/mus_demo_unittests.cc:42: TEST_F(MusDemoTest, CheckMusDemoDraws) { Probably we want to rename the test CheckMusDemoInternalDraws (or alternatively have it check both internal and external modes).
fwang@igalia.com changed reviewers: + sky@chromium.org - skym@chromium.org
https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn File services/ui/demo/BUILD.gn (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn#n... services/ui/demo/BUILD.gn:38: service("mus_demo_internal") { I'm not a big fan of these file and GN target renames from mus_demo to mus_demo_internal. Wouldn't it make more sense for demo to work in internal window mode if is_chromeos=true and external window mode if not? That way you keep mus_demo targets the same but compile the correct version? https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (left): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:106: // aura::WindowManagerDelegate: Remove the includes that were added just for WindowManagerDelegate. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:91: std::unique_ptr<display::ScreenBase> screen_; These must be in the private section. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... services/ui/demo/mus_demo_internal.cc:14: const int kSquareSize = 300; Put this in an anonymous namespace. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... File services/ui/demo/mus_demo_internal.h (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... services/ui/demo/mus_demo_internal.h:8: #include "services/ui/demo/mus_demo.h" Missing imports for WindowManagerDelegate and other symbols used in the header. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... services/ui/demo/mus_demo_internal.h:15: MusDemoInternal(); You're missing a destructor override. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_u... File services/ui/demo/mus_demo_unittests.cc (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_u... services/ui/demo/mus_demo_unittests.cc:42: TEST_F(MusDemoTest, CheckMusDemoDraws) { On 2017/02/14 17:26:05, fwang wrote: > Probably we want to rename the test CheckMusDemoInternalDraws (or alternatively > have it check both internal and external modes). If internal vs external is decided at compile time then it makes sense to leave the test at MusDemoTest and it will test the appropriate thing for the platform? Otherwise, one test fixture MusDemoTest is fine but rename the test case CheckMusDemoInternalDraws.
On 2017/02/14 18:40:11, kylechar wrote: > https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn > File services/ui/demo/BUILD.gn (right): > > https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn#n... > services/ui/demo/BUILD.gn:38: service("mus_demo_internal") { > I'm not a big fan of these file and GN target renames from mus_demo to > mus_demo_internal. Wouldn't it make more sense for demo to work in internal > window mode if is_chromeos=true and external window mode if not? That way you > keep mus_demo targets the same but compile the correct version? +1. This is also my view as per https://codereview.chromium.org/2693923004/#msg7
https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:47: public aura::WindowTreeClientDelegate { so, the resulting MusDemo class still inherits from aura::WindowTreeClientDelegate, but only stubs out its methods? It should be ok for this CL, but maybe in https://codereview.chromium.org/2503923003/ you could also move the inheritance from MusDemo of WindowTreeClientDelegate to MusDemoExternal. Then mus_demo.cc/h would only contain MusDemo (base) declaration, that inherits from service_manager::Service and WindowTreeData class.
@kylechar @tonikitoo: Thanks for the review, I will give another try tomorrow. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn File services/ui/demo/BUILD.gn (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn#n... services/ui/demo/BUILD.gn:38: service("mus_demo_internal") { On 2017/02/14 18:40:11, kylechar wrote: > I'm not a big fan of these file and GN target renames from mus_demo to > mus_demo_internal. Wouldn't it make more sense for demo to work in internal > window mode if is_chromeos=true and external window mode if not? That way you > keep mus_demo targets the same but compile the correct version? I initially wanted to keep all in one class but it seems cleaner to have separate subclasses (e.g. for the window manager specific stuff). But maybe you only mean to do the conditional build in the main.cc? I guess that will work, as long as we do not care about testing both external & internal on a platform. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:47: public aura::WindowTreeClientDelegate { On 2017/02/14 20:00:43, tonikitoo wrote: > so, the resulting MusDemo class still inherits from > aura::WindowTreeClientDelegate, but only stubs out its methods? > I think OnLostConnection is implemented in MusDemo.
https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn File services/ui/demo/BUILD.gn (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/BUILD.gn#n... services/ui/demo/BUILD.gn:38: service("mus_demo_internal") { On 2017/02/14 18:40:11, kylechar wrote: > I'm not a big fan of these file and GN target renames from mus_demo to > mus_demo_internal. Wouldn't it make more sense for demo to work in internal > window mode if is_chromeos=true and external window mode if not? That way you > keep mus_demo targets the same but compile the correct version? Done. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... services/ui/demo/mus_demo_internal.cc:14: const int kSquareSize = 300; On 2017/02/14 18:40:11, kylechar wrote: > Put this in an anonymous namespace. Done. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... File services/ui/demo/mus_demo_internal.h (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... services/ui/demo/mus_demo_internal.h:8: #include "services/ui/demo/mus_demo.h" On 2017/02/14 18:40:11, kylechar wrote: > Missing imports for WindowManagerDelegate and other symbols used in the header. Done. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_i... services/ui/demo/mus_demo_internal.h:15: MusDemoInternal(); On 2017/02/14 18:40:11, kylechar wrote: > You're missing a destructor override. Done. https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_u... File services/ui/demo/mus_demo_unittests.cc (right): https://codereview.chromium.org/2693923004/diff/1/services/ui/demo/mus_demo_u... services/ui/demo/mus_demo_unittests.cc:42: TEST_F(MusDemoTest, CheckMusDemoDraws) { On 2017/02/14 18:40:11, kylechar wrote: > On 2017/02/14 17:26:05, fwang wrote: > > Probably we want to rename the test CheckMusDemoInternalDraws (or > alternatively > > have it check both internal and external modes). > > If internal vs external is decided at compile time then it makes sense to leave > the test at MusDemoTest and it will test the appropriate thing for the platform? > Otherwise, one test fixture MusDemoTest is fine but rename the test case > CheckMusDemoInternalDraws. Acknowledged.
Description was changed from ========== Mus Demo: Extract code specific to internal mode into a separate class This CL extracts code from MusDemo into a derived class MusDemoInternal demonstrating internal mode. The mus_demo service is renamed mus_demo_internal. In a follow-up CL, another derived class MusDemoExternal will demonstrate external mode and will correspond to a another mus_demo_external service tested by mus_demo_unitests. BUG=666958 R=kylechar@chromium.org ========== to ========== Mus Demo: Extract code specific to internal mode into a separate class This CL extracts code from MusDemo into a derived class MusDemoInternal demonstrating internal mode. In a follow-up CL, another derived class MusDemoExternal will demonstrate external mode and will be used for non-ChromeOS Ozone builds. BUG=666958 R=kylechar@chromium.org ==========
The CQ bit was checked by fwang@igalia.com 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 ========== Mus Demo: Extract code specific to internal mode into a separate class This CL extracts code from MusDemo into a derived class MusDemoInternal demonstrating internal mode. In a follow-up CL, another derived class MusDemoExternal will demonstrate external mode and will be used for non-ChromeOS Ozone builds. BUG=666958 R=kylechar@chromium.org ========== to ========== Mus Demo: Extract code specific to internal mode into a separate class This CL extracts code from MusDemo into a derived class MusDemoInternal demonstrating internal mode. In a follow-up CL, another derived class MusDemoExternal will demonstrate external mode and will be used for non-ChromeOS Ozone builds. Some helper functions are added to try and have consistency between internal/external implementations. BUG=666958 R=kylechar@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
informal l g t m (feel free to change int to unsigned, for the size, as in the other CL).
https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo_internal.cc:18: const int kSquareSize = 300; per Antonio's comment, I'll change this to unsigned after the next round of feedback.
fwang@igalia.com changed reviewers: - sky@chromium.org
https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo_internal.cc:18: const int kSquareSize = 300; On 2017/02/15 17:10:02, fwang wrote: > per Antonio's comment, I'll change this to unsigned after the next round of > feedback. So finally I better not: https://codereview.chromium.org/2694843005/#msg31
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:85: } // namespace aura Can you fix the end namespace comment? It's aura instead of ui in all of the files.
https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2693923004/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:85: } // namespace aura On 2017/02/16 16:27:38, kylechar wrote: > Can you fix the end namespace comment? It's aura instead of ui in all of the > files. Done.
lgtm
The CQ bit was checked by fwang@igalia.com
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": 60001, "attempt_start_ts": 1487432593732300,
"parent_rev": "4f5240898acda6b0c928e743ea422db1184ac07b", "commit_rev":
"88bda6e478572197c7c1dcf6bc6fb0b7b3f1e586"}
Message was sent while issue was closed.
Description was changed from ========== Mus Demo: Extract code specific to internal mode into a separate class This CL extracts code from MusDemo into a derived class MusDemoInternal demonstrating internal mode. In a follow-up CL, another derived class MusDemoExternal will demonstrate external mode and will be used for non-ChromeOS Ozone builds. Some helper functions are added to try and have consistency between internal/external implementations. BUG=666958 R=kylechar@chromium.org ========== to ========== Mus Demo: Extract code specific to internal mode into a separate class This CL extracts code from MusDemo into a derived class MusDemoInternal demonstrating internal mode. In a follow-up CL, another derived class MusDemoExternal will demonstrate external mode and will be used for non-ChromeOS Ozone builds. Some helper functions are added to try and have consistency between internal/external implementations. BUG=666958 R=kylechar@chromium.org Review-Url: https://codereview.chromium.org/2693923004 Cr-Commit-Position: refs/heads/master@{#451472} Committed: https://chromium.googlesource.com/chromium/src/+/88bda6e478572197c7c1dcf6bc6f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/88bda6e478572197c7c1dcf6bc6f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
