Add support for a virtual display on ChromeOS

Add support for a virtual display on ChromeOS

The purpose of this feature is to extend the desktop capture functionality by
allowing the creation of a virtual screen at capture request time. The use case
here is to mimic, with e.g. a Chrome Cast, the behaviour that can be achieved
with a physically connected projector i.e that a presenter can have slides
showing whilst keeping notes on their own private screen.

The "Virtual Display" appears as entry in the display picker, with a new
DesktopMediaID type. In the DesktopCaptureDeviceAura if an item of this type is
requested for capture the virtual screen is enabled in the DisplayConfigurator
and the resulting Aura root window used for capture.

BUG=425060
TEST=Use desktopCapture example extension, choose the Virtual Screen when
prompted and observe a new virtual display is created. When the capture is
stopped observe that the virtual display disappears.

[robert.bradford, 2014-10-02 10:51:16]

robert.bradford@intel.com changed reviewers:
+ lionel.g.landwerlin@intel.com

[robert.bradford, 2014-10-02 10:58:03]

robert.bradford@intel.com changed reviewers:
- lionel.g.landwerlin@intel.com

[robert.bradford, 2014-10-09 15:51:18]

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

[robert.bradford, 2014-10-09 15:51:19]

Hi folks, i'd love to hear your feedback on this feature. This is the (delayed)
follow-up to my original version that required X11 driver changes. This change
works on Ozone, and should work on X11 too although i've only tested it on
link_freon right now.

[oshima, 2014-10-09 17:26:43]

Hi, thank you for working on this.

Could you please file a feature request so that we can discuss the feature
including the issues you listed? 

We also need tests (at least on display management side) that covers use cases.

[robert.bradford, 2014-10-14 16:07:36]

On 2014/10/09 17:26:43, oshima wrote:
> Hi, thank you for working on this.
> 
> Could you please file a feature request so that we can discuss the feature
> including the issues you listed? 

Sure, is that done through crbugs.com? Or some other tool or mailing list?

> We also need tests (at least on display management side) that covers use
cases.

I'll start looking into that.

[oshima, 2014-10-14 16:59:44]

On 2014/10/14 16:07:36, robert.bradford wrote:
> On 2014/10/09 17:26:43, oshima wrote:
> > Hi, thank you for working on this.
> > 
> > Could you please file a feature request so that we can discuss the feature
> > including the issues you listed? 
> 
> Sure, is that done through crbugs.com? Or some other tool or mailing list?

Yes, please file it on crbug.com.

> 
> > We also need tests (at least on display management side) that covers use
> cases.
> 
> I'll start looking into that.

[robert.bradford, 2014-10-20 10:43:28]

On 2014/10/14 16:59:44, oshima wrote:
> On 2014/10/14 16:07:36, robert.bradford wrote:
> > On 2014/10/09 17:26:43, oshima wrote:
> > > Hi, thank you for working on this.
> > > 
> > > Could you please file a feature request so that we can discuss the feature
> > > including the issues you listed? 
> > 
> > Sure, is that done through crbugs.com? Or some other tool or mailing list?
> 
> Yes, please file it on http://crbug.com.
>

Bug opened here: https://code.google.com/p/chromium/issues/detail?id=425060

I look forward to receiving your input, in the meantime I will start working out
how to do the tests :-)

[robert.bradford, 2015-01-21 19:08:54]

This latest version has two major improvements over my last overhaul (which was
to support the asnyc display changes.):

- Unit test for adding and removing the virtual display
- Allows the mode of the virtual display to be chosen when the display is
enabled (request from markdavidscott@)

[achuithb, 2015-02-11 23:41:42]

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

[achuithb, 2015-02-11 23:41:44]

https://codereview.chromium.org/615133002/diff/220001/chrome/browser/media/de...
File chrome/browser/media/desktop_media_list_ash.cc (right):

