Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967543002:
Move ShowState-related code from PlatformWindowMus to NativeWidgetMus (Closed)
|
Created:
4 years, 7 months ago by Mark Dittmer Modified:
4 years, 7 months ago Reviewers:
sadrul CC:
chromium-reviews, tfarina, Fady Samuel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ShowState-related code from PlatformWindowMus to NativeWidgetMus
R=sadrul@chromium.org
BUG=609555
Committed: https://crrev.com/71c7728405a2e8ff0ffae53cab82605c0aae5176
Cr-Commit-Position: refs/heads/master@{#393251}
Patch Set 1 #Patch Set 2 : Update logic according to changed ShowState enum #Patch Set 3 : Update logic according to changed ShowState enum #Patch Set 4 : Cast to PlatformWindowDelegate for OnWindowStateChanged #
Total comments: 8
Patch Set 5 : Address code review comments from sadrul #
Total comments: 4
Patch Set 6 : Drop WTHM::show_state #Patch Set 7 : Refactor show_state checks #
Messages
Total messages: 46 (16 generated)
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Update logic according to changed ShowState enum
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Update logic according to changed ShowState enum
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Cast to PlatformWindowDelegate for OnWindowStateChanged
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/60001
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 markdittmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
You should also remove WindowTreeHostMus::show_state_. https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:900: window_tree_host_->platform_window()->Maximize(); Remove these calls to platform_window() https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:914: window_tree_host_->show_state() == ui::PLATFORM_WINDOW_STATE_MAXIMIZED; We should look at |mus_window_obserer_->show_state()|, instead of depending on WTH here (ditto for IsMinimized()) https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.h (right): https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.h:199: void SetShowState(mus::mojom::ShowState show_state); This needs to move up (or down in private). Non-overrides before overrides.
https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:365: return static_cast<ui::PlatformWindowDelegate*>( btw: this static_cast<> isn't necessary, is it?
Address code review comments from sadrul
All comments addressed. https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:365: return static_cast<ui::PlatformWindowDelegate*>( On 2016/05/11 05:35:33, sadrul wrote: > btw: this static_cast<> isn't necessary, is it? It is not. I was thinking downcast when it's an upcast. https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:900: window_tree_host_->platform_window()->Maximize(); On 2016/05/11 05:12:47, sadrul wrote: > Remove these calls to platform_window() Done. https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:914: window_tree_host_->show_state() == ui::PLATFORM_WINDOW_STATE_MAXIMIZED; On 2016/05/11 05:12:47, sadrul wrote: > We should look at |mus_window_obserer_->show_state()|, instead of depending on > WTH here (ditto for IsMinimized()) Done. https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.h (right): https://codereview.chromium.org/1967543002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.h:199: void SetShowState(mus::mojom::ShowState show_state); On 2016/05/11 05:12:47, sadrul wrote: > This needs to move up (or down in private). Non-overrides before overrides. Done.
https://codereview.chromium.org/1967543002/diff/80001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1967543002/diff/80001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:910: mus_window_observer_->show_state() == ui::PLATFORM_WINDOW_STATE_MINIMIZED; Make sure to add show_state() accessor to the observer? https://codereview.chromium.org/1967543002/diff/80001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:921: show_state_before_fullscreen_ = window_tree_host_->show_state(); Get rid of WindowTreeHostMus::show_state
Drop WTHM::show_state
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Refactor show_state checks
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
https://codereview.chromium.org/1967543002/diff/80001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1967543002/diff/80001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:910: mus_window_observer_->show_state() == ui::PLATFORM_WINDOW_STATE_MINIMIZED; On 2016/05/11 14:18:20, sadrul wrote: > Make sure to add show_state() accessor to the observer? Done. https://codereview.chromium.org/1967543002/diff/80001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:921: show_state_before_fullscreen_ = window_tree_host_->show_state(); On 2016/05/11 14:18:20, sadrul wrote: > Get rid of WindowTreeHostMus::show_state Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by markdittmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967543002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967543002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move ShowState-related code from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 ========== to ========== Move ShowState-related code from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 Committed: https://crrev.com/71c7728405a2e8ff0ffae53cab82605c0aae5176 Cr-Commit-Position: refs/heads/master@{#393251} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/71c7728405a2e8ff0ffae53cab82605c0aae5176 Cr-Commit-Position: refs/heads/master@{#393251} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
