Split ui/display types into separate module and have Ozone depend on it

Split ui/display types into separate module and have Ozone depend on it

New dependency model:
ui/ozone -> ui/display/types
ui/display -> ui/ozone
ui/display -> ui/display/types

Note ui/display/types is its own module separate from ui/display.

TBR=derat@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263865

[dnicoara, 2014-04-09 15:11:19]

spang@ suggested a refactoring of the dependency injection model currently used
in Ozone. This CL tries to show the proposed changes.

Adding Ozone & display OWNERS for concerns & questions.

For clarity I'm copying spang@'s proposal:

A dependency on an injection done from ui/ozone is still a dependency on
ui/ozone. So ui/display needs to depend on ui/ozone, not the other way around.

We should try to rearrange ui/display to have an ozone-friendly dependency
structure so that we do not introduce another component with broken
dependencies.

In this instance, something like:

  ui/display/types [defines NativeDisplayDelegate interface]
  ui/display [defines output configurator & instantiates NDD]
  ui/ozone [knows how to build the NDD]

With these deps:

  ui/display -> ui/ozone -> ui/display/types

OutputConfigurator then just does:

OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();

Bonus: You can actually test ui/display without the extreme pain of trying to
set all the ozone singletons without actually be able to use ui/ozone.

[spang, 2014-04-09 15:45:02]

On 2014/04/09 15:11:19, dnicoara wrote:
> spang@ suggested a refactoring of the dependency injection model currently
used
> in Ozone. This CL tries to show the proposed changes.
> 
> Adding Ozone & display OWNERS for concerns & questions.
> 
> For clarity I'm copying spang@'s proposal:
> 
> A dependency on an injection done from ui/ozone is still a dependency on
> ui/ozone. So ui/display needs to depend on ui/ozone, not the other way around.
> 
> We should try to rearrange ui/display to have an ozone-friendly dependency
> structure so that we do not introduce another component with broken
> dependencies.
> 
> In this instance, something like:
> 
>   ui/display/types [defines NativeDisplayDelegate interface]
>   ui/display [defines output configurator & instantiates NDD]
>   ui/ozone [knows how to build the NDD]
> 
> With these deps:
> 
>   ui/display -> ui/ozone -> ui/display/types
> 
> OutputConfigurator then just does:
> 
> OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();
> 
> Bonus: You can actually test ui/display without the extreme pain of trying to
> set all the ozone singletons without actually be able to use ui/ozone.

Looks good, I think we should do it this way if we can.

I'm not sure about the chromeos ifdefs within ui/ozone. Having
GetNativeDisplayDelegate on the base class when OS_CHROMEOS is not set could be
a source of confusion. Having the ifdef makes it very clear this is only used on
ChromeOS.

So: I think I liked it better the other way. It's more ifdefs, but it's less
misleading.

rjkroege?

[rjkroege, 2014-04-09 15:49:50]

On 2014/04/09 15:11:19, dnicoara wrote:
> spang@ suggested a refactoring of the dependency injection model currently
used
> in Ozone. This CL tries to show the proposed changes.
> 
> Adding Ozone & display OWNERS for concerns & questions.
> 
> For clarity I'm copying spang@'s proposal:
> 
> A dependency on an injection done from ui/ozone is still a dependency on
> ui/ozone. So ui/display needs to depend on ui/ozone, not the other way around.

I strongly agree with this.

> 
> We should try to rearrange ui/display to have an ozone-friendly dependency
> structure so that we do not introduce another component with broken
> dependencies.
> 
> In this instance, something like:
> 
>   ui/display/types [defines NativeDisplayDelegate interface]
>   ui/display [defines output configurator & instantiates NDD]
>   ui/ozone [knows how to build the NDD]
> 
> With these deps:
> 
>   ui/display -> ui/ozone -> ui/display/types


But I'm not entirely pleased with this but maybe I just need to be re-educated.
ui/ozone already depends on ui/gfx where we have a gfx::Display and gfx::Screen.
Could we meld gfx::Display with DisplaySnapshot -- it feels to me that they have
enough semantic overlap that they could/should be connected together? Shouldn't
the top-level interface belong in ui/gfx and ChromeOS (or embedded=1) could
provide functionality via a platform-specific delegate? Hand-wavy analogue to
the way of the aura window delegate scheme...

