Finish eliminating PlatformWindowMus

Finish eliminating PlatformWindowMus

R=sadrul@chromium.org
BUG=609555
Committed: https://crrev.com/bb0c85e1aaa882aa7c4c40b0e3da609573f07d8f
Cr-Commit-Position: refs/heads/master@{#396745}

[Mark Dittmer, 2016-05-25 20:44:07]

From wth->SetBounds to observer on OnWindowBoundsChanging

[sadrul, 2016-05-26 01:31:01]

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (left):

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:893:
window_tree_host_->SetBounds(gfx::Rect(bounds.origin(), size));
Would it make sense to keep this here, but to override OnBoundsChanged() in
WindowTreeHostMus and make it a no-op?

Also, can you add some tests for this?

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus_unittest.cc (left):

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:472: 
We should keep these tests.

[Mark Dittmer, 2016-05-26 13:37:33]

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (left):

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:893:
window_tree_host_->SetBounds(gfx::Rect(bounds.origin(), size));
On 2016/05/26 01:31:01, sadrul wrote:
> Would it make sense to keep this here, but to override OnBoundsChanged() in
> WindowTreeHostMus and make it a no-op?
> 
> Also, can you add some tests for this?

I have to admit I don't understand the suggestion. I will say that implementing
the visitor method (new code line 416) ensures that window_->SetBounds will
trigger window_tree_host_->SetBounds, and that other contexts invoking this
mus::Window's SetBounds will do the same. Failing the latter appears to have
caused the mash wireframe bug for which my previous CL was rolled back.

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus_unittest.cc (left):

https://codereview.chromium.org/2009853002/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:472: 
On 2016/05/26 01:31:01, sadrul wrote:
> We should keep these tests.

Done.

[Mark Dittmer, 2016-05-26 13:37:41]

Resurrect unittests

[Mark Dittmer, 2016-05-26 14:09:52]

Add set bounds bypassing widget unittest

[Mark Dittmer, 2016-05-27 13:48:20]

Latest patch includes wireframe fix via observer, and a unittest that catches
the regression in the previous CL. sadrul@chromium.org: PTAL.

[Mark Dittmer, 2016-05-27 13:51:56]

markdittmer@chromium.org changed reviewers:
+ nyquist@chromium.org, sky@chromium.org

[Mark Dittmer, 2016-05-27 13:51:56]

nyquist@chromium.org: Please review changes in blimp (same as approved in
https://codereview.chromium.org/1999083002).

sky@chromium.org: Please review changes in ash (same as approved in
https://codereview.chromium.org/1999083002).

[Mark Dittmer, 2016-05-27 13:51:58]

nyquist@chromium.org: Please review changes in blimp (same as approved in
https://codereview.chromium.org/1999083002).

sky@chromium.org: Please review changes in ash (same as approved in
https://codereview.chromium.org/1999083002).

[Mark Dittmer, 2016-05-27 15:00:13]

rebase

[sky, 2016-05-27 15:45:02]

On Fri, May 27, 2016 at 6:51 AM,  <markdittmer@chromium.org> wrote:
> nyquist@chromium.org: Please review changes in blimp (same as approved in
> https://codereview.chromium.org/1999083002).
>
> sky@chromium.org: Please review changes in ash (same as approved in
> https://codereview.chromium.org/1999083002).

I don't see any changes to ash files in this patch?

>
>
> https://codereview.chromium.org/2009853002/

-- 
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.

[nyquist, 2016-05-27 17:23:40]

Did something happen with your rebase? Seems to be no changes to //blimp either.

[Mark Dittmer, 2016-05-27 18:57:15]

rebase

[Mark Dittmer, 2016-05-27 18:58:53]

Sorry, branch management issue. nyquist@chromium.org, sky@chromium.org: PTAL.

[sky, 2016-05-27 21:44:07]

LGTM

[sadrul, 2016-05-30 14:42:11]

https://codereview.chromium.org/2009853002/diff/100001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2009853002/diff/100001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:41:
OnAcceleratedWidgetAvailable(accelerated_widget, 1.f);
Since we are doing OnAcceleratedWidgetAvailable() explicitly here, and do not
depend on StubWindow() to tell us, we should no longer need to actually tell
StubWindow about the AcceleratedWidget either, right?

https://codereview.chromium.org/2009853002/diff/100001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:41:
OnAcceleratedWidgetAvailable(accelerated_widget, 1.f);
Add a TODO here to find the correct device-scale-factor from |window|.

[Mark Dittmer, 2016-05-30 15:18:45]

Clean-up and add TODO to WTHM

[Mark Dittmer, 2016-05-30 15:24:28]

https://codereview.chromium.org/2009853002/diff/100001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2009853002/diff/100001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:41:
OnAcceleratedWidgetAvailable(accelerated_widget, 1.f);
On 2016/05/30 14:42:11, sadrul wrote:
> Add a TODO here to find the correct device-scale-factor from |window|.

Done.

https://codereview.chromium.org/2009853002/diff/100001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:41:
OnAcceleratedWidgetAvailable(accelerated_widget, 1.f);
On 2016/05/30 14:42:11, sadrul wrote:
> Since we are doing OnAcceleratedWidgetAvailable() explicitly here, and do not
> depend on StubWindow() to tell us, we should no longer need to actually tell
> StubWindow about the AcceleratedWidget either, right?

Done.

[Mark Dittmer, 2016-05-30 15:35:58]

The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-30 15:36:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009853002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009853002/120001

[commit-bot: I haz the power, 2016-05-30 15:38:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-30 15:38:02]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[sadrul, 2016-05-30 15:44:45]

A couple of comments. lgtm with those addressed

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (left):

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:893:
window_tree_host_->SetBounds(gfx::Rect(bounds.origin(), size));
This can actually still stay here, right? (although it isn't necessary, since
the call to mus::Window::SetBounds() will synchronously trigger
OnWindowBoundsChanging() on the observer, and we will set the bounds again). It
can be a little confusing I think. Can you add a comment here to explain this
and a DCHECK() that window_tree_host_->bounds() == window_->bounds()?

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:57: new ui::StubWindow(nullptr)));
This might fit in a line? (make sure to do 'git cl format')

[Mark Dittmer, 2016-05-30 16:01:03]

rebase

[Mark Dittmer, 2016-05-30 16:14:52]

Add DCHECK for bounds change; git cl format

[Mark Dittmer, 2016-05-30 16:14:55]

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (left):

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:893:
window_tree_host_->SetBounds(gfx::Rect(bounds.origin(), size));
On 2016/05/30 15:44:45, sadrul wrote:
> This can actually still stay here, right? (although it isn't necessary, since
> the call to mus::Window::SetBounds() will synchronously trigger
> OnWindowBoundsChanging() on the observer, and we will set the bounds again).
It
> can be a little confusing I think. Can you add a comment here to explain this
> and a DCHECK() that window_tree_host_->bounds() == window_->bounds()?

Done.

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2009853002/diff/120001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:57: new ui::StubWindow(nullptr)));
On 2016/05/30 15:44:45, sadrul wrote:
> This might fit in a line? (make sure to do 'git cl format')

Done.

[Mark Dittmer, 2016-05-30 16:15:08]

The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-30 16:15:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009853002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009853002/160001

[commit-bot: I haz the power, 2016-05-30 17:24:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-30 17:24:44]

Dry run: This issue passed the CQ dry run.

[Mark Dittmer, 2016-05-30 17:41:58]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-05-30 17:41:59]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2009853002/#ps160001
(title: "Add DCHECK for bounds change; git cl format")

[commit-bot: I haz the power, 2016-05-30 17:42:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009853002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009853002/160001

[commit-bot: I haz the power, 2016-05-30 17:45:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-30 17:47:13]

Description was changed from

==========
Finish eliminating PlatformWindowMus

R=sadrul@chromium.org
BUG=609555
==========

to

==========
Finish eliminating PlatformWindowMus

R=sadrul@chromium.org
BUG=609555

Committed: https://crrev.com/bb0c85e1aaa882aa7c4c40b0e3da609573f07d8f
Cr-Commit-Position: refs/heads/master@{#396745}
==========

[commit-bot: I haz the power, 2016-05-30 17:47:15]

Patchset 9 (id:??) landed as
https://crrev.com/bb0c85e1aaa882aa7c4c40b0e3da609573f07d8f
Cr-Commit-Position: refs/heads/master@{#396745}

[msw, 2016-06-02 20:18:45]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-06-02 20:18:46]

Hey Mark, this broke the visibility of the wallpaper picker/manager window in
mash with my WIP CL applied:
  https://codereview.chromium.org/1984433002
(the window simply doesn't appear, but its shelf icon does)
I'll take a closer look, but lmk if you have any tips.

[Mark Dittmer, 2016-06-03 12:53:20]

On 2016/06/02 20:18:46, msw wrote:
> Hey Mark, this broke the visibility of the wallpaper picker/manager window in
> mash with my WIP CL applied:
>   https://codereview.chromium.org/1984433002
> (the window simply doesn't appear, but its shelf icon does)
> I'll take a closer look, but lmk if you have any tips.

I'm not sure what would be causing this. My best guess is that something is
setting the aura window bounds without going through NativeWidgetMus. I notice
that this CL introduces a mus::WindowObserver to update aura when the mus window
changes, but no observer on the aura window to update the mus window. I'm not
certain that this is the source of the issue, but since get/set bounds is the
only behaviour that was left in PlatformWindowMus (deleted in this CL), that's
my best guess.

[sky, 2016-06-03 16:13:46]

I also noticed that I no longer see a chrome window when starting
chrome --mash. It may be related to the same issue.

  -Scott

On Fri, Jun 3, 2016 at 5:53 AM,  <markdittmer@chromium.org> wrote:
> On 2016/06/02 20:18:46, msw wrote:
>> Hey Mark, this broke the visibility of the wallpaper picker/manager window
>> in
>> mash with my WIP CL applied:
>> https://codereview.chromium.org/1984433002
>> (the window simply doesn't appear, but its shelf icon does)
>> I'll take a closer look, but lmk if you have any tips.
>
> I'm not sure what would be causing this. My best guess is that something is
> setting the aura window bounds without going through NativeWidgetMus. I
> notice
> that this CL introduces a mus::WindowObserver to update aura when the mus
> window
> changes, but no observer on the aura window to update the mus window. I'm
> not
> certain that this is the source of the issue, but since get/set bounds is
> the
> only behaviour that was left in PlatformWindowMus (deleted in this CL),
> that's
> my best guess.
>
> https://codereview.chromium.org/2009853002/

-- 
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.

[msw, 2016-06-03 19:11:46]

On 2016/06/03 16:13:46, sky wrote:
> I also noticed that I no longer see a chrome window when starting
> chrome --mash. It may be related to the same issue.

I see Chrome okay, so that's likely unrelated, but this is definitely a bounds
issue. Adding |platform_window()->SetBounds(window->bounds());| at
window_tree_host_mus.cc:57 makes the window appear initially, but it can't be
moved, etc. I'm trying to plumb bounds between WindowTreeHostMus and
mus::Window*, but not having much luck; I'm not sure how this is supposed to
work...

[sky, 2016-06-03 21:22:12]

I figured out my problem. The chrome manifest wasn't getting built so
that chrome was never launching.

On Fri, Jun 3, 2016 at 9:13 AM, Scott Violet <sky@chromium.org> wrote:
> I also noticed that I no longer see a chrome window when starting
> chrome --mash. It may be related to the same issue.
>
>   -Scott
>
> On Fri, Jun 3, 2016 at 5:53 AM,  <markdittmer@chromium.org> wrote:
>> On 2016/06/02 20:18:46, msw wrote:
>>> Hey Mark, this broke the visibility of the wallpaper picker/manager window
>>> in
>>> mash with my WIP CL applied:
>>> https://codereview.chromium.org/1984433002
>>> (the window simply doesn't appear, but its shelf icon does)
>>> I'll take a closer look, but lmk if you have any tips.
>>
>> I'm not sure what would be causing this. My best guess is that something is
>> setting the aura window bounds without going through NativeWidgetMus. I
>> notice
>> that this CL introduces a mus::WindowObserver to update aura when the mus
>> window
>> changes, but no observer on the aura window to update the mus window. I'm
>> not
>> certain that this is the source of the issue, but since get/set bounds is
>> the
>> only behaviour that was left in PlatformWindowMus (deleted in this CL),
>> that's
>> my best guess.
>>
>> https://codereview.chromium.org/2009853002/

-- 
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.