Add support for virtual displays

Add support for virtual displays

This CL allows the programmatic additional and removal of a virtual display for
the purposes of both multiple display testing and to allow extending the the
workspace for capture with e.g. Chrome Cast.

BUG=425060
TEST=New tests added to DisplayConfiguratorTest. Interactive testing when
combined with CL to enable controls for virtual display in display settings.

Committed: https://crrev.com/3d88a677ef10af9780edef84ccde79b32de51cb6
Cr-Commit-Position: refs/heads/master@{#364332}

[robert.bradford, 2015-11-17 16:44:37]

robert.bradford@intel.com changed reviewers:
+ jdufault@chromium.org

[robert.bradford, 2015-11-17 16:44:37]

jdufault@ would be cool if you could take a look before I ask oshima@ for
review.

[achuithb, 2015-11-17 19:29:26]

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

[commit-bot: I haz the power, 2015-11-17 19:30:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456623002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456623002/1

[jdufault, 2015-11-17 20:10:21]

Generally looks good, but I'm not very familiar with the code here. Let's see
what oshima@ has to say.

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator_unittest.cc:398: const
DisplayConfigurator::DisplayStateList* cached =
Any reason this is a pointer? Can we just copy cached_displays, given that we're
always updating the cached list before checking it?

It looks like UpdateCachedOutputsEvenAfterFailure does the same thing you have
here - maybe change the logic there too?

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator_unittest.cc:425: cached =
&configurator_.cached_displays();
If cached remains a pointer/reference, is this needed?

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_snapshot_virtual.cc (right):

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_snapshot_virtual.cc:18: gfx::Size(200, 200),
Any idea on what better dimensions would be?

[commit-bot: I haz the power, 2015-11-17 20:21:42]

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

[commit-bot: I haz the power, 2015-11-17 20:21:43]

Dry run: This issue passed the CQ dry run.

[robert.bradford, 2015-11-18 12:46:10]

robert.bradford@intel.com changed reviewers:
+ oshima@chromium.org

[robert.bradford, 2015-11-18 12:46:10]

robert.bradford@intel.com changed required reviewers:
+ oshima@chromium.org

[robert.bradford, 2015-11-18 12:46:10]

+oshima@ for OWNERS review

Thanks jdufault@!

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator_unittest.cc:398: const
DisplayConfigurator::DisplayStateList* cached =
On 2015/11/17 20:10:21, jdufault wrote:
> Any reason this is a pointer? Can we just copy cached_displays, given that
we're
> always updating the cached list before checking it?
> 
> It looks like UpdateCachedOutputsEvenAfterFailure does the same thing you have
> here - maybe change the logic there too?

Good catch. As you spotted, I must have copied this from the above test, which
has the same mistake. By taking the address of the const ref to the internal
vector the goal was not to have cheap access to the internal datastructure of
the DisplayConfigurator.

I've dropped the unnecessary calls to cached_displays() in all the tests.

https://codereview.chromium.org/1456623002/diff/1/ui/display/chromeos/display...
ui/display/chromeos/display_configurator_unittest.cc:425: cached =
&configurator_.cached_displays();
On 2015/11/17 20:10:21, jdufault wrote:
> If cached remains a pointer/reference, is this needed?

Acknowledged.

[oshima, 2015-11-24 22:19:28]

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:665: // Exclude virtual displays.
I believe this is not to cast the protected content. 
Can you explain why we can exclude this?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1111:
virtual_display_snapshots_.erase(it);
where  are DisplaySnapshotVirtual objects deleted?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:434: int64_t last_virtual_display_id_
= 0;
Can you factor out GetID in ui/display/util/edid_parser.cc and use
0x8000 as manufacturer id, and 0x as product code and this id as a output index?
please make sure that it will not overflow.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator_unittest.cc:345:
&configurator_.cached_displays();
const ... & cached =

(I understand this is used in this file, but this is better)

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator_unittest.cc:346:
ASSERT_EQ(static_cast<size_t>(1), cached->size());
1u

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator_unittest.cc:365: &outputs_[0], NULL)
nullptr. same for other files

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:18: gfx::Size(200, 200),
why not display_size ?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.h (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:22:
DisplaySnapshotVirtual(int64_t display_id, gfx::Size display_size);
const gfx::Size &

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:31: void set_display_id(int64_t
display_id) { display_id_ = display_id; }
where this is used?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
File ui/display/chromeos/update_display_configuration_task.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
ui/display/chromeos/update_display_configuration_task.cc:53:
cached_displays_.push_back(display);
cached_display.insert(cache_display.end(), display.begin(), display.end());

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
File ui/display/chromeos/update_display_configuration_task.h (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
ui/display/chromeos/update_display_configuration_task.h:40: }
please document ownership

[robert.bradford, 2015-11-26 16:29:41]

Thank you for the review oshima@. All points addressed, but I did have a few
questions below.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:665: // Exclude virtual displays.
On 2015/11/24 22:19:27, oshima wrote:
> I believe this is not to cast the protected content. 
> Can you explain why we can exclude this?

My aim was to prevent protected content from being rendered to the virtual
display where it could be recaptured. Maybe I misunderstood the flow here and
this code doesn't have that affect?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1111:
virtual_display_snapshots_.erase(it);
On 2015/11/24 22:19:27, oshima wrote:
> where  are DisplaySnapshotVirtual objects deleted?

I'm using a base::ScopedVector. This means that I can very easily get the
std::vector<DisplaySnapshot> needed to inject into the DisplayConfigurationTask.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:434: int64_t last_virtual_display_id_
= 0;
On 2015/11/24 22:19:27, oshima wrote:
> Can you factor out GetID in ui/display/util/edid_parser.cc and use
> 0x8000 as manufacturer id, and 0x as product code and this id as a output
index?
> please make sure that it will not overflow.

Ah, that's a great solution. I wasn't very happy with it as it stood.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator_unittest.cc:345:
&configurator_.cached_displays();
On 2015/11/24 22:19:27, oshima wrote:
> const ... & cached =
> 
> (I understand this is used in this file, but this is better)

Done. And I addressed the other user of this.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator_unittest.cc:346:
ASSERT_EQ(static_cast<size_t>(1), cached->size());
On 2015/11/24 22:19:27, oshima wrote:
> 1u

Done. And some similar ones too.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator_unittest.cc:365: &outputs_[0], NULL)
On 2015/11/24 22:19:27, oshima wrote:
> nullptr. same for other files

Done.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:18: gfx::Size(200, 200),
On 2015/11/24 22:19:27, oshima wrote:
> why not display_size ?

This parameter is for the physical size. I've replaced it with dimensions for a
non-hidpi display.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.h (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:22:
DisplaySnapshotVirtual(int64_t display_id, gfx::Size display_size);
On 2015/11/24 22:19:27, oshima wrote:
> const gfx::Size &

Done.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:31: void set_display_id(int64_t
display_id) { display_id_ = display_id; }
On 2015/11/24 22:19:27, oshima wrote:
> where this is used?

Left over from the pre-DisplayConfigurator split out and my rebase. Dropped.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
File ui/display/chromeos/update_display_configuration_task.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
ui/display/chromeos/update_display_configuration_task.cc:53:
cached_displays_.push_back(display);
On 2015/11/24 22:19:27, oshima wrote:
> cached_display.insert(cache_display.end(), display.begin(), display.end());

Done.

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
File ui/display/chromeos/update_display_configuration_task.h (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/upd...
ui/display/chromeos/update_display_configuration_task.h:40: }
On 2015/11/24 22:19:27, oshima wrote:
> please document ownership

Done. And the comment with the actual vector declaration updated too.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/DEPS
File ui/display/DEPS (right):

https://codereview.chromium.org/1456623002/diff/40001/ui/display/DEPS#newcode3
ui/display/DEPS:3: "+ui/gfx/display.h",
Needed for gfx::Display::kInvalidDisplayID.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1105: GetDisplayID(0x8000, 0x0,
++last_virtual_display_id_), display_size);
Would you prefer a kReservedManufacturerID constant. If so, where would it best
live?

[oshima, 2015-12-02 20:34:27]

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:665: // Exclude virtual displays.
On 2015/11/26 16:29:40, robert.bradford wrote:
> On 2015/11/24 22:19:27, oshima wrote:
> > I believe this is not to cast the protected content. 
> > Can you explain why we can exclude this?
> 
> My aim was to prevent protected content from being rendered to the virtual
> display where it could be recaptured. Maybe I misunderstood the flow here and
> this code doesn't have that affect?

Can you add comment?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:18: gfx::Size(200, 200),
On 2015/11/26 16:29:40, robert.bradford wrote:
> On 2015/11/24 22:19:27, oshima wrote:
> > why not display_size ?
> 
> This parameter is for the physical size. I've replaced it with dimensions for
a
> non-hidpi display.

I think this will be used to compute the DPI threshold to determine device scale
factor.
Will the physical size be available through cast API?

For now, please add comment that this is picked to use lower dsf.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:667: // Exclude virtual displays.
please mention the possibility to be recaptured.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1105: GetDisplayID(0x8000, 0x0,
++last_virtual_display_id_), display_size);
On 2015/11/26 16:29:41, robert.bradford wrote:
> Would you prefer a kReservedManufacturerID constant. If so, where would it
best
> live?