> 
> OutputConfigurator then just does:
> 
> OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();

yes. 

> 
> Bonus: You can actually test ui/display without the extreme pain of trying to
> set all the ozone singletons without actually be able to use ui/ozone.

Awesome bonus.

[spang, 2014-04-09 16:00:50]

On 2014/04/09 15:49:50, rjkroege wrote:
> On 2014/04/09 15:11:19, dnicoara wrote:
> > spang@ suggested a refactoring of the dependency injection model currently
> used
> > in Ozone. This CL tries to show the proposed changes.
> > 
> > Adding Ozone & display OWNERS for concerns & questions.
> > 
> > For clarity I'm copying spang@'s proposal:
> > 
> > A dependency on an injection done from ui/ozone is still a dependency on
> > ui/ozone. So ui/display needs to depend on ui/ozone, not the other way
around.
> 
> I strongly agree with this.
> 
> > 
> > We should try to rearrange ui/display to have an ozone-friendly dependency
> > structure so that we do not introduce another component with broken
> > dependencies.
> > 
> > In this instance, something like:
> > 
> >   ui/display/types [defines NativeDisplayDelegate interface]
> >   ui/display [defines output configurator & instantiates NDD]
> >   ui/ozone [knows how to build the NDD]
> > 
> > With these deps:
> > 
> >   ui/display -> ui/ozone -> ui/display/types
> 
> 
> But I'm not entirely pleased with this but maybe I just need to be
re-educated.
> ui/ozone already depends on ui/gfx where we have a gfx::Display and
gfx::Screen.
> Could we meld gfx::Display with DisplaySnapshot -- it feels to me that they
have
> enough semantic overlap that they could/should be connected together?
Shouldn't
> the top-level interface belong in ui/gfx and ChromeOS (or embedded=1) could
> provide functionality via a platform-specific delegate? Hand-wavy analogue to
> the way of the aura window delegate scheme...

You're saying we should move NativeDisplayDelegate & friends to ui/gfx/chromeos,
or something like that?

That would also work fine, and I don't have a strong preference, but I imagine
the ui/display OWNERS would like to own this code.

> 
> > 
> > OutputConfigurator then just does:
> > 
> > OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();
> 
> yes. 
> 
> > 
> > Bonus: You can actually test ui/display without the extreme pain of trying
to
> > set all the ozone singletons without actually be able to use ui/ozone.
> 
> Awesome bonus.

[dnicoara, 2014-04-09 16:04:56]

On 2014/04/09 16:00:50, spang wrote:
> On 2014/04/09 15:49:50, rjkroege wrote:
> > On 2014/04/09 15:11:19, dnicoara wrote:
> > > spang@ suggested a refactoring of the dependency injection model currently
> > used
> > > in Ozone. This CL tries to show the proposed changes.
> > > 
> > > Adding Ozone & display OWNERS for concerns & questions.
> > > 
> > > For clarity I'm copying spang@'s proposal:
> > > 
> > > A dependency on an injection done from ui/ozone is still a dependency on
> > > ui/ozone. So ui/display needs to depend on ui/ozone, not the other way
> around.
> > 
> > I strongly agree with this.
> > 
> > > 
> > > We should try to rearrange ui/display to have an ozone-friendly dependency
> > > structure so that we do not introduce another component with broken
> > > dependencies.
> > > 
> > > In this instance, something like:
> > > 
> > >   ui/display/types [defines NativeDisplayDelegate interface]
> > >   ui/display [defines output configurator & instantiates NDD]
> > >   ui/ozone [knows how to build the NDD]
> > > 
> > > With these deps:
> > > 
> > >   ui/display -> ui/ozone -> ui/display/types
> > 
> > 
> > But I'm not entirely pleased with this but maybe I just need to be
> re-educated.
> > ui/ozone already depends on ui/gfx where we have a gfx::Display and
> gfx::Screen.
> > Could we meld gfx::Display with DisplaySnapshot -- it feels to me that they
> have
> > enough semantic overlap that they could/should be connected together?
> Shouldn't
> > the top-level interface belong in ui/gfx and ChromeOS (or embedded=1) could
> > provide functionality via a platform-specific delegate? Hand-wavy analogue
to
> > the way of the aura window delegate scheme...
> 
> You're saying we should move NativeDisplayDelegate & friends to
ui/gfx/chromeos,
> or something like that?
> 
> That would also work fine, and I don't have a strong preference, but I imagine
> the ui/display OWNERS would like to own this code.
> 

