Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737213002:
Update window_parenting_utils to use aura::window (Closed)
DescriptionUpdate window_parenting_utils to use aura::window instead of WmWindow.
BUG=687657
Review-Url: https://codereview.chromium.org/2737213002
Cr-Commit-Position: refs/heads/master@{#456080}
Committed: https://chromium.googlesource.com/chromium/src/+/7aa2519f44df0777bb5e457ed01cc4788a931705
Patch Set 1 #Patch Set 2 : fix #
Total comments: 4
Patch Set 3 : address comments #
Total comments: 4
Patch Set 4 : fix nits #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== refactor update wm_window_utils BUG= ========== to ========== Update window_parenting_utils to use aura::window instead of WmWindow. BUG=687657 ==========
The CQ bit was checked by yiyix@chromium.org 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...
yiyix@chromium.org changed reviewers: + sky@chromium.org
@sky, could you please take a look at this patch?
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by yiyix@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Could you sort out the compile issues and I'll take a look? I say that as I suspect you're going to end up touching more files.
On 2017/03/09 03:18:10, sky wrote: > Could you sort out the compile issues and I'll take a look? I say that as I > suspect you're going to end up touching more files. Sorry, I compiled locally and i asked you for reviewer without realizing there is compiling problems. I will remove you from reviewer list for now.
yiyix@chromium.org changed reviewers: - sky@chromium.org
The CQ bit was checked by yiyix@chromium.org 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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by yiyix@chromium.org 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...
No need to review me from the reviewers list. Just ping the patch once it's updated. -Scott On Thu, Mar 9, 2017 at 8:21 AM, <yiyix@chromium.org> wrote: > On 2017/03/09 03:18:10, sky wrote: >> Could you sort out the compile issues and I'll take a look? I say that as >> I >> suspect you're going to end up touching more files. > > Sorry, I compiled locally and i asked you for reviewer without realizing > there > is compiling problems. I will remove you from reviewer list for now. > > https://codereview.chromium.org/2737213002/ -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/09 17:26:23, sky wrote: > No need to review me from the reviewers list. Just ping the patch once > it's updated. > > -Scott > > On Thu, Mar 9, 2017 at 8:21 AM, <mailto:yiyix@chromium.org> wrote: > > On 2017/03/09 03:18:10, sky wrote: > >> Could you sort out the compile issues and I'll take a look? I say that as > >> I > >> suspect you're going to end up touching more files. > > > > Sorry, I compiled locally and i asked you for reviewer without realizing > > there > > is compiling problems. I will remove you from reviewer list for now. > > > > https://codereview.chromium.org/2737213002/ > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > It's ready :) could you please review it?
https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/default_s... File ash/common/wm/default_state.cc (right): https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/default_s... ash/common/wm/default_state.cc:705: window->GetParent()->aura_window(), It isn't readily obvious to me if GetParent() is non-null in all these situations. I think it's safer to do window->aura_window()->parent(). This is assuming you know window is non-null, which I believe it is. Please do this change in the other files as well. https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/dock/dock... File ash/common/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:188: old_parent->aura_window(), Similar comment about nullness for old_parent and GetParent().
@sky, Could you please take another look? https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/default_s... File ash/common/wm/default_state.cc (right): https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/default_s... ash/common/wm/default_state.cc:705: window->GetParent()->aura_window(), On 2017/03/09 20:57:45, sky wrote: > It isn't readily obvious to me if GetParent() is non-null in all these > situations. I think it's safer to do window->aura_window()->parent(). This is > assuming you know window is non-null, which I believe it is. Please do this > change in the other files as well. It is good to have as safe guard. Added. https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/dock/dock... File ash/common/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/2737213002/diff/20001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:188: old_parent->aura_window(), On 2017/03/09 20:57:45, sky wrote: > Similar comment about nullness for old_parent and GetParent(). Done.
https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... File ash/common/wm/dock/docked_window_resizer.cc (right): https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_resizer.cc:271: window->GetParent()->aura_window(), window->parent()->aura_window()? https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... File ash/common/wm/panels/panel_window_resizer.cc (right): https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... ash/common/wm/panels/panel_window_resizer.cc:188: ; Not needed.
@sky, could you take another look? Thank you. https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... File ash/common/wm/dock/docked_window_resizer.cc (right): https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_resizer.cc:271: window->GetParent()->aura_window(), On 2017/03/09 23:31:00, sky wrote: > window->parent()->aura_window()? so sorry, i missed this one. https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... File ash/common/wm/panels/panel_window_resizer.cc (right): https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... ash/common/wm/panels/panel_window_resizer.cc:188: ; On 2017/03/09 23:31:00, sky wrote: > Not needed. Done.
On 2017/03/10 00:55:57, yiyix wrote: > @sky, could you take another look? Thank you. > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... > File ash/common/wm/dock/docked_window_resizer.cc (right): > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... > ash/common/wm/dock/docked_window_resizer.cc:271: > window->GetParent()->aura_window(), > On 2017/03/09 23:31:00, sky wrote: > > window->parent()->aura_window()? > > so sorry, i missed this one. > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... > File ash/common/wm/panels/panel_window_resizer.cc (right): > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... > ash/common/wm/panels/panel_window_resizer.cc:188: ; > On 2017/03/09 23:31:00, sky wrote: > > Not needed. > > Done. AFAICT the patch is the same? Did you forget to upload a new one?
On 2017/03/10 01:00:08, sky wrote: > On 2017/03/10 00:55:57, yiyix wrote: > > @sky, could you take another look? Thank you. > > > > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... > > File ash/common/wm/dock/docked_window_resizer.cc (right): > > > > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/dock/dock... > > ash/common/wm/dock/docked_window_resizer.cc:271: > > window->GetParent()->aura_window(), > > On 2017/03/09 23:31:00, sky wrote: > > > window->parent()->aura_window()? > > > > so sorry, i missed this one. > > > > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... > > File ash/common/wm/panels/panel_window_resizer.cc (right): > > > > > https://codereview.chromium.org/2737213002/diff/40001/ash/common/wm/panels/pa... > > ash/common/wm/panels/panel_window_resizer.cc:188: ; > > On 2017/03/09 23:31:00, sky wrote: > > > Not needed. > > > > Done. > > AFAICT the patch is the same? Did you forget to upload a new one? OMG, i was working on last minute perf. OMG, will not happening again.
LGTM
The CQ bit was checked by yiyix@chromium.org
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": 1489162086894520,
"parent_rev": "6c77b2f4468702c9d4fc551d6e7bb3fbf25c2922", "commit_rev":
"7aa2519f44df0777bb5e457ed01cc4788a931705"}
Message was sent while issue was closed.
Description was changed from ========== Update window_parenting_utils to use aura::window instead of WmWindow. BUG=687657 ========== to ========== Update window_parenting_utils to use aura::window instead of WmWindow. BUG=687657 Review-Url: https://codereview.chromium.org/2737213002 Cr-Commit-Position: refs/heads/master@{#456080} Committed: https://chromium.googlesource.com/chromium/src/+/7aa2519f44df0777bb5e457ed01c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7aa2519f44df0777bb5e457ed01c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