Yes, and you can put it in the anonymous namespace in this file. Please add
comment saying that this is reserved in EDID spec.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1117: last_virtual_display_id_--;
can you walk through the list (after erase) and set it back to the largest id? I
think it's more stable.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/util/display...
File ui/display/util/display_util.h (right):

https://codereview.chromium.org/1456623002/diff/40001/ui/display/util/display...
ui/display/util/display_util.h:31: DISPLAY_UTIL_EXPORT int64_t
GetDisplayID(uint16_t manufacturer_id,
Generate/Create DisplayID (whichever you prefer)

[robert.bradford, 2015-12-03 18:44:57]

Thanks oshima@ could you take another look?

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:18: gfx::Size(200, 200),
On 2015/12/02 20:34:27, oshima wrote:
> On 2015/11/26 16:29:40, robert.bradford wrote:
> > On 2015/11/24 22:19:27, oshima wrote:
> > > why not display_size ?
> > 
> > This parameter is for the physical size. I've replaced it with dimensions
for
> a
> > non-hidpi display.
> 
> I think this will be used to compute the DPI threshold to determine device
scale
> factor.
> Will the physical size be available through cast API?
> 
> For now, please add comment that this is picked to use lower dsf.

Or, as i've just posted, calculate the physical display based on 96dpi. How does
that sound?

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:667: // Exclude virtual displays.
On 2015/12/02 20:34:27, oshima wrote:
> please mention the possibility to be recaptured.

Done.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1105: GetDisplayID(0x8000, 0x0,
++last_virtual_display_id_), display_size);
On 2015/12/02 20:34:27, oshima wrote:
> On 2015/11/26 16:29:41, robert.bradford wrote:
> > Would you prefer a kReservedManufacturerID constant. If so, where would it
> best
> > live?
> 
> Yes, and you can put it in the anonymous namespace in this file. Please add
> comment saying that this is reserved in EDID spec.