I don't think this would fly. I believe the idea is to move the display bits
from ui/gfx to ui/display and consolidate them here.

> > 
> > > 
> > > OutputConfigurator then just does:
> > > 
> > > OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();
> > 
> > yes. 
> > 
> > > 
> > > Bonus: You can actually test ui/display without the extreme pain of trying
> to
> > > set all the ozone singletons without actually be able to use ui/ozone.
> > 
> > Awesome bonus.

[oshima, 2014-04-09 16:42:46]

On Wed, Apr 9, 2014 at 9:04 AM, <dnicoara@chromium.org> wrote:

> On 2014/04/09 16:00:50, spang wrote:
>
>> On 2014/04/09 15:49:50, rjkroege wrote:
>> > On 2014/04/09 15:11:19, dnicoara wrote:
>> > > spang@ suggested a refactoring of the dependency injection model
>> currently
>> > used
>> > > in Ozone. This CL tries to show the proposed changes.
>> > >
>> > > Adding Ozone & display OWNERS for concerns & questions.
>> > >
>> > > For clarity I'm copying spang@'s proposal:
>> > >
>> > > A dependency on an injection done from ui/ozone is still a dependency
>> on
>> > > ui/ozone. So ui/display needs to depend on ui/ozone, not the other way
>> around.
>> >
>> > I strongly agree with this.
>> >
>> > >
>> > > We should try to rearrange ui/display to have an ozone-friendly
>> dependency
>> > > structure so that we do not introduce another component with broken
>> > > dependencies.
>> > >
>> > > In this instance, something like:
>> > >
>> > >   ui/display/types [defines NativeDisplayDelegate interface]
>> > >   ui/display [defines output configurator & instantiates NDD]
>> > >   ui/ozone [knows how to build the NDD]
>> > >
>> > > With these deps:
>> > >
>> > >   ui/display -> ui/ozone -> ui/display/types
>> >
>> >
>> > But I'm not entirely pleased with this but maybe I just need to be
>> re-educated.
>> > ui/ozone already depends on ui/gfx where we have a gfx::Display and
>> gfx::Screen.
>> > Could we meld gfx::Display with DisplaySnapshot -- it feels to me that
>> they
>> have
>> > enough semantic overlap that they could/should be connected together?
>> Shouldn't
>> > the top-level interface belong in ui/gfx and ChromeOS (or embedded=1)
>> could
>> > provide functionality via a platform-specific delegate? Hand-wavy
>> analogue
>>
> to
>
>> > the way of the aura window delegate scheme...
>>
>
>  You're saying we should move NativeDisplayDelegate & friends to
>>
> ui/gfx/chromeos,
>
>> or something like that?
>>
>
>  That would also work fine, and I don't have a strong preference, but I
>> imagine
>> the ui/display OWNERS would like to own this code.
>>
>
>
> I don't think this would fly. I believe the idea is to move the display
> bits
> from ui/gfx to ui/display and consolidate them here.
>
>
Do you mean this CL will be abandoned? (Just want to know if I should read
 through comments and code or just ignore this)


>
>  >
>> > >
>> > > OutputConfigurator then just does:
>> > >
>> > > OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();
>> >
>> > yes.
>> >
>> > >
>> > > Bonus: You can actually test ui/display without the extreme pain of
>> trying
>> to
>> > > set all the ozone singletons without actually be able to use ui/ozone.
>> >
>> > Awesome bonus.
>>
>
>
>
> https://codereview.chromium.org/230763004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dnicoara, 2014-04-09 16:45:39]

