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

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

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests

Committed: https://crrev.com/107f22ddc225e474c00a4a3814032e691051f9be
Cr-Commit-Position: refs/heads/master@{#433246}

[riajiang, 2016-11-09 16:14:14]

Patchset #1 (id:1) has been deleted

[riajiang, 2016-11-09 16:15:04]

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

[commit-bot: I haz the power, 2016-11-09 16:15:20]

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-11-09 16:24:04]

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

[commit-bot: I haz the power, 2016-11-09 16:24:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[riajiang, 2016-11-10 21:53:40]

Description was changed from

==========
Change GetAllDisplays() to return a const vector&.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests
==========

to

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

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests
==========

[riajiang, 2016-11-10 22:34:19]

Patchset #2 (id:40001) has been deleted

[riajiang, 2016-11-10 22:34:32]

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

[commit-bot: I haz the power, 2016-11-10 22:35:03]

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

[riajiang, 2016-11-10 22:57:34]

Patchset #1 (id:20001) has been deleted

[riajiang, 2016-11-10 23:11:22]

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

[riajiang, 2016-11-10 23:11:32]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-11-10 23:12:09]

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-11-10 23:29:03]

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

[commit-bot: I haz the power, 2016-11-10 23:29:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[riajiang, 2016-11-11 19:42:16]

Patchset #2 (id:100001) has been deleted

[riajiang, 2016-11-11 20:43:00]

Patchset #3 (id:140001) has been deleted

[riajiang, 2016-11-11 22:22:16]

Patchset #4 (id:180001) has been deleted

[riajiang, 2016-11-11 22:25:16]

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

[riajiang, 2016-11-11 22:25:23]

Patchset #1 (id:80001) has been deleted

[riajiang, 2016-11-11 22:25:30]

Patchset #1 (id:120001) has been deleted

[riajiang, 2016-11-11 22:25:37]

Patchset #1 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-11-11 22:26:24]

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-11-11 23:23:40]

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

[commit-bot: I haz the power, 2016-11-11 23:23:41]

Dry run: This issue passed the CQ dry run.

[riajiang, 2016-11-11 23:46:43]

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

[riajiang, 2016-11-11 23:48:26]

sadrul@ and sky@, PTAL. Thanks!

[robliao, 2016-11-12 00:11:05]

robliao@chromium.org changed reviewers:
+ robliao@chromium.org

[robliao, 2016-11-12 00:11:06]

screen_win lgtm + nit

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
File ui/display/win/screen_win.h (right):

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
ui/display/win/screen_win.h:187: // The corresponding display::Display.
Nit: Linebreak above and use this as the comment "The display::Displays
corresponding to |screen_win_displays_| for GetAllDisplays(). This must be
updated anytime |screen_win_displays_| is updated."

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

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
File ui/display/win/screen_win.cc (right):

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
ui/display/win/screen_win.cc:491: const std::vector<display::Display>&
old_displays = GetAllDisplays();
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]

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
File ui/display/win/screen_win.cc (right):

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
ui/display/win/screen_win.cc:491: const std::vector<display::Display>&
old_displays = GetAllDisplays();
On 2016/11/12 02:43:00, sadrul wrote:
> 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.

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]

Please take another look. Thanks!

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
File ui/display/win/screen_win.cc (right):

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
ui/display/win/screen_win.cc:491: const std::vector<display::Display>&
old_displays = GetAllDisplays();
On 2016/11/12 02:43:00, sadrul wrote:
> 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.

Good point! Done.

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
ui/display/win/screen_win.cc:491: const std::vector<display::Display>&
old_displays = GetAllDisplays();
On 2016/11/12 07:07:00, robliao wrote:
> On 2016/11/12 02:43:00, sadrul wrote:
> > 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.
> 
> 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_ }
> 

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?

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
File ui/display/win/screen_win.h (right):

https://codereview.chromium.org/2489873002/diff/200001/ui/display/win/screen_...
ui/display/win/screen_win.h:187: // The corresponding display::Display.
On 2016/11/12 00:11:06, robliao wrote:
> Nit: Linebreak above and use this as the comment "The display::Displays
> corresponding to |screen_win_displays_| for GetAllDisplays(). This must be
> updated anytime |screen_win_displays_| is updated."

Done.

[sadrul, 2016-11-16 00:24:39]

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

Looks like the only additional information ScreenWinDisplay has is the pixel
bounds. I think it would make sense to move that into display::Display (and it
is pretty likely we will need that for mustash/high-dpi too), and get rid of
ScreenWindowDisplay, and update ScreenWin to deal with the normal
display::Display objects, like the rest of the platforms.

