Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)
     aura_unittests

Committed: https://crrev.com/d02d32f9400a58ea0c9e5beb1ecc25f5beefe484
Cr-Commit-Position: refs/heads/master@{#440224}

[riajiang, 2016-12-15 21:30:26]

Patchset #1 (id:1) has been deleted

[riajiang, 2016-12-15 21:30:33]

Patchset #1 (id:20001) has been deleted

[riajiang, 2016-12-15 21:32:13]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[riajiang, 2016-12-15 21:32:14]

Hi sadrul@ and sky@, PTAL. Thanks!

[sky, 2016-12-15 22:07:32]

Can you write tests for this too?

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.h (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.h:136: // Applies to BOUNDS. This should be in
pixels.
Name the member as such to reinforce that, e.g. bounds_in_pixels.

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1352: const gfx::Rect& transit_bounds) {
Rename to transit_bounds_in_pixels.

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1369:
gfx::ConvertRectToPixel(device_scale_factor, bounds_in_dip));
I think you should fix 672151 before this, otherwise I'm a bit worried about
rounding errors leading to weird bugs. By that I mean the conversion from pixels
-> dip -> pixels won't always give the same value, leading to confusion about
the actual bounds.

[sadrul, 2016-12-16 18:14:08]

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:928: window->GetWindow()->bounds()) != bounds)
Some conversions are happening in WindowPortMus, and some here in
WindowTreeClient. Would it be possible to keep all the conversion in one place?

https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1034: window_tree_host_->GetBoundsInPixels());
DCHECK_EQ

[chromium-reviews, 2016-12-16 19:45:23]

We can keep all conversions in WindowTreeClient, but then
ScopedServerChange would be in DIP and WindowPortMus::SetBoundsFromServer
would take in DIP. I think "server" implies pixels... wdyt? Should we
change these two names?

On Fri, Dec 16, 2016, 1:14 PM <sadrul@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
> File ui/aura/mus/window_tree_client.cc (right):
>
>
>
https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
> ui/aura/mus/window_tree_client.cc:928: window->GetWindow()->bounds()) !=
> bounds)
> Some conversions are happening in WindowPortMus, and some here in
> WindowTreeClient. Would it be possible to keep all the conversion in one
> place?
>
>
>
https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
> File ui/views/mus/native_widget_mus.cc (right):
>
>
>
https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
> ui/views/mus/native_widget_mus.cc:1034:
> window_tree_host_->GetBoundsInPixels());
> DCHECK_EQ
>
> https://codereview.chromium.org/2579883002/
>

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

[sky, 2016-12-16 20:37:03]

I agree with Sadrul. If possible centralize the conversion in
WindowTreeClient. The less places that have to do conversion, the less
error prone.

On Fri, Dec 16, 2016 at 11:45 AM, 'Ria Jiang' via Chromium-reviews
<chromium-reviews@chromium.org> wrote:
> We can keep all conversions in WindowTreeClient, but then ScopedServerChange
> would be in DIP and WindowPortMus::SetBoundsFromServer would take in DIP. I
> think "server" implies pixels... wdyt? Should we change these two names?
>
>
> On Fri, Dec 16, 2016, 1:14 PM <sadrul@chromium.org> wrote:
>>
>>
>>
>>
https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
>> File ui/aura/mus/window_tree_client.cc (right):
>>
>>
>>
https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
>> ui/aura/mus/window_tree_client.cc:928: window->GetWindow()->bounds()) !=
>> bounds)
>> Some conversions are happening in WindowPortMus, and some here in
>> WindowTreeClient. Would it be possible to keep all the conversion in one
>> place?
>>
>>
>>
https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
>> File ui/views/mus/native_widget_mus.cc (right):
>>
>>
>>
https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
>> ui/views/mus/native_widget_mus.cc:1034:
>> window_tree_host_->GetBoundsInPixels());
>> DCHECK_EQ
>>
>> https://codereview.chromium.org/2579883002/

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

[kylechar, 2016-12-19 18:28:20]

Description was changed from

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)
==========

to

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)
==========

[riajiang, 2016-12-19 23:49:50]

Patchset #2 (id:60001) has been deleted

[riajiang, 2016-12-19 23:51:55]

Also added tests. PTAL. Thanks!

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.h (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.h:136: // Applies to BOUNDS. This should be in
pixels.
On 2016/12/15 22:07:32, sky wrote:
> Name the member as such to reinforce that, e.g. bounds_in_pixels.

In order to move conversions to WTC, this now takes in bounds_in_dip.

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:928: window->GetWindow()->bounds()) != bounds)
On 2016/12/16 18:14:08, sadrul wrote:
> Some conversions are happening in WindowPortMus, and some here in
> WindowTreeClient. Would it be possible to keep all the conversion in one
place?