On 2014/04/09 16:42:46, oshima wrote:
> On Wed, Apr 9, 2014 at 9:04 AM, <mailto:dnicoara@chromium.org> wrote:
> 
> > On 2014/04/09 16:00:50, spang wrote:
> >
> >> On 2014/04/09 15:49:50, rjkroege wrote:
> >> > On 2014/04/09 15:11:19, dnicoara wrote:
> >> > > spang@ suggested a refactoring of the dependency injection model
> >> currently
> >> > used
> >> > > in Ozone. This CL tries to show the proposed changes.
> >> > >
> >> > > Adding Ozone & display OWNERS for concerns & questions.
> >> > >
> >> > > For clarity I'm copying spang@'s proposal:
> >> > >
> >> > > A dependency on an injection done from ui/ozone is still a dependency
> >> on
> >> > > ui/ozone. So ui/display needs to depend on ui/ozone, not the other way
> >> around.
> >> >
> >> > I strongly agree with this.
> >> >
> >> > >
> >> > > We should try to rearrange ui/display to have an ozone-friendly
> >> dependency
> >> > > structure so that we do not introduce another component with broken
> >> > > dependencies.
> >> > >
> >> > > In this instance, something like:
> >> > >
> >> > >   ui/display/types [defines NativeDisplayDelegate interface]
> >> > >   ui/display [defines output configurator & instantiates NDD]
> >> > >   ui/ozone [knows how to build the NDD]
> >> > >
> >> > > With these deps:
> >> > >
> >> > >   ui/display -> ui/ozone -> ui/display/types
> >> >
> >> >
> >> > But I'm not entirely pleased with this but maybe I just need to be
> >> re-educated.
> >> > ui/ozone already depends on ui/gfx where we have a gfx::Display and
> >> gfx::Screen.
> >> > Could we meld gfx::Display with DisplaySnapshot -- it feels to me that
> >> they
> >> have
> >> > enough semantic overlap that they could/should be connected together?
> >> Shouldn't
> >> > the top-level interface belong in ui/gfx and ChromeOS (or embedded=1)
> >> could
> >> > provide functionality via a platform-specific delegate? Hand-wavy
> >> analogue
> >>
> > to
> >
> >> > the way of the aura window delegate scheme...
> >>
> >
> >  You're saying we should move NativeDisplayDelegate & friends to
> >>
> > ui/gfx/chromeos,
> >
> >> or something like that?
> >>
> >
> >  That would also work fine, and I don't have a strong preference, but I
> >> imagine
> >> the ui/display OWNERS would like to own this code.
> >>
> >
> >
> > I don't think this would fly. I believe the idea is to move the display
> > bits
> > from ui/gfx to ui/display and consolidate them here.
> >
> >
> Do you mean this CL will be abandoned? (Just want to know if I should read
>  through comments and code or just ignore this)
> 

I was referring to spang@'s comment on moving the display types to ui/gfx.

Please take a look at both the comments & changes.

> 
> >
> >  >
> >> > >
> >> > > OutputConfigurator then just does:
> >> > >
> >> > > OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();
> >> >
> >> > yes.
> >> >
> >> > >
> >> > > Bonus: You can actually test ui/display without the extreme pain of
> >> trying
> >> to
> >> > > set all the ozone singletons without actually be able to use ui/ozone.
> >> >
> >> > Awesome bonus.
> >>
> >
> >
> >
> > https://codereview.chromium.org/230763004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-04-09 16:51:35]

