Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2679213003:
Refactor Mus demo to prepare support for multiple external windows (Closed)
DescriptionRefactor Mus demo to prepare support for multiple external windows
In order to demonstrate multiple external windows (with their own
spinnig square), the Mus demo must store information for each of the
window. This CL groups the relevant WindowTreeHost-specific data into
a separate class, so that it will be more convenient to handle
them. In particular, the initialization, cleanup and animation of
the data are moved into that class.
BUG=666958
Review-Url: https://codereview.chromium.org/2679213003
Cr-Commit-Position: refs/heads/master@{#449356}
Committed: https://chromium.googlesource.com/chromium/src/+/5a530bcea5b4fcadadb6fa6af191f91c880a06b1
Patch Set 1 #Patch Set 2 : Use a class instead of a struct to improve data separation. #
Total comments: 3
Messages
Total messages: 25 (14 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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ========== Refactor Mus demo to prepare support for multiple external windows In order to demonstrate multiple external windows (with their own spinnig square), the Mus demo must store information for each of the window. This CL groups the relevant WindowTreeHost-specific data into a separate structure, so that it will be more convenient to handle them. Also, the initialization of the data is moved into a separate BeginDrawingFrames function that could be shared between the OnEmbed and OnWmNewDisplay calls. BUG=666958 ========== to ========== Refactor Mus demo to prepare support for multiple external windows In order to demonstrate multiple external windows (with their own spinnig square), the Mus demo must store information for each of the window. This CL groups the relevant WindowTreeHost-specific data into a separate class, so that it will be more convenient to handle them. In particular, the initialization, cleanup and animation of the data are moved into that class. BUG=666958 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/08 16:10:29, fwang wrote: > PTAL Differently from Thomas' original approach in [1], Fred and I agreed to make the WindowTreeData class a non-POD class. In the end, code got simpler too. informal l g t m. [1] https://codereview.chromium.org/2622103004/
fwang@igalia.com changed reviewers: + kylechar@chromium.org, rjkroege@chromium.org
lgtm in a follow-on patch (doing it separately probably makes review easier) can you move the child subclass implementation together in the source file.
On 2017/02/09 17:38:55, rjkroege wrote: > lgtm > > in a follow-on patch (doing it separately probably makes review easier) can you > move the child subclass implementation together in the source file. As far as I understand, in external window mode there is no WindowManager so you shouldn't make a connection over that Mojo API. So I'd think you'd need a different mus demo implementation that doesn't implement WindowManagerDelegate? I might be missing some context from the Ozone wayland meeting though.
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...
On 2017/02/09 17:38:55, rjkroege wrote: > lgtm > > in a follow-on patch (doing it separately probably makes review easier) can you > move the child subclass implementation together in the source file. Sure. I wanted to do that but that would have made review harder. I'm landing this now and will come back to it tomorrow.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486664253189110,
"parent_rev": "2f1469d2244ff0e9342e4b3f0b698e91600cb42a", "commit_rev":
"5a530bcea5b4fcadadb6fa6af191f91c880a06b1"}
Message was sent while issue was closed.
Description was changed from ========== Refactor Mus demo to prepare support for multiple external windows In order to demonstrate multiple external windows (with their own spinnig square), the Mus demo must store information for each of the window. This CL groups the relevant WindowTreeHost-specific data into a separate class, so that it will be more convenient to handle them. In particular, the initialization, cleanup and animation of the data are moved into that class. BUG=666958 ========== to ========== Refactor Mus demo to prepare support for multiple external windows In order to demonstrate multiple external windows (with their own spinnig square), the Mus demo must store information for each of the window. This CL groups the relevant WindowTreeHost-specific data into a separate class, so that it will be more convenient to handle them. In particular, the initialization, cleanup and animation of the data are moved into that class. BUG=666958 Review-Url: https://codereview.chromium.org/2679213003 Cr-Commit-Position: refs/heads/master@{#449356} Committed: https://chromium.googlesource.com/chromium/src/+/5a530bcea5b4fcadadb6fa6af191... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/5a530bcea5b4fcadadb6fa6af191...
Message was sent while issue was closed.
https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:84: aura::Window* root_window_ = nullptr; Why do you need to cache this? window_tree_host_->window() gives you the root. https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:87: std::unique_ptr<aura::Window> bitmap_window_; Windows are generally owned by their parent. Are you sure you need a unique_ptr here? https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:100: }; DISALLOW...
Message was sent while issue was closed.
On 2017/02/09 20:53:16, sky wrote: > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... > File services/ui/demo/mus_demo.cc (right): > > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:84: aura::Window* root_window_ = nullptr; > Why do you need to cache this? window_tree_host_->window() gives you the root. > > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:87: std::unique_ptr<aura::Window> bitmap_window_; > Windows are generally owned by their parent. Are you sure you need a unique_ptr > here? > > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:100: }; > DISALLOW... @sky: Thank you for the feedback. This CL was just moving existing code from MusDemo. I was wondering the same for root_window_ and bitmap_window_ and I saw Tom's original patch removed the last_draw_frame_time_ thing. I'll try doing these in a follow-up CL.
Message was sent while issue was closed.
SG On Thu, Feb 9, 2017 at 1:05 PM, <fwang@igalia.com> wrote: > On 2017/02/09 20:53:16, sky wrote: >> > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... >> File services/ui/demo/mus_demo.cc (right): >> >> > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... >> services/ui/demo/mus_demo.cc:84: aura::Window* root_window_ = nullptr; >> Why do you need to cache this? window_tree_host_->window() gives you the >> root. >> >> > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... >> services/ui/demo/mus_demo.cc:87: std::unique_ptr<aura::Window> >> bitmap_window_; >> Windows are generally owned by their parent. Are you sure you need a > unique_ptr >> here? >> >> > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_de... >> services/ui/demo/mus_demo.cc:100: }; >> DISALLOW... > > @sky: Thank you for the feedback. This CL was just moving existing code from > MusDemo. I was wondering the same for root_window_ and bitmap_window_ and I > saw > Tom's original patch removed the last_draw_frame_time_ thing. I'll try doing > these in a follow-up CL. > > https://codereview.chromium.org/2679213003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