https://codereview.chromium.org/615133002/diff/220001/chrome/browser/media/de...
chrome/browser/media/desktop_media_list_ash.cc:215: // DisplayConfigurator only
works when running on ChromeOS
nit: period at end of comment

https://codereview.chromium.org/615133002/diff/220001/chrome/browser/media/de...
chrome/browser/media/desktop_media_list_ash.cc:227: // TODO: There is no
thumbnail for the virtual screen right now
period at the end. Add your name in the TODO(<name>), add a bug-id.

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:136: void
OnDisplayRemoved(const gfx::Display& old_display) override{};
override {}

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:138: uint32_t
changed_metrics) override{};
override {}

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:243: 
remove newline

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:288:
gfx::Screen::GetNativeScreen()->RemoveObserver(this);
Shouldn't this be within the if stmt below?

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:292:
configurator->DisableVirtualDisplay();
Why not combine the above two statements?

ash::Shell:GetInstance()->display_configurator()->DisableVirtualDisplay();

https://codereview.chromium.org/615133002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:561:
ui::DisplayConfigurator* configurator =
SHould there not be a USE_ASH define?
Should there be a dcheck for RunningOnCros?

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/con...
File ui/display/chromeos/configure_displays_task.cc (right):

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/con...
ui/display/chromeos/configure_displays_task.cc:79: // NativeDisplayDelegate
period

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

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:148: if
(configurator_->virtual_display_mode_.get()) {
if (IsVirtualDisplayEnabled()) easier to read?

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1031: LOG(WARNING)
NOTREACHED may be better here

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1041: LOG(WARNING) << "Attempt to
disable virtual display when it is not enabled";
NOTREACHED might be better here

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1045: int64
DisplayConfigurator::GetVirtualDisplayId(void) {
don't need void

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1046: for (size_t i = 0; i <
cached_displays_.size(); i++) {
++i

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

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:287: // Enable/disable virtual output
period

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:290: bool virtual_display_enabled() {
return virtual_display_mode_.get(); }
this should be a const fn and it should be camelcase.
bool IsVirtualDisplayEnabled() const { ... }

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:292: // Find the virtual display id,
returns -1 if none
period

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:427: // Virtual output control
period

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

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:38: }  // namespace ui
newline

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

https://codereview.chromium.org/615133002/diff/220001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:15: virtual
~DisplaySnapshotVirtual();
override

[robert.bradford, 2015-02-12 17:25:05]

Thanks achuithb@ for your diligent review. I've addressed all the issues you
highlighted and took on all your recommendations.

Thanks!

[achuithb, 2015-02-13 00:14:32]

On 2015/02/12 17:25:05, robert.bradford wrote:
> Thanks achuithb@ for your diligent review. I've addressed all the issues you
> highlighted and took on all your recommendations.
> 
> Thanks!

In the future, you should click on individual comments, click Done, and then
Publish and Mail Comments.

There are other comments in your CL that don't have a period at the end - please
fix those as well.

I'm not qualified to do more than a nitpicky style review of this CL. It's lgtm
to me, but somebody more familiar with the code needs to review it as well.

[robert.bradford, 2015-02-13 13:59:32]

New patchsets have been uploaded after l-g-t-m from achuith@chromium.org

[achuithb, 2015-02-23 22:27:22]

achuith@chromium.org changed reviewers:
+ derat@chromium.org, marcheu@chromium.org

[achuithb, 2015-02-23 22:27:23]

Adding some more potential reviewers.

[Daniel Erat, 2015-02-23 22:41:15]

derat@chromium.org changed reviewers:
+ dnicoara@chromium.org

[Daniel Erat, 2015-02-23 22:41:16]

i'll take a look at ui/display/, but dnicoara@ should too.

[Daniel Erat, 2015-02-23 23:14:54]

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:150: display_state.display = new
DisplaySnapshotVirtual(
are you leaking this? DisplayState documents that it doesn't own its |display|
member.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1046: for (size_t i = 0; i <
cached_displays_.size(); ++i) {
nit: for (const auto& display : cached_displays_) {

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:287: // Enable/disable virtual
output.
nit: Change to "Enables/disables" to match style of other comments

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:288: void
EnableVirtualDisplay(scoped_ptr<DisplayMode> display_mode);
passing in a DisplayMode seems a bit strange to me (do you really just need to
pass the size?), but i'll let dnicoara@ comment on this.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:290: bool IsVirtualDisplayEnabled()
const { return virtual_display_mode_.get(); }
if this is inlined, it should be named like is_virtual_display_enabled(). it'd
be better to leave it named like this and not inline it, though. see
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:292: // Find the virtual display id,
returns -1 if none.
nit: s/Find/Finds/

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:15: gfx::Size(200, 200),
where do these numbers come from?

(i have flashbacks of 200x200 dimensions from excessive gtk+ programming in the
past)

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:12: class DisplaySnapshotVirtual
: public DisplaySnapshot {
add a comment documenting what this class is for

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:14:
DisplaySnapshotVirtual(uint32_t index, DisplayMode* mode);
add a comment documenting what |index| is and whether ownership of |mode| is
passed (and if it is, used scoped_ptr instead)

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
File ui/display/chromeos/display_util.h (right):

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_util.h:30: bool
IsNativeDisplay(ui::DisplayConnectionType type);
add a comment. also, i think it's confusing to use "Native" here. in other
contexts, like ui/gfx/native_widget_types.h, "native" means platform-specific. i
think that something like IsPhysicalDisplay() or IsRealDisplay() would be more
appropriate here.

[Sergey Ulanov, 2015-02-23 23:16:12]

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:233: bool
DesktopVideoCaptureMachine::Start(
Can the virtual-screen logic be moved out of this class? I think it can be
implemented as follows:
  - Change DesktopVideoCaptureMachine constructor to take aura::Window instead
of DesktopMediaID.
  - Change DesktopCaptureDeviceAura constructor to take aura::Window instead of
DesktopMediaID.
  - Add a new class called e.g. VirtualScreenCapturer that implements
media::VideoCaptureDevice and wraps DesktopCaptureDeviceAura, which actually
captures frames.

That way you will need fewer ifdefs and the code will be easier to read.

[piman, 2015-02-24 01:58:29]

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:37: #include
"ash/display/display_controller.h"
content/ is not allowed to depend on ash/

[dnicoara, 2015-02-24 16:27:06]

I'll need to have another pass at it. I left a few comments for now.

A couple of questions:
1) How does mirror mode work? I'm assuming it'll be software mirror.
2) In the future, would it be useful to have multiple virtual displays?

https://codereview.chromium.org/615133002/diff/260001/ui/display/BUILD.gn
File ui/display/BUILD.gn (right):

https://codereview.chromium.org/615133002/diff/260001/ui/display/BUILD.gn#new...
ui/display/BUILD.gn:16: "chromeos/display_snapshot_virtual.cc",
nit: Keep files in order.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:150: display_state.display = new
DisplaySnapshotVirtual(
On 2015/02/23 23:14:53, Daniel Erat wrote:
> are you leaking this? DisplayState documents that it doesn't own its |display|
> member.

Yes, this needs to be kept track of. It is leaking right now.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1045: int64
DisplayConfigurator::GetVirtualDisplayId() {
You could remove this function. The caller can use the list of displays
(DisplayConfigurator::cached_displays()) to figure out if there are any virtual
displays.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:288: void
EnableVirtualDisplay(scoped_ptr<DisplayMode> display_mode);
On 2015/02/23 23:14:53, Daniel Erat wrote:
> passing in a DisplayMode seems a bit strange to me (do you really just need to
> pass the size?), but i'll let dnicoara@ comment on this.

Same, I think we should just pass in a size and not expose DisplayMode in the
interface.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:293: int64 GetVirtualDisplayId();
nit: Please use the 'int64_t' variant. This is the recommended format. Same in
other places.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:15: gfx::Size(200, 200),
On 2015/02/23 23:14:53, Daniel Erat wrote:
> where do these numbers come from?
> 
> (i have flashbacks of 200x200 dimensions from excessive gtk+ programming in
the
> past)

This would be the physical size of the display. We may want to have something
smarter here since Ash uses this and the resolution to compute the DPI.

Or at least have a TODO to revisit this.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:23: modes_.push_back(new
DisplayMode(mode->size(), mode->is_interlaced(),
This mode is leaking.

[oshima, 2015-02-24 19:43:21]

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
File content/browser/media/capture/DEPS (right):

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
content/browser/media/capture/DEPS:6: "+ash/shell.h",
content shouldn't depend on ash/

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:233: bool
DesktopVideoCaptureMachine::Start(
On 2015/02/23 23:16:12, Sergey Ulanov wrote:
> Can the virtual-screen logic be moved out of this class? I think it can be
> implemented as follows:
>   - Change DesktopVideoCaptureMachine constructor to take aura::Window instead
> of DesktopMediaID.
>   - Change DesktopCaptureDeviceAura constructor to take aura::Window instead
of
> DesktopMediaID.
>   - Add a new class called e.g. VirtualScreenCapturer that implements
> media::VideoCaptureDevice and wraps DesktopCaptureDeviceAura, which actually
> captures frames.
> 
> That way you will need fewer ifdefs and the code will be easier to read.

and please move ash code from content/.

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:242: #if
defined(USE_ASH)
no USE_ASH in content/. You probably should just use OS_CHROMEOS because this
won't work on win ash.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1026: scoped_ptr<DisplayMode>
display_mode) {
DCHECK(!IsVirtualDisplayEnabled()) << "...".

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1029: ConfigureDisplays();
Is it harmful to reconfigure?

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1037: if (IsVirtualDisplayEnabled())
{
ditto

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:19: 
private:

[robert.bradford, 2015-03-03 19:58:01]

Thanks everyone for your reviews. I was out travelling this last week so wasn't
able to respond as quickly as i'd like.

All the superficial issues have been addressed and I have a refactor patch in
progress to enable Sergey's idea to simplify the code
(https://codereview.chromium.org/974513002/)

But I still have two problems remaining, both around removing the ash use:

For the DisplayConfigurator I think a LazyInstance singleton like this patch
might work: https://codereview.chromium.org/972153004/

But i'm not sure how to solve the mapping from display ID to window that i'm
currently doing with the DisplayController.

Looking forward to hearing your thoughts on these.

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/615133002/diff/260001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:233: bool
DesktopVideoCaptureMachine::Start(
On 2015/02/23 23:16:12, Sergey Ulanov wrote:
> Can the virtual-screen logic be moved out of this class? I think it can be
> implemented as follows:
>   - Change DesktopVideoCaptureMachine constructor to take aura::Window instead
> of DesktopMediaID.
>   - Change DesktopCaptureDeviceAura constructor to take aura::Window instead
of
> DesktopMediaID.
>   - Add a new class called e.g. VirtualScreenCapturer that implements
> media::VideoCaptureDevice and wraps DesktopCaptureDeviceAura, which actually
> captures frames.
> 
> That way you will need fewer ifdefs and the code will be easier to read.

I've started working on the non virtual display refactoring here:
https://codereview.chromium.org/974513002/

https://codereview.chromium.org/615133002/diff/260001/ui/display/BUILD.gn
File ui/display/BUILD.gn (right):

https://codereview.chromium.org/615133002/diff/260001/ui/display/BUILD.gn#new...
ui/display/BUILD.gn:16: "chromeos/display_snapshot_virtual.cc",
On 2015/02/24 16:27:06, dnicoara wrote:
> nit: Keep files in order.

Done.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:150: display_state.display = new
DisplaySnapshotVirtual(
On 2015/02/24 16:27:06, dnicoara wrote:
> On 2015/02/23 23:14:53, Daniel Erat wrote:
> > are you leaking this? DisplayState documents that it doesn't own its
|display|
> > member.
> 
> Yes, this needs to be kept track of. It is leaking right now.

Refactored this so that DisplayConfigurator has a scoped_ptr that for the
DisplaySnapshotVirtual thus avoiding this leak.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1026: scoped_ptr<DisplayMode>
display_mode) {
On 2015/02/24 19:43:21, oshima wrote:
> DCHECK(!IsVirtualDisplayEnabled()) << "...".

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1029: ConfigureDisplays();
On 2015/02/24 19:43:21, oshima wrote:
> Is it harmful to reconfigure?

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.cc:1037: if (IsVirtualDisplayEnabled())
{
On 2015/02/24 19:43:21, oshima wrote:
> ditto
> 

Done.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:287: // Enable/disable virtual
output.
On 2015/02/23 23:14:53, Daniel Erat wrote:
> nit: Change to "Enables/disables" to match style of other comments

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:288: void
EnableVirtualDisplay(scoped_ptr<DisplayMode> display_mode);
On 2015/02/24 16:27:06, dnicoara wrote:
> On 2015/02/23 23:14:53, Daniel Erat wrote:
> > passing in a DisplayMode seems a bit strange to me (do you really just need
to
> > pass the size?), but i'll let dnicoara@ comment on this.
> 
> Same, I think we should just pass in a size and not expose DisplayMode in the
> interface.

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:290: bool IsVirtualDisplayEnabled()
const { return virtual_display_mode_.get(); }
On 2015/02/23 23:14:53, Daniel Erat wrote:
> if this is inlined, it should be named like is_virtual_display_enabled(). it'd
> be better to leave it named like this and not inline it, though. see
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_configurator.h:293: int64 GetVirtualDisplayId();
On 2015/02/24 16:27:06, dnicoara wrote:
> nit: Please use the 'int64_t' variant. This is the recommended format. Same in
> other places.

Done.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:15: gfx::Size(200, 200),
On 2015/02/24 16:27:06, dnicoara wrote:
> On 2015/02/23 23:14:53, Daniel Erat wrote:
> > where do these numbers come from?
> > 
> > (i have flashbacks of 200x200 dimensions from excessive gtk+ programming in
> the
> > past)
> 
> This would be the physical size of the display. We may want to have something
> smarter here since Ash uses this and the resolution to compute the DPI.
> 
> Or at least have a TODO to revisit this.

I've put a TODO in here for now but would love to hear thoughts on how we could
improve this or maybe just tweak the constants.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.cc:23: modes_.push_back(new
DisplayMode(mode->size(), mode->is_interlaced(),
On 2015/02/24 16:27:06, dnicoara wrote:
> This mode is leaking.

Fixed by adding a scoped_ptr member to store the internal mode after contruction
via a gfx::Size.

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

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:12: class DisplaySnapshotVirtual
: public DisplaySnapshot {
On 2015/02/23 23:14:54, Daniel Erat wrote:
> add a comment documenting what this class is for

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:14:
DisplaySnapshotVirtual(uint32_t index, DisplayMode* mode);
On 2015/02/23 23:14:54, Daniel Erat wrote:
> add a comment documenting what |index| is and whether ownership of |mode| is
> passed (and if it is, used scoped_ptr instead)

The constructor prototype now looks like this:

DisplaySnapshotVirtual(int64_t display_id, gfx::Size display_size);

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_snapshot_virtual.h:19: 
On 2015/02/24 19:43:21, oshima wrote:
> private:

Done.

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
File ui/display/chromeos/display_util.h (right):

https://codereview.chromium.org/615133002/diff/260001/ui/display/chromeos/dis...
ui/display/chromeos/display_util.h:30: bool
IsNativeDisplay(ui::DisplayConnectionType type);
On 2015/02/23 23:14:54, Daniel Erat wrote:
> add a comment. also, i think it's confusing to use "Native" here. in other
> contexts, like ui/gfx/native_widget_types.h, "native" means platform-specific.
i
> think that something like IsPhysicalDisplay() or IsRealDisplay() would be more
> appropriate here.

Done. Moved to IsPhysicalDisplayType() with an appropriate comment as. The Type
suffix as it does not take a display but instead the connection type.

[oshima, 2015-03-11 22:45:33]

can you upload the updated patch? (the latest is still 3 weeks old)

[robert.bradford, 2015-03-12 17:48:41]

Hi folks, it took me some time to work out how to remove the deps on ash from
content/. The best solution I came up with is to have a singleton that lives
inside ash (spawned from the DesktopMediaListAsh) that listens for the display
being added and updates the AuraWindow in the
content::DesktopMediaID::AuraWindowRegistry. 

I'm not convinced this is the best design ever and would love to hear other
ideas on how to solve this problem.

This change also deps on https://codereview.chromium.org/972153004/ to make
ui::DisplayConfigurator a singleton and
https://codereview.chromium.org/974513002/ to remove all the #ifdefs from
desktop_capture_device_aura.cc

Thank you for your patience and I look forward to hearing your review comments.

[robert.bradford, 2015-03-12 17:48:42]

Hi folks, it took me some time to work out how to remove the deps on ash from
content/. The best solution I came up with is to have a singleton that lives
inside ash (spawned from the DesktopMediaListAsh) that listens for the display
being added and updates the AuraWindow in the
content::DesktopMediaID::AuraWindowRegistry. 

I'm not convinced this is the best design ever and would love to hear other
ideas on how to solve this problem.

This change also deps on https://codereview.chromium.org/972153004/ to make
ui::DisplayConfigurator a singleton and
https://codereview.chromium.org/974513002/ to remove all the #ifdefs from
desktop_capture_device_aura.cc

Thank you for your patience and I look forward to hearing your review comments.

[achuithb, 2015-05-11 19:51:29]

Is this CL still being worked on?

[robert.bradford, 2016-01-28 15:46:45]

Virtual display control was landed in http://crrev.com/364332

[robert.bradford, 2016-01-28 15:46:55]

Description was changed from

==========
Add support for a virtual display on ChromeOS

The purpose of this feature is to extend the desktop capture functionality by
allowing the creation of a virtual screen at capture request time. The use case
here is to mimic, with e.g. a Chrome Cast, the behaviour that can be achieved
with a physically connected projector i.e that a presenter can have slides
showing whilst keeping notes on their own private screen.

The "Virtual Display" appears as entry in the display picker, with a new
DesktopMediaID type. In the DesktopCaptureDeviceAura if an item of this type is
requested for capture the virtual screen is enabled in the DisplayConfigurator
and the resulting Aura root window used for capture.

BUG=425060
TEST=Use desktopCapture example extension, choose the Virtual Screen when
prompted and observe a new virtual display is created. When the capture is
stopped observe that the virtual display disappears.
==========

to

==========
Add support for a virtual display on ChromeOS

The purpose of this feature is to extend the desktop capture functionality by
allowing the creation of a virtual screen at capture request time. The use case
here is to mimic, with e.g. a Chrome Cast, the behaviour that can be achieved
with a physically connected projector i.e that a presenter can have slides
showing whilst keeping notes on their own private screen.

The "Virtual Display" appears as entry in the display picker, with a new
DesktopMediaID type. In the DesktopCaptureDeviceAura if an item of this type is
requested for capture the virtual screen is enabled in the DisplayConfigurator
and the resulting Aura root window used for capture.

BUG=425060
TEST=Use desktopCapture example extension, choose the Virtual Screen when
prompted and observe a new virtual display is created. When the capture is
stopped observe that the virtual display disappears.
==========