On 2014/04/09 16:45:39, dnicoara wrote:
> On 2014/04/09 16:42:46, oshima wrote:
> > On Wed, Apr 9, 2014 at 9:04 AM, <mailto:dnicoara@chromium.org> wrote:
> > 
> > > On 2014/04/09 16:00:50, spang wrote:
> > >
> > >> On 2014/04/09 15:49:50, rjkroege wrote:
> > >> > On 2014/04/09 15:11:19, dnicoara wrote:
> > >> > > spang@ suggested a refactoring of the dependency injection model
> > >> currently
> > >> > used
> > >> > > in Ozone. This CL tries to show the proposed changes.
> > >> > >
> > >> > > Adding Ozone & display OWNERS for concerns & questions.
> > >> > >
> > >> > > For clarity I'm copying spang@'s proposal:
> > >> > >
> > >> > > A dependency on an injection done from ui/ozone is still a dependency
> > >> on
> > >> > > ui/ozone. So ui/display needs to depend on ui/ozone, not the other
way
> > >> around.
> > >> >
> > >> > I strongly agree with this.
> > >> >
> > >> > >
> > >> > > We should try to rearrange ui/display to have an ozone-friendly
> > >> dependency
> > >> > > structure so that we do not introduce another component with broken
> > >> > > dependencies.
> > >> > >
> > >> > > In this instance, something like:
> > >> > >
> > >> > >   ui/display/types [defines NativeDisplayDelegate interface]
> > >> > >   ui/display [defines output configurator & instantiates NDD]
> > >> > >   ui/ozone [knows how to build the NDD]
> > >> > >
> > >> > > With these deps:
> > >> > >
> > >> > >   ui/display -> ui/ozone -> ui/display/types
> > >> >
> > >> >
> > >> > But I'm not entirely pleased with this but maybe I just need to be
> > >> re-educated.
> > >> > ui/ozone already depends on ui/gfx where we have a gfx::Display and
> > >> gfx::Screen.
> > >> > Could we meld gfx::Display with DisplaySnapshot -- it feels to me that
> > >> they
> > >> have
> > >> > enough semantic overlap that they could/should be connected together?
> > >> Shouldn't
> > >> > the top-level interface belong in ui/gfx and ChromeOS (or embedded=1)
> > >> could
> > >> > provide functionality via a platform-specific delegate? Hand-wavy
> > >> analogue
> > >>
> > > to
> > >
> > >> > the way of the aura window delegate scheme...
> > >>
> > >
> > >  You're saying we should move NativeDisplayDelegate & friends to
> > >>
> > > ui/gfx/chromeos,
> > >
> > >> or something like that?
> > >>
> > >
> > >  That would also work fine, and I don't have a strong preference, but I
> > >> imagine
> > >> the ui/display OWNERS would like to own this code.
> > >>
> > >
> > >
> > > I don't think this would fly. I believe the idea is to move the display
> > > bits
> > > from ui/gfx to ui/display and consolidate them here.
> > >
> > >
> > Do you mean this CL will be abandoned? (Just want to know if I should read
> >  through comments and code or just ignore this)
> > 
> 
> I was referring to spang@'s comment on moving the display types to ui/gfx.
> 
> Please take a look at both the comments & changes.
> 

oshima: In particular we are fishing for OWNERS approval for split ui/display
into two pieces so that it can work well for ozone builds.

ozone is different than existing platforms because all platform code is
late-bound and defined in its own component (ui/ozone).

So ozone needs to reference the display-related platform types, but ui/display
needs ozone to instantiate the NativeDisplayDelegate. This is a dependency cycle
unless the platform-implemented types are split away from ui/display.

> > 
> > >
> > >  >
> > >> > >
> > >> > > OutputConfigurator then just does:
> > >> > >
> > >> > > OzonePlatform::GetInstance()->CreateNativeDisplayDelegate();
> > >> >
> > >> > yes.
> > >> >
> > >> > >
> > >> > > Bonus: You can actually test ui/display without the extreme pain of
> > >> trying
> > >> to
> > >> > > set all the ozone singletons without actually be able to use
ui/ozone.
> > >> >
> > >> > Awesome bonus.
> > >>
> > >
> > >
> > >
> > > https://codereview.chromium.org/230763004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Daniel Erat, 2014-04-09 20:31:02]

i don't think i have any objections to this approach.

[spang, 2014-04-09 22:31:48]

https://codereview.chromium.org/230763004/diff/1/ui/display/types/DEPS
File ui/display/types/DEPS (right):

https://codereview.chromium.org/230763004/diff/1/ui/display/types/DEPS#newcode3
ui/display/types/DEPS:3: "+ui/display/display_export.h",
You need a new

display_types_export.h

for the new component. Otherwise the component build won't catch dependency
errors between these two components.

