Simplify calls for scale factor

Simplify calls for scale factor

Made callsites simpler by replacing multiple commands
with one api |ui::GetScaleFactorForNativeView(gfx::NativeView)|.
Android got further simpler by |ViewAndroid::GetDipScale()|

BUG=699891
Review-Url: https://codereview.chromium.org/2739113002
Cr-Commit-Position: refs/heads/master@{#456560}
Committed: https://chromium.googlesource.com/chromium/src/+/61bc73e74f6cd55f220c22d2c8f154573b2f1ef8

[Jinsuk Kim, 2017-03-10 00:21:17]

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

[commit-bot: I haz the power, 2017-03-10 00:22:07]

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

[Jinsuk Kim, 2017-03-10 00:36:37]

Description was changed from

==========
ui::GetScaleFactorForNativeView for scale factor

Made call sites simpler by replacing multiple commands
with one api that returns scale factor for a given
gfx::NativeView.

BUG=699891
==========

to

==========
Simplify calls for scale factor

Made callsites simpler by replacing multiple commands
with one api |ui::GetScaleFactorForNativeView(gfx::NativeView)|.
Android got further simpler by |ViewAndroid::GetDipScale()|

BUG=699891
==========

[commit-bot: I haz the power, 2017-03-10 01:13:44]

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

[commit-bot: I haz the power, 2017-03-10 01:13:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jinsuk Kim, 2017-03-10 03:24:24]

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

[commit-bot: I haz the power, 2017-03-10 03:24:51]

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, 2017-03-10 03:41:03]

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

[commit-bot: I haz the power, 2017-03-10 03:41:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jinsuk Kim, 2017-03-10 04:20:52]

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

[commit-bot: I haz the power, 2017-03-10 04:21:07]

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

[Jinsuk Kim, 2017-03-10 04:22:21]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-10 04:34:02]

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

[commit-bot: I haz the power, 2017-03-10 04:34:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Jinsuk Kim, 2017-03-10 06:01:15]

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

[commit-bot: I haz the power, 2017-03-10 06:01:34]

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

[Jinsuk Kim, 2017-03-10 06:01:51]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-10 07:32:09]

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

[commit-bot: I haz the power, 2017-03-10 07:32:10]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2017-03-12 23:17:20]

jinsukkim@chromium.org changed reviewers:
+ tapted@chromium.org

[Jinsuk Kim, 2017-03-12 23:17:21]

Note: render_widget_host_view_mac.mm was changed so that only ctor /
|display::DisplayObserver::OnDisplayMetricsChanged| call
|Screen::GetDisplayNearestWindow| which is expensive in Mac.

[tapted, 2017-03-13 00:02:33]

https://codereview.chromium.org/2739113002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1732:
device_scale_factor_ = display.device_scale_factor();
This isn't quite the same. OnDisplayMetricsChanged() is only triggered when the
display metrics change (e.g. add/remove display, or change resolution). It's
_not_ triggered when the window simply moves to a different display, which is
far more common.

I don't think we should add |device_scale_factor_| to RenderWidgetHostViewMac --
it is duplicating the function of |current_device_scale_factor_| in
RenderWidgetHostViewBase in any case.

But `ui::GetScaleFactorForNativeView(..)` is an inexpensive method call on Mac
under most situations -- we shouldn't worry about caching it - just call it each
time.

[Jinsuk Kim, 2017-03-13 00:25:01]

https://codereview.chromium.org/2739113002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1732:
device_scale_factor_ = display.device_scale_factor();
On 2017/03/13 00:02:33, tapted wrote:
> This isn't quite the same. OnDisplayMetricsChanged() is only triggered when
the
> display metrics change (e.g. add/remove display, or change resolution). It's
> _not_ triggered when the window simply moves to a different display, which is
> far more common.
> 
> I don't think we should add |device_scale_factor_| to RenderWidgetHostViewMac
--
> it is duplicating the function of |current_device_scale_factor_| in
> RenderWidgetHostViewBase in any case.
> 
> But `ui::GetScaleFactorForNativeView(..)` is an inexpensive method call on Mac
> under most situations -- we shouldn't worry about caching it - just call it
each
> time.

Reverted the change. Removed an unused method only.

[tapted, 2017-03-13 00:28:39]

lgtm with the following - thanks!

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1521: float
scale_factor = display::Screen::GetScreen()
use ui::GetScaleFactorForNativeView(..)?

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1578: float
scale_factor = display::Screen::GetScreen()
use ui::GetScaleFactorForNativeView(..)?

[Jinsuk Kim, 2017-03-13 00:31:30]

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1521: float
scale_factor = display::Screen::GetScreen()
On 2017/03/13 00:28:39, tapted wrote:
> use ui::GetScaleFactorForNativeView(..)?

On mac that's not the same, and changing it caused test failure. See the earlier
patch.

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1578: float
scale_factor = display::Screen::GetScreen()
On 2017/03/13 00:28:39, tapted wrote:
> use ui::GetScaleFactorForNativeView(..)?

ditto

[tapted, 2017-03-13 00:44:34]

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1521: float
scale_factor = display::Screen::GetScreen()
On 2017/03/13 00:31:30, Jinsuk Kim wrote:
> On 2017/03/13 00:28:39, tapted wrote:
> > use ui::GetScaleFactorForNativeView(..)?
> 
> On mac that's not the same, and changing it caused test failure. See the
earlier
> patch.

Then I think we want to update layout_mac.mm:

float GetScaleFactorForNativeView(gfx::NativeView view) {
  return GetScaleFactorScaleForNativeView(view);
}

should become something like

