MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.

MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.
Add a cross-platform unit-test for that functionality.

TEST=views_unittests --gtest_filter="WidgetTest.OnDeviceScaleFactorChanged"
BUG=415024
Committed: https://crrev.com/ba587a918ca859045437211ebca414e08f61320b
Cr-Commit-Position: refs/heads/master@{#366573}

[Andrey Kraynov, 2015-12-17 15:40:50]

iceman@yandex-team.ru changed reviewers:
+ tapted@chromium.org

[Andrey Kraynov, 2015-12-17 15:40:51]

Hi!

I see a lot of 'Not implemented reached' messages when running
views_examples_with_content_exe on Mac. It seems to be non critical for
views_examples to work, but I hope that it can be fixed.

Here is my first attempt to fix some NOTIMPLEMENTED lines.
Could you take a look please? Is it correct?
If yes, then I can move further.

Thanks in advance!

[tapted, 2015-12-17 22:38:20]

Thanks for taking an interest! But these need some tests.

Part of the reason these were NOTIMPLEMENTED() for so long because there's no
test coverage for these! (on any platform). The approach to bringing things up
has mostly been to run views_unittests, see what fails, and implement stuff
until it stops failing. (all views_unittests did all pass on Mac at one point,
but some ~5 new tests added aren't passing on Mac yet).

I've sketched out a cross-platform test for DeviceScaleFactorChanged - I think
it's pretty straightforward, but I haven't compiled/run any of it so there may
be some traps lurking.

https://codereview.chromium.org/1533883002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1533883002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:876:
native_widget_mac_->GetWidget()->DeviceScaleFactorChanged(
E.g. here a good test would be in widget_unittest.cc, something like

class ScaleFactorView : public View {
..
  // Overridden from ui::LayerDelegate:
  void OnDeviceScaleFactorChanged(float device_scale_factor) override {
    last_scale_factor_ = device_scale_factor;
    View::OnDeviceScaleFactorChanged(device_scale_factor);
  }
  float last_scale_factor() const { return last_scale_factor_; };
..
 private:
  float last_scale_factor_ = 0;

  DISALLOW..etc
};

TEST_F(WidgetTest, OnDeviceScaleFactorChanged) {
  Widget* widget = CreateTopLevelPlatformWidget();
  ScaleFactorView* view = new ScaleFactorView;  
  widget->GetContentsView->AddChildView(view);
  
  // Adding the view should notify the view about the initial scale factor.
  EXPECT_EQ(1.0f, view->last_scale_factor());

  // Changes should be propagated.
  widget->GetLayer()->OnDeviceScaleFactorChanged(2.0f);
  EXPECT_EQ(2.0f, view->last_scale_factor());
  
  widget->CloseNow();
}

https://codereview.chromium.org/1533883002/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1533883002/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac.mm:484: bridge_->layer()->SetOpacity(opacity /
255.0);
I don't know if this is correct. E.g. DesktopWindowTreeHostX11::SetOpacity(..)
uses X11 methods to change the opacity.

On Mac, we probably want to do

[GetNativeWindow() setAlphaValue:opacity / 255.0];

Let's move this to another CL - I might need to play with it a bit (and it will
need a test too :)

[Andrey Kraynov, 2015-12-18 13:51:59]

I greatly appreciate your help with that, thank you for detailed answer!

https://codereview.chromium.org/1533883002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1533883002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:876:
native_widget_mac_->GetWidget()->DeviceScaleFactorChanged(
OK, I've tried to run this test.
But it seems that assumption that the parent view should notify the child view
about the initial scale factor
fails both on Win and Mac. First expect in this test always fails.

Could you point me to the code that leads to expected behavior?
Is it a bug that should be fixed?

https://codereview.chromium.org/1533883002/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1533883002/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac.mm:484: bridge_->layer()->SetOpacity(opacity /
255.0);
On 2015/12/17 22:38:20, tapted wrote:
> I don't know if this is correct. E.g. DesktopWindowTreeHostX11::SetOpacity(..)
> uses X11 methods to change the opacity.
> 
> On Mac, we probably want to do
> 
> [GetNativeWindow() setAlphaValue:opacity / 255.0];
> 
> Let's move this to another CL - I might need to play with it a bit (and it
will
> need a test too :)

OK, I reverted my changes in this file.

[tapted, 2015-12-21 07:19:51]