[spang, 2014-04-09 22:33:32]

https://codereview.chromium.org/230763004/diff/1/ui/display/display.gyp
File ui/display/display.gyp (right):

https://codereview.chromium.org/230763004/diff/1/ui/display/display.gyp#newco...
ui/display/display.gyp:18: 'DISPLAY_IMPLEMENTATION',
DISPLAY_TYPES_IMPLEMENTATION

This relates to the DISPLAY_TYPES_EXPORT macro that you have to add.

[oshima, 2014-04-09 23:40:36]

i'm fine with it too. one q.

https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS
File ui/display/DEPS (right):

https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS#newcode4
ui/display/DEPS:4: "+ui/ozone",
where we need this? can this be moved to ui/display/ozone ?

[spang, 2014-04-09 23:47:08]

On 2014/04/09 23:40:36, oshima wrote:
> i'm fine with it too. one q.
> 
> https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS
> File ui/display/DEPS (right):
> 
> https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS#newcode4
> ui/display/DEPS:4: "+ui/ozone",
> where we need this? can this be moved to ui/display/ozone ?

No because this is for output_configurator, which is in ui/display/chromeos. The
relevant line is:

    native_display_delegate_.reset(
         OzonePlatform::GetInstance()->CreateNativeDisplayDelegate());

OzonePlatform is from ui/ozone.

[dnicoara, 2014-04-10 01:22:25]

https://codereview.chromium.org/230763004/diff/1/ui/display/display.gyp
File ui/display/display.gyp (right):

https://codereview.chromium.org/230763004/diff/1/ui/display/display.gyp#newco...
ui/display/display.gyp:18: 'DISPLAY_IMPLEMENTATION',
On 2014/04/09 22:33:33, spang wrote:
> DISPLAY_TYPES_IMPLEMENTATION
> 
> This relates to the DISPLAY_TYPES_EXPORT macro that you have to add.

Done.

https://codereview.chromium.org/230763004/diff/1/ui/display/types/DEPS
File ui/display/types/DEPS (right):

https://codereview.chromium.org/230763004/diff/1/ui/display/types/DEPS#newcode3
ui/display/types/DEPS:3: "+ui/display/display_export.h",
On 2014/04/09 22:31:49, spang wrote:
> You need a new
> 
> display_types_export.h
> 
> for the new component. Otherwise the component build won't catch dependency
> errors between these two components. 

Done.

[oshima, 2014-04-10 14:29:37]

On 2014/04/09 23:47:08, spang wrote:
> On 2014/04/09 23:40:36, oshima wrote:
> > i'm fine with it too. one q.
> > 
> > https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS
> > File ui/display/DEPS (right):
> > 
> > https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS#newcode4
> > ui/display/DEPS:4: "+ui/ozone",
> > where we need this? can this be moved to ui/display/ozone ?
> 
> No because this is for output_configurator, which is in ui/display/chromeos.
The
> relevant line is:
> 
>     native_display_delegate_.reset(
>          OzonePlatform::GetInstance()->CreateNativeDisplayDelegate());
> 
> OzonePlatform is from ui/ozone.

Init should take delegates. We can have a platform specific Init methods (in
ozone dir),
which creates their own impl and pass to Init. This way, we can eliminate ifdef
from
display_configurator.

[dnicoara, 2014-04-10 15:06:33]

On 2014/04/10 14:29:37, oshima wrote:
> On 2014/04/09 23:47:08, spang wrote:
> > On 2014/04/09 23:40:36, oshima wrote:
> > > i'm fine with it too. one q.
> > > 
> > > https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS
> > > File ui/display/DEPS (right):
> > > 
> > > https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS#newcode4
> > > ui/display/DEPS:4: "+ui/ozone",
> > > where we need this? can this be moved to ui/display/ozone ?
> > 
> > No because this is for output_configurator, which is in ui/display/chromeos.
> The
> > relevant line is:
> > 
> >     native_display_delegate_.reset(
> >          OzonePlatform::GetInstance()->CreateNativeDisplayDelegate());
> > 
> > OzonePlatform is from ui/ozone.
> 
> Init should take delegates. We can have a platform specific Init methods (in
> ozone dir),
> which creates their own impl and pass to Init. This way, we can eliminate
ifdef
> from
> display_configurator.

