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

ui/display/win/screen_win.cc

Index: ui/display/win/screen_win.cc
diff --git a/ui/display/win/screen_win.cc b/ui/display/win/screen_win.cc
index 86d6be41cd4e4a33e8a7dcc09600930582fa9e3e..55dc04a0cf7fb96e2dca9f2349077c59ce119317 100644
--- a/ui/display/win/screen_win.cc
+++ b/ui/display/win/screen_win.cc
@@ -384,8 +384,8 @@ 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(
@@ -443,6 +443,7 @@ gfx::Rect ScreenWin::DIPToScreenRectInWindow(gfx::NativeView view,
 void ScreenWin::UpdateFromDisplayInfos(
     const std::vector<DisplayInfo>& display_infos) {
   screen_win_displays_ = DisplayInfosToScreenWinDisplays(display_infos);
+  displays_ = ScreenWinDisplaysToDisplays(screen_win_displays_);
 }
 
 void ScreenWin::Initialize() {
@@ -487,7 +488,7 @@ void ScreenWin::OnWndProc(HWND hwnd,
   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());
 }