On 2015/12/18 13:51:59, Andrey Kraynov wrote:
> I greatly appreciate your help with that, thank you for detailed answer!
> 
>
https://codereview.chromium.org/1533883002/diff/1/ui/views/cocoa/bridged_nati...
> File ui/views/cocoa/bridged_native_widget.mm (right):
> 
>
https://codereview.chromium.org/1533883002/diff/1/ui/views/cocoa/bridged_nati...
> ui/views/cocoa/bridged_native_widget.mm:876:
> native_widget_mac_->GetWidget()->DeviceScaleFactorChanged(
> OK, I've tried to run this test.
> But it seems that assumption that the parent view should notify the child view
> about the initial scale factor
> fails both on Win and Mac. First expect in this test always fails.
> 
> Could you point me to the code that leads to expected behavior?
> Is it a bug that should be fixed?

Hm - it's unintuitive, but it looks like things that currently depend on it will
not care. Label overrides it to invalidate a cached RenderText, but it's invalid
at the start anyway, so that's not a problem. View's own implementation
overrides it in order to implement SnapLayerToPixelBoundary(), but that's a
no-op for views that are not layer-backed (and not an issue since the call to
the View's override of LayerDelegate::OnDeviceScaleFactorChanged() will come
from its own layer anyway).

[tapted, 2015-12-21 07:20:16]

https://codereview.chromium.org/1533883002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1533883002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
CL description: can now say, e.g. "MacViews: Implement
BridgedNativeWidget::OnDeviceScaleFactorChanged(..)"

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3427: TEST_F(WidgetTest,
OnDeviceScaleFactorChanged) {
nit: comment describing the test like
// Ensure scale factor changes are propagated from the native Widget.

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3429: scoped_ptr<Widget, WidgetCloser>
widget(CreateTopLevelPlatformWidget());
ooh neat. we should do this more. WidgetCloser probably belongs in widget_test.h
but that's a good follow-up. (Perhaps the CreateFooWidget(..) methods should
even return a scoped_ptr..)

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3434: // Adding the view should notify the
view about the initial scale factor.
I guess

  // For views that are not layer-backed, adding the view won't notify the view
  // about the initial scale factor. Fake it.
  widget->GetLayer()->OnDeviceScaleFactorChanged(scale_factor);

[tapted, 2015-12-21 07:23:49]

and lgtm after those changes (and one more)

You'll need a ui/views owner for the cross-platform test (I think sadrul is
still around)

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3432: float scale_factor =
widget->GetLayer()->device_scale_factor();
heh, I guess we should check that this isn't 0 as well, or everything could be
"working"

[Andrey Kraynov, 2015-12-21 09:21:17]

Description was changed from

==========
MacViews: Implement few BridgedNativeWidget and NativeWidgetMac methods.

BUG=415024
==========

to

==========
MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.
Add a cross-platform unit-test for that functionality.

TEST=views_unittests --gtest_filter="WidgetTest.OnDeviceScaleFactorChanged"
BUG=415024
==========

[Andrey Kraynov, 2015-12-21 09:47:19]

I tried to fix your remarks.
Could you take a look again, please?
If everything is OK, then I will add an owner for widget_unittest.cc file.
Thanks!

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3427: TEST_F(WidgetTest,
OnDeviceScaleFactorChanged) {
On 2015/12/21 07:20:16, tapted wrote:
> nit: comment describing the test like
> // Ensure scale factor changes are propagated from the native Widget.

Done.

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3429: scoped_ptr<Widget, WidgetCloser>
widget(CreateTopLevelPlatformWidget());
On 2015/12/21 07:20:16, tapted wrote:
> ooh neat. we should do this more. WidgetCloser probably belongs in
widget_test.h
> but that's a good follow-up. (Perhaps the CreateFooWidget(..) methods should
> even return a scoped_ptr..)

IMHO, this line can be much better with implemented make_scoped_ptr(ptr,
deleter) function, like
auto scoped = make_scoped_ptr(CreateFooWidget(), [](Widget* w){ w->CloseNow();
});

A colleague of mine already proposed a CL with that function,
https://codereview.chromium.org/1036133003/
How do you think, can it be usefull?

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3432: float scale_factor =
widget->GetLayer()->device_scale_factor();
On 2015/12/21 07:23:49, tapted wrote:
> heh, I guess we should check that this isn't 0 as well, or everything could be
> "working"

Done.

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3434: // Adding the view should notify the
view about the initial scale factor.
On 2015/12/21 07:20:16, tapted wrote:
> I guess
> 
>   // For views that are not layer-backed, adding the view won't notify the
view
>   // about the initial scale factor. Fake it.
>   widget->GetLayer()->OnDeviceScaleFactorChanged(scale_factor);

Unfortunately, ui::Layer class compares scale factor value with cached value and
returns.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...
I used a direct call to ScaleFactorView::OnDeviceScaleFactorChanged function.
After that EXPECT looks strange. How do you think, is it OK?

[tapted, 2015-12-21 22:54:11]

