Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688013003:
Mus Demo: Code cleanup in MusDemo::WindowTreeData (Closed)
DescriptionMus Demo: Code cleanup in MusDemo::WindowTreeData
This CL performs the following cleanup:
* The logging of the time past since the last frame drawing is removed.
* The root_window_ and bitmap_window_ members are removed since they
can be accessed from the window_tree_host_. Actually, only the bitmap
window is now handled outside the initialization function and a
helper function is added to retrieve it.
* The destructor is removed since the cleanup is done when members are
destroyed: The bitmap window is owned by its parent itself owned by
WindowTreeData::window_tree_host_ and WindowTreeData::timer_
is stopped in base::Timer::~Timer().
* Add missing DISALLOW_COPY_AND_ASSIGN to WindowTreeData.
* Use proper coordinates for the bitmap window operations.
BUG=666958
R=sky@chromium.org
Review-Url: https://codereview.chromium.org/2688013003
Cr-Commit-Position: refs/heads/master@{#450023}
Committed: https://chromium.googlesource.com/chromium/src/+/01d9b79fa9fda1529d3c1d09ae1eb0f0d68140c7
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address sky's review comments. #
Total comments: 1
Patch Set 3 : Final fix before landing. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 23 (16 generated)
Description was changed from ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window are now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ member and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). BUG=666958 R=sky@chromium.org ========== to ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ member and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). BUG=666958 R=sky@chromium.org ==========
Description was changed from ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ member and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). BUG=666958 R=sky@chromium.org ========== to ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). BUG=666958 R=sky@chromium.org ==========
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.
https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:85: aura::Window* BitmapWindow() { GetBitmapWindow(), or as this is inlined and cheap, bitmap_window(). https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:86: return window_tree_host_->window()->children()[0]; DCHECK(!children.empty()) https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:100: }; DISALLOW... https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:114: bitmap_window->SetBounds(root_window->bounds()); bitmap_window should have the same size, but an origin of 0x0, e.g. gfx::Rect(root_window->bounds().size()); https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:272: BitmapWindow()->SchedulePaintInRect(BitmapWindow()->bounds()); SchedulePaintInRect() takes a rectangle in the coordinates of the view you are calling paint on. You are passing the bounds of the same window. Bounds are in the coordinates of the parent. In other words you have a coordinate system mismatch here. It doesn't really matter in this case since you know the origin is 0x0, but it's good practice to do the right thing in case that changes, which is window->SchedulePaintInRect(gfx::Rect(window->bounds().size());
Description was changed from ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). BUG=666958 R=sky@chromium.org ========== to ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). * Add missing DISALLOW_COPY_AND_ASSIGN to WindowTreeData. * Use proper coordinates for the bitmap window operations. BUG=666958 R=sky@chromium.org ==========
Thank you for the review. Comments addressed. https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.cc File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:85: aura::Window* BitmapWindow() { On 2017/02/10 18:48:37, sky wrote: > GetBitmapWindow(), or as this is inlined and cheap, bitmap_window(). Done. https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:86: return window_tree_host_->window()->children()[0]; On 2017/02/10 18:48:38, sky wrote: > DCHECK(!children.empty()) Done. https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:100: }; On 2017/02/10 18:48:38, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:114: bitmap_window->SetBounds(root_window->bounds()); On 2017/02/10 18:48:38, sky wrote: > bitmap_window should have the same size, but an origin of 0x0, e.g. > gfx::Rect(root_window->bounds().size()); Done. https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... services/ui/demo/mus_demo.cc:272: BitmapWindow()->SchedulePaintInRect(BitmapWindow()->bounds()); On 2017/02/10 18:48:37, sky wrote: > SchedulePaintInRect() takes a rectangle in the coordinates of the view you are > calling paint on. You are passing the bounds of the same window. Bounds are in > the coordinates of the parent. In other words you have a coordinate system > mismatch here. It doesn't really matter in this case since you know the origin > is 0x0, but it's good practice to do the right thing in case that changes, which > is window->SchedulePaintInRect(gfx::Rect(window->bounds().size()); Done.
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.
On 2017/02/11 09:58:48, fwang wrote: > Thank you for the review. Comments addressed. > > https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.cc > File services/ui/demo/mus_demo.cc (right): > > https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... > services/ui/demo/mus_demo.cc:85: aura::Window* BitmapWindow() { > On 2017/02/10 18:48:37, sky wrote: > > GetBitmapWindow(), or as this is inlined and cheap, bitmap_window(). > > Done. > > https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... > services/ui/demo/mus_demo.cc:86: return > window_tree_host_->window()->children()[0]; > On 2017/02/10 18:48:38, sky wrote: > > DCHECK(!children.empty()) > > Done. > > https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... > services/ui/demo/mus_demo.cc:100: }; > On 2017/02/10 18:48:38, sky wrote: > > DISALLOW... > > Done. > > https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... > services/ui/demo/mus_demo.cc:114: > bitmap_window->SetBounds(root_window->bounds()); > On 2017/02/10 18:48:38, sky wrote: > > bitmap_window should have the same size, but an origin of 0x0, e.g. > > gfx::Rect(root_window->bounds().size()); > > Done. > > https://codereview.chromium.org/2688013003/diff/1/services/ui/demo/mus_demo.c... > services/ui/demo/mus_demo.cc:272: > BitmapWindow()->SchedulePaintInRect(BitmapWindow()->bounds()); > On 2017/02/10 18:48:37, sky wrote: > > SchedulePaintInRect() takes a rectangle in the coordinates of the view you are > > calling paint on. You are passing the bounds of the same window. Bounds are in > > the coordinates of the parent. In other words you have a coordinate system > > mismatch here. It doesn't really matter in this case since you know the origin > > is 0x0, but it's good practice to do the right thing in case that changes, > which > > is window->SchedulePaintInRect(gfx::Rect(window->bounds().size()); > > Done. informal l g t m
LGTM https://codereview.chromium.org/2688013003/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2688013003/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:276: gfx::Rect(bitmap_window()->bounds().size())); gfx::Rect(bounds.size());
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by fwang@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2688013003/#ps60001 (title: "Final fix before landing.")
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": 1487009550086050,
"parent_rev": "6ed406b794156c16f82ba5d76776381acaeb8f33", "commit_rev":
"01d9b79fa9fda1529d3c1d09ae1eb0f0d68140c7"}
Message was sent while issue was closed.
Description was changed from ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). * Add missing DISALLOW_COPY_AND_ASSIGN to WindowTreeData. * Use proper coordinates for the bitmap window operations. BUG=666958 R=sky@chromium.org ========== to ========== Mus Demo: Code cleanup in MusDemo::WindowTreeData This CL performs the following cleanup: * The logging of the time past since the last frame drawing is removed. * The root_window_ and bitmap_window_ members are removed since they can be accessed from the window_tree_host_. Actually, only the bitmap window is now handled outside the initialization function and a helper function is added to retrieve it. * The destructor is removed since the cleanup is done when members are destroyed: The bitmap window is owned by its parent itself owned by WindowTreeData::window_tree_host_ and WindowTreeData::timer_ is stopped in base::Timer::~Timer(). * Add missing DISALLOW_COPY_AND_ASSIGN to WindowTreeData. * Use proper coordinates for the bitmap window operations. BUG=666958 R=sky@chromium.org Review-Url: https://codereview.chromium.org/2688013003 Cr-Commit-Position: refs/heads/master@{#450023} Committed: https://chromium.googlesource.com/chromium/src/+/01d9b79fa9fda1529d3c1d09ae1e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/01d9b79fa9fda1529d3c1d09ae1e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
