Change GetAllDisplays() in all screens to return a const vector&.

ui/display/win/screen_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/display/win/screen_win.h"
 
 #include <windows.h>
 #include <shellscalingapi.h>
 
 #include <algorithm>
@@ skipping 366 matching lines @@
 
 gfx::NativeWindow ScreenWin::GetWindowAtScreenPoint(const gfx::Point& point) {
   gfx::Point point_in_pixels = DIPToScreenPoint(point);
   return GetNativeWindowFromHWND(WindowFromPoint(point_in_pixels.ToPOINT()));
 }
 
 int ScreenWin::GetNumDisplays() const {
   return static_cast<int>(screen_win_displays_.size());
 }
 
-std::vector<display::Display> ScreenWin::GetAllDisplays() const {
-  return ScreenWinDisplaysToDisplays(screen_win_displays_);
+const std::vector<display::Display>& ScreenWin::GetAllDisplays() const {
+  return displays_;
 }
 
 display::Display ScreenWin::GetDisplayNearestWindow(
     gfx::NativeView window) const {
   HWND window_hwnd = GetHWNDFromNativeView(window);
   if (!window_hwnd) {
     // When |window| isn't rooted to a display, we should just return the
     // default display so we get some correct display information like the
     // scaling factor.
     return GetPrimaryDisplay();
@@ skipping 37 matching lines @@
 
 gfx::Rect ScreenWin::DIPToScreenRectInWindow(gfx::NativeView view,
                                              const gfx::Rect& dip_rect) const {
   HWND hwnd = view ? GetHWNDFromNativeView(view) : nullptr;
   return DIPToScreenRect(hwnd, dip_rect);
 }
 
 void ScreenWin::UpdateFromDisplayInfos(
     const std::vector<DisplayInfo>& display_infos) {
   screen_win_displays_ = DisplayInfosToScreenWinDisplays(display_infos);
+  displays_ = ScreenWinDisplaysToDisplays(screen_win_displays_);
 }
 
 void ScreenWin::Initialize() {
   singleton_hwnd_observer_.reset(
       new gfx::SingletonHwndObserver(
           base::Bind(&ScreenWin::OnWndProc, base::Unretained(this))));
   UpdateFromDisplayInfos(GetDisplayInfosFromSystem());
   RecordDisplayScaleFactors();
 }
 
@@ skipping 24 matching lines @@
   return ::GetSystemMetrics(metric);
 }
 
 void ScreenWin::OnWndProc(HWND hwnd,
                           UINT message,
                           WPARAM wparam,
                           LPARAM lparam) {
   if (message != WM_DISPLAYCHANGE)
     return;
 
-  std::vector<display::Display> old_displays = GetAllDisplays();
+  const std::vector<display::Display>& old_displays = GetAllDisplays();

[sadrul, 2016/11/12 02:43:00]

This looks a bit tricky. Since |displays_| is getting updated in the next line,
using old_displays after that in the following line will probably not do the
right thing (as in, |old_displays| will be the same as |new_displays| in
NotifyDisplaysChanged())? I think you want to do something more like:

  std::vector<display::Display> old_displays = std::move(displays_);
  ...

This way, |old_displays| is a separate list, and since it's doing a move, you
avoid the extra copy.

[robliao, 2016/11/12 07:07:00]

Nice catch. Agreed on this point. Now that I think about it, I'm not sure it's
really appropriate to use a const std::vector<>& here since GetAllDisplays (as
in this case) may not necessarily have a backing variable, unlike a regular C++
style simple accessor.

In this particular case, a computation is necessary to generate the displays and
then we can hand the displays off. The copy is likely elided in this case and no
backing store is necessary.

My recollection is that it's up to the internal implementation to decide how it
wants to implement its own displays. This is principally the reason why
ScreenWin has ScreenWinDisplays; ScreenWinDisplays provides extra information
only necessary on Windows.

Taking a step back, what's the motivation for the const std::vector<>& change? 
It's not clear to me from https://codereview.chromium.org/2361283002/ why this
is needed.

Assuming there's a good reason for this, GetAllDisplays() should probably change
to const std::vector<display::Displays> displays() { return displays_ }

[riajiang, 2016/11/15 20:08:13]

Good point! Done.

[riajiang, 2016/11/15 20:08:13]

We are changing GetAllDisplays() to return a const vector& so that
GetDisplayWithDisplayId() (from https://codereview.chromium.org/2361283002/) can
use GetAllDisplays() and return a &.

ScreenWin is the only place that we have platform specific display
ScreenWinDisplays so this shouldn't affect other places?

   UpdateFromDisplayInfos(GetDisplayInfosFromSystem());
   change_notifier_.NotifyDisplaysChanged(old_displays, GetAllDisplays());
 }
 
 ScreenWinDisplay ScreenWin::GetScreenWinDisplayNearestHWND(HWND hwnd)
     const {
   return GetScreenWinDisplay(MonitorInfoFromWindow(hwnd,
                                                    MONITOR_DEFAULTTONEAREST));
 }
 
@@ skipping 87 matching lines @@
     if (std::find(unique_scale_factors.begin(), unique_scale_factors.end(),
                   reported_scale) == unique_scale_factors.end()) {
       unique_scale_factors.push_back(reported_scale);
       UMA_HISTOGRAM_SPARSE_SLOWLY("UI.DeviceScale", reported_scale);
     }
   }
 }
 
 }  // namespace win
 }  // namespace display