Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(482)

Issue 2679213003: Refactor Mus demo to prepare support for multiple external windows (Closed)

Created:
3 years, 10 months ago by fwang
Modified:
3 years, 10 months ago
Reviewers:
rjkroege, kylechar, sky
CC:
chromium-reviews, rjkroege, Tom (Use chromium acct)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/5a530bcea5b4fcadadb6fa6af191f91c880a06b1

Patch Set 1 #

Patch Set 2 : Use a class instead of a struct to improve data separation. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -28 lines) Patch
M services/ui/demo/mus_demo.h View 1 3 chunks +2 lines, -23 lines 0 comments Download
M services/ui/demo/mus_demo.cc View 1 5 chunks +45 lines, -5 lines 3 comments Download

Messages

Total messages: 25 (14 generated)
fwang
PTAL
3 years, 10 months ago (2017-02-08 16:10:29 UTC) #4
tonikitoo
On 2017/02/08 16:10:29, fwang wrote: > PTAL Differently from Thomas' original approach in [1], Fred ...
3 years, 10 months ago (2017-02-08 17:12:28 UTC) #12
fwang
3 years, 10 months ago (2017-02-09 07:02:40 UTC) #14
rjkroege
lgtm in a follow-on patch (doing it separately probably makes review easier) can you move ...
3 years, 10 months ago (2017-02-09 17:38:55 UTC) #15
kylechar
On 2017/02/09 17:38:55, rjkroege wrote: > lgtm > > in a follow-on patch (doing it ...
3 years, 10 months ago (2017-02-09 17:57:02 UTC) #16
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/2679213003/20001
3 years, 10 months ago (2017-02-09 18:18:12 UTC) #18
fwang
On 2017/02/09 17:38:55, rjkroege wrote: > lgtm > > in a follow-on patch (doing it ...
3 years, 10 months ago (2017-02-09 18:18:22 UTC) #19
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/5a530bcea5b4fcadadb6fa6af191f91c880a06b1
3 years, 10 months ago (2017-02-09 18:31:14 UTC) #22
sky
https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_demo.cc#newcode84 services/ui/demo/mus_demo.cc:84: aura::Window* root_window_ = nullptr; Why do you need to ...
3 years, 10 months ago (2017-02-09 20:53:16 UTC) #23
fwang
On 2017/02/09 20:53:16, sky wrote: > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_demo.cc > File services/ui/demo/mus_demo.cc (right): > > https://codereview.chromium.org/2679213003/diff/20001/services/ui/demo/mus_demo.cc#newcode84 > ...
3 years, 10 months ago (2017-02-09 21:05:45 UTC) #24
sky
3 years, 10 months ago (2017-02-09 21:06:52 UTC) #25
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.

Powered by Google App Engine
This is Rietveld 408576698