Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694843005:
Mus Demo: Move definition of WindowTreeData into separate files. (Closed)
DescriptionMus 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)
PTAL
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
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... services/ui/demo/mus_demo.h:54: class WindowTreeData { This is a pretty big class. Is there any reason it can't go in it's own file? https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:88: std::unique_ptr<WindowTreeData> window_tree_data_; Move to private section.
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... services/ui/demo/mus_demo.h:54: class WindowTreeData { On 2017/02/14 16:58:10, kylechar wrote: > This is a pretty big class. Is there any reason it can't go in it's own file? I guess so. I was not sure either :-) https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:88: std::unique_ptr<WindowTreeData> window_tree_data_; On 2017/02/14 16:58:10, kylechar wrote: > Move to private section. Yes, this will be needed later (see https://codereview.chromium.org/2693923004/ and https://codereview.chromium.org/2622103004/) but I did this move in that commit as it was more logical.
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... services/ui/demo/mus_demo.h:88: std::unique_ptr<WindowTreeData> window_tree_data_; On 2017/02/14 17:20:26, fwang wrote: > On 2017/02/14 16:58:10, kylechar wrote: > > Move to private section. > > Yes, this will be needed later (see https://codereview.chromium.org/2693923004/ > and https://codereview.chromium.org/2622103004/) but I did this move in that > commit as it was more logical. It should be in the private section regardless. If a subclass needs to use it then a protected accessor method can be added. https://google.github.io/styleguide/cppguide.html#Access_Control
On 2017/02/14 18:27:00, kylechar wrote: > Is there a new patch coming? > I won't be back to this until tomorrow. But maybe we do not want that at the end if we keep all in mus_demo.cpp
Description was changed from ========== 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 the mus_demo header. Additionally, this CL moves WindowTreeData::DrawFrame as that was not done in [1]. [1] https://codereview.chromium.org/2688123002/ BUG=666958 R=kylechar@chromium.org ========== to ========== 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 ==========
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... services/ui/demo/mus_demo.h:54: class WindowTreeData { On 2017/02/14 16:58:10, kylechar wrote: > This is a pretty big class. Is there any reason it can't go in it's own file? Done. https://codereview.chromium.org/2694843005/diff/1/services/ui/demo/mus_demo.h... services/ui/demo/mus_demo.h:88: std::unique_ptr<WindowTreeData> window_tree_data_; On 2017/02/14 16:58:10, kylechar wrote: > Move to private section. Done.
Description was changed from ========== 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 ========== to ========== 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 ==========
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...
lgtm once comments are fixed. https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... services/ui/demo/window_tree_data.cc:121: } // namespace aura // namespace ui https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... services/ui/demo/window_tree_data.h:26: virtual ~WindowTreeData(); Will this become a base class that needs a virtual destructor? Just double checking. https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... services/ui/demo/window_tree_data.h:58: } // namespace aura // namespace ui
https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... services/ui/demo/window_tree_data.cc:121: } // namespace aura On 2017/02/15 14:35:06, kylechar wrote: > // namespace ui Acknowledged. https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... services/ui/demo/window_tree_data.h:26: virtual ~WindowTreeData(); On 2017/02/15 14:35:06, kylechar wrote: > Will this become a base class that needs a virtual destructor? Just double > checking. Mmh, no. I guess I should remove the virtual keyword. https://codereview.chromium.org/2694843005/diff/20001/services/ui/demo/window... services/ui/demo/window_tree_data.h:58: } // namespace aura On 2017/02/15 14:35:06, kylechar wrote: > // namespace ui Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2694843005/diff/40001/services/ui/demo/window... services/ui/demo/window_tree_data.h:30: bool IsInitialized() { return !!window_tree_host_; } minor: I like the idea of using 'const' to methods like this. https://codereview.chromium.org/2694843005/diff/40001/services/ui/demo/window... services/ui/demo/window_tree_data.h:52: const int square_size_; minor: unsigned? (now that we are touching it)? well, if it will be compared against signed int's and it will warn, then leave it alone.
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...
The CQ bit was checked by fwang@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from tonikitoo@igalia.com, kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2694843005/#ps60001 (title: "tonikitoo's comments")
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": 1487172766657530,
"parent_rev": "085d96d2ce61f416d03f95f228196f55746e63ed", "commit_rev":
"da4637b0c789f6360dacdea216dd5316972862ee"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/da4637b0c789f6360dacdea216dd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/da4637b0c789f6360dacdea216dd...
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 450726 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2694413003/ by bruthig@chromium.org. The reason for reverting is: This change appears to have broken the compile step of the following build: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/8536.
Message was sent while issue was closed.
On 2017/02/15 18:17:25, findit-for-me wrote: > FYI: Findit identified this CL at revision 450726 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... ninja -t msvc -e environment.x64 -- C:\b\c\cipd\goma/gomacc.exe (..) c:\b\c\b\win_x64_archive\src\services\ui\demo\window_tree_data.cc(37): error C2220: warning treated as error - no 'object' file generated c:\b\c\b\win_x64_archive\src\services\ui\demo\window_tree_data.cc(37): warning C4146: unary minus operator applied to unsigned type, result still unsigned [26140/48744] CXX obj/components/keyed_service/content/content/browser_context_keyed_base_factory.obj So the "use of unsigned" actually caused it. I think this is just a bogus compiler, but well. void DrawSquare(const gfx::Rect& bounds, double angle, SkCanvas* canvas, unsigned size) { // Create SkRect to draw centered inside the bounds. gfx::Point top_left = bounds.CenterPoint(); top_left.Offset(-size / 2, -size / 2); <-----------
Message was sent while issue was closed.
Description was changed from ========== 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/+/da4637b0c789f6360dacdea216dd... ========== to ========== 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/+/da4637b0c789f6360dacdea216dd... ==========
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...
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/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