riajiang@: Mind filing a bug for this task?

lgtm

[riajiang, 2016-11-16 01:15:36]

riajiang@chromium.org changed reviewers:
+ stevenjb@chromium.org

[riajiang, 2016-11-16 01:15:36]

+stevenjb@ for extensions/browser/api/system_display/system_display_apitest.cc,
thanks!

> Looks like the only additional information ScreenWinDisplay has is the pixel
> bounds. I think it would make sense to move that into display::Display (and it
> is pretty likely we will need that for mustash/high-dpi too), and get rid of
> ScreenWindowDisplay, and update ScreenWin to deal with the normal
> display::Display objects, like the rest of the platforms.
> 
> riajiang@: Mind filing a bug for this task?

Filed bug crbug.com/665669.

[stevenjb, 2016-11-16 22:31:07]

extensions/browser/api/system_display/ lgtm

https://codereview.chromium.org/2489873002/diff/220001/ui/display/win/screen_...
File ui/display/win/screen_win.cc (right):

https://codereview.chromium.org/2489873002/diff/220001/ui/display/win/screen_...
ui/display/win/screen_win.cc:493:
change_notifier_.NotifyDisplaysChanged(old_displays, GetAllDisplays());
nit: This might be more clear if it just used displays_ instead of
GetAllDisplays().

[sky, 2016-11-17 17:06:39]

ash and chrome LGTM

[riajiang, 2016-11-17 17:11:58]

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

[commit-bot: I haz the power, 2016-11-17 17:12:39]

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-11-17 17:24:05]

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

[commit-bot: I haz the power, 2016-11-17 17:24:06]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[riajiang, 2016-11-17 18:49:27]

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

[commit-bot: I haz the power, 2016-11-17 18:50:04]

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-11-17 19:22:32]

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

[commit-bot: I haz the power, 2016-11-17 19:22:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[riajiang, 2016-11-17 19:54:05]

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

[commit-bot: I haz the power, 2016-11-17 19:55:32]

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-11-17 20:29:08]

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

[commit-bot: I haz the power, 2016-11-17 20:29:10]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[riajiang, 2016-11-17 20:45:39]

Patchset #5 (id:280001) has been deleted

[riajiang, 2016-11-17 20:45:49]

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

[commit-bot: I haz the power, 2016-11-17 20:47:07]

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

[riajiang, 2016-11-17 22:56:53]

https://codereview.chromium.org/2489873002/diff/220001/ui/display/win/screen_...
File ui/display/win/screen_win.cc (right):

https://codereview.chromium.org/2489873002/diff/220001/ui/display/win/screen_...
ui/display/win/screen_win.cc:493:
change_notifier_.NotifyDisplaysChanged(old_displays, GetAllDisplays());
On 2016/11/16 22:31:07, stevenjb wrote:
> nit: This might be more clear if it just used displays_ instead of
> GetAllDisplays().

Done.

[riajiang, 2016-11-17 22:56:59]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-11-17 22:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, stevenjb@chromium.org, sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2489873002/#ps320001
(title: "comment")

[commit-bot: I haz the power, 2016-11-17 22:57: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, 2016-11-18 01:05:03]

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

[commit-bot: I haz the power, 2016-11-18 01:05:04]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[riajiang, 2016-11-18 03:46:33]

The CQ bit was checked by riajiang@chromium.org

[commit-bot: I haz the power, 2016-11-18 03:47:50]

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-11-18 05:58:51]

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

[commit-bot: I haz the power, 2016-11-18 05:58:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[riajiang, 2016-11-18 16:40:49]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-11-18 16:40:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, stevenjb@chromium.org, sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2489873002/#ps340001
(title: "roll back to rebase as dummy_screen_android is reverted")

[commit-bot: I haz the power, 2016-11-18 16:41:34]

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-11-18 18:38:43]

Description was changed from

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

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests
==========

to

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

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests
==========

[commit-bot: I haz the power, 2016-11-18 18:38:47]

Committed patchset #7 (id:340001)

[commit-bot: I haz the power, 2016-11-18 18:41:01]

Description was changed from

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

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests
==========

to

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

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by tests

Committed: https://crrev.com/107f22ddc225e474c00a4a3814032e691051f9be
Cr-Commit-Position: refs/heads/master@{#433246}
==========

[commit-bot: I haz the power, 2016-11-18 18:41:02]

Patchset 7 (id:??) landed as
https://crrev.com/107f22ddc225e474c00a4a3814032e691051f9be
Cr-Commit-Position: refs/heads/master@{#433246}