lgtm - thanks!

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3429: scoped_ptr<Widget, WidgetCloser>
widget(CreateTopLevelPlatformWidget());
On 2015/12/21 09:47:19, Andrey Kraynov wrote:
> On 2015/12/21 07:20:16, tapted wrote:
> > ooh neat. we should do this more. WidgetCloser probably belongs in
> widget_test.h
> > but that's a good follow-up. (Perhaps the CreateFooWidget(..) methods should
> > even return a scoped_ptr..)
> 
> IMHO, this line can be much better with implemented make_scoped_ptr(ptr,
> deleter) function, like
> auto scoped = make_scoped_ptr(CreateFooWidget(), [](Widget* w){ w->CloseNow();
> });
> 
> A colleague of mine already proposed a CL with that function,
> https://codereview.chromium.org/1036133003/
> How do you think, can it be usefull?

For this example, I think it's more useful if CreateTopLevelPlatformWidget()
returned the scoped_ptr, in which case it would be a stretch to use a lambda
since you need a return type. Then the line here would just be

auto widget = CreateTopLevelPlatformWidget();

But there may be cases where returning a scoped_ptr in specific tests breaks
down. E.g. those testing other ways to close, so it would need the right balance
to ensure those tests don't lose readability.

https://codereview.chromium.org/1533883002/diff/40001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3434: // Adding the view should notify the
view about the initial scale factor.
On 2015/12/21 09:47:19, Andrey Kraynov wrote:
> On 2015/12/21 07:20:16, tapted wrote:
> > I guess
> > 
> >   // For views that are not layer-backed, adding the view won't notify the
> view
> >   // about the initial scale factor. Fake it.
> >   widget->GetLayer()->OnDeviceScaleFactorChanged(scale_factor);
> 
> Unfortunately, ui::Layer class compares scale factor value with cached value
and
> returns.
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...
> I used a direct call to ScaleFactorView::OnDeviceScaleFactorChanged function.
> After that EXPECT looks strange. How do you think, is it OK?

yep - I think that's fine. Just takes "faking it" to another level :)

[Andrey Kraynov, 2015-12-22 08:04:32]

iceman@yandex-team.ru changed reviewers:
+ sadrul@chromium.org

[Andrey Kraynov, 2015-12-22 08:04:33]

+ sadrul@ as an owner for widget_test.cc file.
PTAL

[sadrul, 2015-12-22 08:13:34]

lgtm

https://codereview.chromium.org/1533883002/diff/60001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/1533883002/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3405: ScaleFactorView() = default;
How is this different from 'ScaleFactorView() {}'?

https://codereview.chromium.org/1533883002/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3423: };
Hm, we should be using this in more tests above.

[Andrey Kraynov, 2015-12-22 08:38:57]

Thanks!

https://codereview.chromium.org/1533883002/diff/60001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/1533883002/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3405: ScaleFactorView() = default;
On 2015/12/22 08:13:34, sadrul (slow until new year) wrote:
> How is this different from 'ScaleFactorView() {}'?

Both forms of ctor will behave the same here.
But '= default' syntax have some bonuses, there is nice post at
http://stackoverflow.com/questions/20828907/the-new-keyword-default-in-c11
And I prefer using it if possible.

https://codereview.chromium.org/1533883002/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:3423: };
On 2015/12/22 08:13:34, sadrul (slow until new year) wrote:
> Hm, we should be using this in more tests above.

Don't you mind if I will create a CL to use this pattern in widget_unittest.cc
file?

[Andrey Kraynov, 2015-12-22 08:53:57]

The CQ bit was checked by iceman@yandex-team.ru

[commit-bot: I haz the power, 2015-12-22 08:54:13]

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

[commit-bot: I haz the power, 2015-12-22 09:19:31]

Description was changed from

==========
MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.
Add a cross-platform unit-test for that functionality.

TEST=views_unittests --gtest_filter="WidgetTest.OnDeviceScaleFactorChanged"
BUG=415024
==========

to

==========
MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.
Add a cross-platform unit-test for that functionality.

TEST=views_unittests --gtest_filter="WidgetTest.OnDeviceScaleFactorChanged"
BUG=415024
==========

[commit-bot: I haz the power, 2015-12-22 09:19:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-22 09:20:42]

Description was changed from

==========
MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.
Add a cross-platform unit-test for that functionality.

TEST=views_unittests --gtest_filter="WidgetTest.OnDeviceScaleFactorChanged"
BUG=415024
==========

to

==========
MacViews: Implement BridgedNativeWidget::OnDeviceScaleFactorChanged function.
Add a cross-platform unit-test for that functionality.

TEST=views_unittests --gtest_filter="WidgetTest.OnDeviceScaleFactorChanged"
BUG=415024

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

[commit-bot: I haz the power, 2015-12-22 09:20:43]

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