Moved all conversions in WTC. ServerChangeData now takes bounds_in_dip.

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1352: const gfx::Rect& transit_bounds) {
On 2016/12/15 22:07:32, sky wrote:
> Rename to transit_bounds_in_pixels.

Done.

https://codereview.chromium.org/2579883002/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1369:
gfx::ConvertRectToPixel(device_scale_factor, bounds_in_dip));
On 2016/12/15 22:07:32, sky wrote:
> I think you should fix 672151 before this, otherwise I'm a bit worried about
> rounding errors leading to weird bugs. By that I mean the conversion from
pixels
> -> dip -> pixels won't always give the same value, leading to confusion about
> the actual bounds.

As discussed, ignoring this comment.

https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2579883002/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1034: window_tree_host_->GetBoundsInPixels());
On 2016/12/16 18:14:08, sadrul wrote:
> DCHECK_EQ

Done.

[riajiang, 2016-12-19 23:56:20]

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

[commit-bot: I haz the power, 2016-12-19 23:57:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 01:00:57]

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

[commit-bot: I haz the power, 2016-12-20 01:00:57]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-21 16:22:45]

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:539: const gfx::Rect& revert_bounds) {
Let's call this revert_bounds_in_pixels

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:541:
GetWindowTreeHostMus(window)->SetBoundsFromServer(revert_bounds);
Add a comment here that WindowTreeHost expects bounds to be in pixels.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:651: ScheduleInFlightBoundsChange(
  float device_scale_factor = ScaleFactorForDisplay(window->GetWindow());

then use that for the conversions below.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1540: offset.y() * device_scale_factor,
Use gfx::ScaleVector2d(offset, device_scale_factor)

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_host_mus.cc:70: ->GetDisplayNearestWindow(window())
Does GetDisplayNearestWindow() return the correct display? Should you use
|display_id_| to look up the display instead?

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_host_mus.h (right):

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_host_mus.h:55: void SetBoundsFromServer(const gfx::Rect&
bounds);
bounds_in_pixels

[riajiang, 2016-12-21 17:47:39]

Patchset #4 (id:120001) has been deleted

[riajiang, 2016-12-21 17:48:33]

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:539: const gfx::Rect& revert_bounds) {
On 2016/12/21 16:22:44, sadrul wrote:
> Let's call this revert_bounds_in_pixels

Done.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:541:
GetWindowTreeHostMus(window)->SetBoundsFromServer(revert_bounds);
On 2016/12/21 16:22:44, sadrul wrote:
> Add a comment here that WindowTreeHost expects bounds to be in pixels.

Done.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:651: ScheduleInFlightBoundsChange(
On 2016/12/21 16:22:44, sadrul wrote:
>   float device_scale_factor = ScaleFactorForDisplay(window->GetWindow());
> 
> then use that for the conversions below.

Done.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1540: offset.y() * device_scale_factor,
On 2016/12/21 16:22:44, sadrul wrote:
> Use gfx::ScaleVector2d(offset, device_scale_factor)

Done.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_host_mus.cc:70: ->GetDisplayNearestWindow(window())
On 2016/12/21 16:22:44, sadrul wrote:
> Does GetDisplayNearestWindow() return the correct display? Should you use
> |display_id_| to look up the display instead?

As discussed offline, changed to use |display_id_| to get the DSF.

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_host_mus.h (right):

https://codereview.chromium.org/2579883002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_host_mus.h:55: void SetBoundsFromServer(const gfx::Rect&
bounds);
On 2016/12/21 16:22:44, sadrul wrote:
> bounds_in_pixels

Done.

[sadrul, 2016-12-21 19:30:10]

lgtm

[riajiang, 2016-12-21 19:32:43]

The CQ bit was checked by riajiang@chromium.org

[commit-bot: I haz the power, 2016-12-21 19:33:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 21:34:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482348763123710,
"parent_rev": "922fdc7ea9ab5d4bcd9ee8ad05ee31d2ae48143f", "commit_rev":
"92c3efd6c8186f61f918aef76bd4aa8c45b1bb2a"}

[commit-bot: I haz the power, 2016-12-21 21:34:42]

Description was changed from

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)
==========

to

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2579883002
==========

[commit-bot: I haz the power, 2016-12-21 21:34:44]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-12-21 21:37:16]

Description was changed from

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2579883002
==========

to

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)

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

[commit-bot: I haz the power, 2016-12-21 21:37:17]

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

[riajiang1, 2016-12-21 21:51:09]

Description was changed from

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)

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

to

==========
Scale bounds in aura-mus client-lib and NativeWidgetMus in hdpi mode.

Also use the correct device scale factor in WindowTreeHostMus.

BUG=646942
TEST=manual (with --force-device-scale-factor=2)
     aura_unittests

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