Done. And I used 1 << 15 to further clarify.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1117: last_virtual_display_id_--;
On 2015/12/02 20:34:27, oshima wrote:
> can you walk through the list (after erase) and set it back to the largest id?
I
> think it's more stable.

Done.

https://codereview.chromium.org/1456623002/diff/40001/ui/display/util/display...
File ui/display/util/display_util.h (right):

https://codereview.chromium.org/1456623002/diff/40001/ui/display/util/display...
ui/display/util/display_util.h:31: DISPLAY_UTIL_EXPORT int64_t
GetDisplayID(uint16_t manufacturer_id,
On 2015/12/02 20:34:27, oshima wrote:
> Generate/Create DisplayID (whichever you prefer)

Done -> GenerateDisplayID.

[oshima, 2015-12-03 18:54:07]

lgtm

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
File ui/display/chromeos/display_snapshot_virtual.cc (right):

https://codereview.chromium.org/1456623002/diff/20001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:18: gfx::Size(200, 200),
On 2015/12/03 18:44:57, robert.bradford wrote:
> On 2015/12/02 20:34:27, oshima wrote:
> > On 2015/11/26 16:29:40, robert.bradford wrote:
> > > On 2015/11/24 22:19:27, oshima wrote:
> > > > why not display_size ?
> > > 
> > > This parameter is for the physical size. I've replaced it with dimensions
> for
> > a
> > > non-hidpi display.
> > 
> > I think this will be used to compute the DPI threshold to determine device
> scale
> > factor.
> > Will the physical size be available through cast API?
> > 
> > For now, please add comment that this is picked to use lower dsf.
> 
> Or, as i've just posted, calculate the physical display based on 96dpi. How
does
> that sound?

sgtm

https://codereview.chromium.org/1456623002/diff/60001/ui/display/chromeos/dis...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/1456623002/diff/60001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1136: highest_virtual_display_id =
display->display_id();
optional: std::max if it fits in the single line (and nuke {})

[robert.bradford, 2015-12-09 15:08:18]

The CQ bit was checked by robert.bradford@intel.com

[robert.bradford, 2015-12-09 15:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1456623002/#ps80001
(title: "Use std::max as suggested by oshima@")

[commit-bot: I haz the power, 2015-12-09 15:09:38]

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

[commit-bot: I haz the power, 2015-12-09 17:19:50]

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

[commit-bot: I haz the power, 2015-12-09 17:19:51]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[robert.bradford, 2015-12-10 11:25:39]

The CQ bit was checked by robert.bradford@intel.com

[commit-bot: I haz the power, 2015-12-10 11:25:53]

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

[commit-bot: I haz the power, 2015-12-10 11:51:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-10 11:52:32]

Description was changed from

==========
Add support for virtual displays

This CL allows the programmatic additional and removal of a virtual display for
the purposes of both multiple display testing and to allow extending the the
workspace for capture with e.g. Chrome Cast.

BUG=425060
TEST=New tests added to DisplayConfiguratorTest. Interactive testing when
combined with CL to enable controls for virtual display in display settings.
==========

to

==========
Add support for virtual displays

This CL allows the programmatic additional and removal of a virtual display for
the purposes of both multiple display testing and to allow extending the the
workspace for capture with e.g. Chrome Cast.

BUG=425060
TEST=New tests added to DisplayConfiguratorTest. Interactive testing when
combined with CL to enable controls for virtual display in display settings.

Committed: https://crrev.com/3d88a677ef10af9780edef84ccde79b32de51cb6
Cr-Commit-Position: refs/heads/master@{#364332}
==========

[commit-bot: I haz the power, 2015-12-10 11:52:32]

Patchset 5 (id:??) landed as
https://crrev.com/3d88a677ef10af9780edef84ccde79b32de51cb6
Cr-Commit-Position: refs/heads/master@{#364332}