Perhaps I'm confused due to the amount of Init's in your proposal. I'm not sure
if what you're proposing involves setting the delegate when ash/shell.cc calls
DisplayConfigurator::Init?

My interpretation of your proposal would be to create a
DisplayConfigurator::PlatformInit() which is defined in ui/display/x11 and
ui/display/ozone and PlatformInit calls Init(native_display_delegate,
touchscreen_delegate). ash/shell.cc would then call PlatformInit(). Is this
close to what you were thinking?

[oshima, 2014-04-10 16:40:22]

On 2014/04/10 15:06:33, dnicoara wrote:
> On 2014/04/10 14:29:37, oshima wrote:
> > On 2014/04/09 23:47:08, spang wrote:
> > > On 2014/04/09 23:40:36, oshima wrote:
> > > > i'm fine with it too. one q.
> > > > 
> > > > https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS
> > > > File ui/display/DEPS (right):
> > > > 
> > > >
https://codereview.chromium.org/230763004/diff/1/ui/display/DEPS#newcode4
> > > > ui/display/DEPS:4: "+ui/ozone",
> > > > where we need this? can this be moved to ui/display/ozone ?
> > > 
> > > No because this is for output_configurator, which is in
ui/display/chromeos.
> > The
> > > relevant line is:
> > > 
> > >     native_display_delegate_.reset(
> > >          OzonePlatform::GetInstance()->CreateNativeDisplayDelegate());
> > > 
> > > OzonePlatform is from ui/ozone.
> > 
> > Init should take delegates. We can have a platform specific Init methods (in
> > ozone dir),
> > which creates their own impl and pass to Init. This way, we can eliminate
> ifdef
> > from
> > display_configurator.
> 
> Perhaps I'm confused due to the amount of Init's in your proposal. I'm not
sure
> if what you're proposing involves setting the delegate when ash/shell.cc calls
> DisplayConfigurator::Init?
> 
> My interpretation of your proposal would be to create a
> DisplayConfigurator::PlatformInit() which is defined in ui/display/x11 and
> ui/display/ozone and PlatformInit calls Init(native_display_delegate,
> touchscreen_delegate). ash/shell.cc would then call PlatformInit(). Is this
> close to what you were thinking?

Exactly how we create delegates and initialize DisplayController is outside of
the scope of my proposal,
but what you described should work too.

[dnicoara, 2014-04-10 19:56:31]

Updated based on oshima@'s advice. Also updated all remaining  unittest
dependencies & includes.

[oshima, 2014-04-10 22:31:56]

On 2014/04/10 19:56:31, dnicoara wrote:
> Updated based on oshima@'s advice. Also updated all remaining  unittest
> dependencies & includes.

ui/display, ash/ lgtm. Other reviews may prefer different ways to initialize
delegates
(a factory function like CreatePlatformOutputConfigurator() for exmaple).
I'll be fine with either way, so you can just follow their suggestion if any.

[spang, 2014-04-11 19:11:29]

On 2014/04/10 22:31:56, oshima (OOO April 11 - 21) wrote:
> On 2014/04/10 19:56:31, dnicoara wrote:
> > Updated based on oshima@'s advice. Also updated all remaining  unittest
> > dependencies & includes.
> 
> ui/display, ash/ lgtm. Other reviews may prefer different ways to initialize
> delegates
> (a factory function like CreatePlatformOutputConfigurator() for exmaple).
> I'll be fine with either way, so you can just follow their suggestion if any.

ui/ozone lgtm with nits

I do still think having any NativeDisplayDelegate references outside a ChromeOS
ifdef is wrong..

[dnicoara, 2014-04-11 19:49:06]