float GetScaleFactorForNativeView(gfx::NativeView view) {
  if (Display::HasForceDeviceScaleFactor())
    return Display::GetForcedDeviceScaleFactor();
  return GetScaleFactorScaleForNativeView(view);
}

[Jinsuk Kim, 2017-03-13 01:03:49]

On 2017/03/13 00:44:34, tapted wrote:
>
https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/2739113002/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_mac.mm:1521: float
> scale_factor = display::Screen::GetScreen()
> On 2017/03/13 00:31:30, Jinsuk Kim wrote:
> > On 2017/03/13 00:28:39, tapted wrote:
> > > use ui::GetScaleFactorForNativeView(..)?
> > 
> > On mac that's not the same, and changing it caused test failure. See the
> earlier
> > patch.
> 
> Then I think we want to update layout_mac.mm:
> 
> float GetScaleFactorForNativeView(gfx::NativeView view) {
>   return GetScaleFactorScaleForNativeView(view);
> }
> 
> should become something like
> 
> float GetScaleFactorForNativeView(gfx::NativeView view) {
>   if (Display::HasForceDeviceScaleFactor())
>     return Display::GetForcedDeviceScaleFactor();
>   return GetScaleFactorScaleForNativeView(view);
> }

Thanks for the comment. Done.

[Jinsuk Kim, 2017-03-13 01:37:02]

jinsukkim@chromium.org changed reviewers:
+ avi@chromium.org, boliu@chromium.org, erg@chromium.org

[Jinsuk Kim, 2017-03-13 01:37:02]

avi@ please review the changes in:
  ui/snapshot/DEPS which added a new depend-on path

erg@ please review the changes in: 
  ui/aura/window_tree_host_x11.cc
  ui/views/widget/desktop_aura/desktop_drag_...client_aurax11.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc

boliu@ please review the changes in content/

[Avi (use Gerrit), 2017-03-13 01:46:19]

On 2017/03/13 01:37:02, Jinsuk Kim wrote:
> avi@ please review the changes in:
>   ui/snapshot/DEPS which added a new depend-on path

LGTM

[tapted, 2017-03-13 01:47:01]

still lgtm % nit

https://codereview.chromium.org/2739113002/diff/100001/ui/base/layout_mac.mm
File ui/base/layout_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/100001/ui/base/layout_mac.mm#...
ui/base/layout_mac.mm:10: #include <Cocoa/Cocoa.h>
nit: this should appear between layout.h and sdk_forward_declarations.h with a
blank line on both sides. (also it should be #import instead of #include)

[boliu, 2017-03-13 01:51:57]

lgtm

https://codereview.chromium.org/2739113002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/2739113002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:666: *
ui::GetScaleFactorForNativeView(GetNativeView()),
all other platforms are switching to GetScaleFactorForNativeView, but android is
switching away from it?

[Jinsuk Kim, 2017-03-13 01:58:22]

https://codereview.chromium.org/2739113002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/2739113002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:666: *
ui::GetScaleFactorForNativeView(GetNativeView()),
On 2017/03/13 01:51:57, boliu wrote:
> all other platforms are switching to GetScaleFactorForNativeView, but android
is
> switching away from it?

ViewAndroid::GetDipScale() calls exactly that. I believe this is easier to read.

https://codereview.chromium.org/2739113002/diff/100001/ui/base/layout_mac.mm
File ui/base/layout_mac.mm (right):

https://codereview.chromium.org/2739113002/diff/100001/ui/base/layout_mac.mm#...
ui/base/layout_mac.mm:10: #include <Cocoa/Cocoa.h>
On 2017/03/13 01:47:01, tapted wrote:
> nit: this should appear between layout.h and sdk_forward_declarations.h with a
> blank line on both sides. (also it should be #import instead of #include)

Done.

[Elliot Glaysher, 2017-03-13 17:44:32]

renames in x11 lgtm

[Jinsuk Kim, 2017-03-13 23:25:51]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-03-13 23:25:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
avi@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2739113002/#ps120001
(title: "nit")

[commit-bot: I haz the power, 2017-03-13 23:26:28]

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

[Jinsuk Kim, 2017-03-13 23:32:39]

The CQ bit was unchecked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-03-13 23:53:58]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-03-13 23:54:04]

The CQ bit was unchecked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-03-13 23:55:00]

The CQ bit was checked by jinsukkim@chromium.org

[commit-bot: I haz the power, 2017-03-13 23:55:54]

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, 2017-03-14 00:36:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489449300453490,
"parent_rev": "57c348516ccf9a680791408ba89b2bd2dbf90b17", "commit_rev":
"61bc73e74f6cd55f220c22d2c8f154573b2f1ef8"}

[commit-bot: I haz the power, 2017-03-14 00:37:56]

Description was changed from

==========
Simplify calls for scale factor

Made callsites simpler by replacing multiple commands
with one api |ui::GetScaleFactorForNativeView(gfx::NativeView)|.
Android got further simpler by |ViewAndroid::GetDipScale()|

BUG=699891
==========

to

==========
Simplify calls for scale factor

Made callsites simpler by replacing multiple commands
with one api |ui::GetScaleFactorForNativeView(gfx::NativeView)|.
Android got further simpler by |ViewAndroid::GetDipScale()|

BUG=699891

Review-Url: https://codereview.chromium.org/2739113002
Cr-Commit-Position: refs/heads/master@{#456560}
Committed:
https://chromium.googlesource.com/chromium/src/+/61bc73e74f6cd55f220c22d2c8f1...
==========

[commit-bot: I haz the power, 2017-03-14 00:37:57]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/61bc73e74f6cd55f220c22d2c8f1...