On 2014/04/11 19:11:29, spang wrote:
> On 2014/04/10 22:31:56, oshima (OOO April 11 - 21) wrote:
> > On 2014/04/10 19:56:31, dnicoara wrote:
> > > Updated based on oshima@'s advice. Also updated all remaining  unittest
> > > dependencies & includes.
> > 
> > ui/display, ash/ lgtm. Other reviews may prefer different ways to initialize
> > delegates
> > (a factory function like CreatePlatformOutputConfigurator() for exmaple).
> > I'll be fine with either way, so you can just follow their suggestion if
any.
> 
> ui/ozone lgtm with nits
> 
> I do still think having any NativeDisplayDelegate references outside a
ChromeOS
> ifdef is wrong..

Done.

[rjkroege, 2014-04-11 21:53:08]

I'm still not sure that I like this approach but if I have a better idea, I'll
bike-shed later. :-)

lgtm with nits

https://codereview.chromium.org/230763004/diff/100001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/230763004/diff/100001/ash/ash.gyp#newcode1071
ash/ash.gyp:1071: '../ui/display/display.gyp:display_types',
this is redundant as it comes from display?

https://codereview.chromium.org/230763004/diff/100001/ui/display/types/DEPS
File ui/display/types/DEPS (right):

https://codereview.chromium.org/230763004/diff/100001/ui/display/types/DEPS#n...
ui/display/types/DEPS:3: "+ui/display/types",
I didn't know that DEPS needed to include itself. I learn something new
everyday.

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone.gyp
File ui/ozone/ozone.gyp (right):

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone.gyp#newc...
ui/ozone/ozone.gyp:35: 'common/chromeos/native_display_delegate_ozone.cc',
adding a # excluded by filename_rules.gypi might help people still getting use
to the chrome way.

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone_platform.h
File ui/ozone/ozone_platform.h (right):

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone_platform...
ui/ozone/ozone_platform.h:14: #if defined(OS_CHROMEOS)
unnecessary? you can forward declare NativeDisplayDelegate

[dnicoara, 2014-04-14 14:43:03]

https://codereview.chromium.org/230763004/diff/100001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/230763004/diff/100001/ash/ash.gyp#newcode1071
ash/ash.gyp:1071: '../ui/display/display.gyp:display_types',
On 2014/04/11 21:53:09, rjkroege wrote:
> this is redundant as it comes from display?

This is needed since 'display' doesn't export its dependencies. Perhaps
'display_types' should be exported by 'display'?

https://codereview.chromium.org/230763004/diff/100001/ui/display/types/DEPS
File ui/display/types/DEPS (right):

https://codereview.chromium.org/230763004/diff/100001/ui/display/types/DEPS#n...
ui/display/types/DEPS:3: "+ui/display/types",
On 2014/04/11 21:53:09, rjkroege wrote:
> I didn't know that DEPS needed to include itself. I learn something new
> everyday.

It's because the exclusion rule is right above this one.

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone.gyp
File ui/ozone/ozone.gyp (right):

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone.gyp#newc...
ui/ozone/ozone.gyp:35: 'common/chromeos/native_display_delegate_ozone.cc',
On 2014/04/11 21:53:09, rjkroege wrote:
> adding a # excluded by filename_rules.gypi might help people still getting use
> to the chrome way.

Done.

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone_platform.h
File ui/ozone/ozone_platform.h (right):

https://codereview.chromium.org/230763004/diff/100001/ui/ozone/ozone_platform...
ui/ozone/ozone_platform.h:14: #if defined(OS_CHROMEOS)
On 2014/04/11 21:53:09, rjkroege wrote:
> unnecessary? you can forward declare NativeDisplayDelegate

Done.

[dnicoara, 2014-04-14 14:43:36]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-04-14 14:43:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/230763004/120001

[commit-bot: I haz the power, 2014-04-14 18:56:35]

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

[commit-bot: I haz the power, 2014-04-14 18:56:36]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[dnicoara, 2014-04-14 18:58:51]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-04-14 18:59:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/230763004/120001

[commit-bot: I haz the power, 2014-04-14 22:27:24]

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

[commit-bot: I haz the power, 2014-04-14 22:27:25]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[dnicoara, 2014-04-15 14:01:48]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-04-15 14:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/230763004/140001

[commit-bot: I haz the power, 2014-04-15 16:26:57]

Change committed as 263865