Refactoring display configuration state to allow generic state objects

Refactoring display configuration state to allow generic state objects

OutputConfigurator now uses NativeDisplayDelegate impls to create generic DisplaySnapshot and DisplayMode objects. OutputConfigurator interacts with these objects then calls into NativeDisplayDelegate to perform the actual configuration.

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

[dnicoara, 2014-03-05 00:06:34]

I was trying to keep this patch small, unfortunately there are a lot of unit
test changes as well. Suggestions on how to split it further are welcome.

I need to add more comments in places, but I thought it would be good if I can
get some comments in the meantime. Let me know what you think.

https://codereview.chromium.org/187073002/diff/1/ash/display/display_change_o...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/187073002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:130:
gfx::Display::SetInternalDisplayId(output.display->display_id());
Rather than have this here, I assign the display_id to index in
NativeDisplayDelegateX11 if we couldn't parse the EDID.

https://codereview.chromium.org/187073002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:157: int64 id =
output.display->display_id();
Same as above.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:65: const ui::DisplayMode* selected_mode;
Since this (and mirror_mode) is OutputConfigurator specific code I thought it
would be good to keep it here rather than in  DisplaySnapshot. I'm also not
convinced that we need to keep track of these, though I didn't want to add too
much complexity to this CL.

[Daniel Erat, 2014-03-05 01:56:11]

nice! this approach seems fine to me.

https://codereview.chromium.org/187073002/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/187073002/diff/1/ash/ash.gyp#newcode826
ash/ash.gyp:826: '../ui/display/display.gyp:display',
nit: keep this list alphabetized

https://codereview.chromium.org/187073002/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/chromeos.gyp#newcode30
chromeos/chromeos.gyp:30: '../ui/gfx/gfx.gyp:gfx_geometry',
i don't think that ui/gfx is currently permitted by chromeos/DEPS, but hopefully
it's okay to add...

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.cc:63: // Returns a string representation
of ui::DisplayMode.
nit: maybe add a TODO to move this to DisplayMode

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.cc:701: for (size_t i = 0; i <
cached_outputs_.size(); ++i) {
nit: remove curly brackets

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:42: typedef std::vector<const
ui::DisplayMode*> DisplayModeList;
doesn't look like you're using this anywhere in the header -- did you mean for
it to be an std::vector<InternalDisplayState> instead?

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:54: struct InternalDisplayState {
i think that the word "internal" here may be confusing, as it could imply that
this only applies to internal displays. "AdditionalDisplayState"?
"ExtraDisplayState"? or maybe just "DisplayState" is good enough?

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:97: // Queries the resolution
(|width|x|height|) in pixels
nit: update this comment

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:330: // |width| and |height| are the
width and height of the combined framebuffer.
nit: update this comment

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator_unittest.cc (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:39: std::string
DisplayModeDescription(const ui::DisplayMode& mode) {
nit: maybe rename to DisplayModeToString() to make it a bit more clear that this
identifier is a function rather than a class? ditto for
DisplaySnapshotDescription -> DisplaySnapshotToString.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:155: : log_(log),
configure_touchscreens_(false) {}
nit: one member per line

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:170: if
(configure_touchscreens_)
nit: add curly brackets to this outer if statement

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:384: static const
ui::DisplayMode kBigMode;
not sure how much this matters in tests, but we usually avoid non-POD static
members. these could just be made non-static and initialized in the c'tor, i
think.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:463:
GetFramebufferAction(kSmallMode.size(), &outputs_[0], NULL).c_str(),
it is cool to see these lines get unwrapped :-)

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_mode.h (right):

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_mode.h:14: class DISPLAY_EXPORT DisplayMode {
nit: add a short comment describing what this corresponds to (and maybe
mentioning that there are implementation-specific derived classes)

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/x11/disp...
File ui/display/chromeos/x11/display_mode_x11.cc (right):

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/x11/disp...
ui/display/chromeos/x11/display_mode_x11.cc:13: : DisplayMode(size, interlaced,
refresh_rate), mode_id_(mode_id) {}
nit: one member per line

https://codereview.chromium.org/187073002/diff/1/ui/display/display_util.h
File ui/display/display_util.h (right):

https://codereview.chromium.org/187073002/diff/1/ui/display/display_util.h#ne...
ui/display/display_util.h:16: OutputType GetOutputTypeFromName(const
std::string& name);
should this be chrome os (or X11)-specific?

[dnicoara, 2014-03-05 17:29:24]

+ oshima@ for opinion on inclusion of ui/gfx/geometry in chromeos/.

https://codereview.chromium.org/187073002/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/187073002/diff/1/ash/ash.gyp#newcode826
ash/ash.gyp:826: '../ui/display/display.gyp:display',
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: keep this list alphabetized

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/chromeos.gyp#newcode30
chromeos/chromeos.gyp:30: '../ui/gfx/gfx.gyp:gfx_geometry',
On 2014/03/05 01:56:12, Daniel Erat wrote:
> i don't think that ui/gfx is currently permitted by chromeos/DEPS, but
hopefully
> it's okay to add...

Last time I spoke with oshima@ about this, he said it should be fine to add a
dependency on gfx_geometry. I'm adding him to the reviews to confirm.

Also, I should talk to ben@ about moving ui/gfx/geometry out since it is just a
bunch of utility classes with no dependencies to ui/gfx.

Also, I think I found a loop-hole since presubmit didn't complain since
ui/display depends on ui/gfx/geometry and including display_mode.h and
display_snapshot.h brings in the geometry includes.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.cc:63: // Returns a string representation
of ui::DisplayMode.
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: maybe add a TODO to move this to DisplayMode

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.cc:701: for (size_t i = 0; i <
cached_outputs_.size(); ++i) {
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: remove curly brackets

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:42: typedef std::vector<const
ui::DisplayMode*> DisplayModeList;
On 2014/03/05 01:56:12, Daniel Erat wrote:
> doesn't look like you're using this anywhere in the header -- did you mean for
> it to be an std::vector<InternalDisplayState> instead?

Done.

I was using this in some of the ash/ files that depend on this. But on further
analysis it makes more sense to typedef as you've mentioned since it covers more
of the ash/ changes.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:54: struct InternalDisplayState {
On 2014/03/05 01:56:12, Daniel Erat wrote:
> i think that the word "internal" here may be confusing, as it could imply that
> this only applies to internal displays. "AdditionalDisplayState"?
> "ExtraDisplayState"? or maybe just "DisplayState" is good enough?

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:97: // Queries the resolution
(|width|x|height|) in pixels
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: update this comment

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator.h:330: // |width| and |height| are the
width and height of the combined framebuffer.
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: update this comment

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
File chromeos/display/output_configurator_unittest.cc (right):

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:39: std::string
DisplayModeDescription(const ui::DisplayMode& mode) {
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: maybe rename to DisplayModeToString() to make it a bit more clear that
this
> identifier is a function rather than a class? ditto for
> DisplaySnapshotDescription -> DisplaySnapshotToString.

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:155: : log_(log),
configure_touchscreens_(false) {}
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: one member per line

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:170: if
(configure_touchscreens_)
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: add curly brackets to this outer if statement

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:384: static const
ui::DisplayMode kBigMode;
On 2014/03/05 01:56:12, Daniel Erat wrote:
> not sure how much this matters in tests, but we usually avoid non-POD static
> members. these could just be made non-static and initialized in the c'tor, i
> think.

Done.

https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
chromeos/display/output_configurator_unittest.cc:463:
GetFramebufferAction(kSmallMode.size(), &outputs_[0], NULL).c_str(),
On 2014/03/05 01:56:12, Daniel Erat wrote:
> it is cool to see these lines get unwrapped :-)

:) Yes, looks much nicer.

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_mode.h (right):

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_mode.h:14: class DISPLAY_EXPORT DisplayMode {
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: add a short comment describing what this corresponds to (and maybe
> mentioning that there are implementation-specific derived classes)

Done.

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/x11/disp...
File ui/display/chromeos/x11/display_mode_x11.cc (right):

https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/x11/disp...
ui/display/chromeos/x11/display_mode_x11.cc:13: : DisplayMode(size, interlaced,
refresh_rate), mode_id_(mode_id) {}
On 2014/03/05 01:56:12, Daniel Erat wrote:
> nit: one member per line

Done.

https://codereview.chromium.org/187073002/diff/1/ui/display/display_util.h
File ui/display/display_util.h (right):

https://codereview.chromium.org/187073002/diff/1/ui/display/display_util.h#ne...
ui/display/display_util.h:16: OutputType GetOutputTypeFromName(const
std::string& name);
On 2014/03/05 01:56:12, Daniel Erat wrote:
> should this be chrome os (or X11)-specific?

I moved it under ui/display/chromeos for now. If we needed it for other builds
we can move it back to ui/display. Luckily this will be useful for Ozone as
well, so not X11 specific.

[dnicoara, 2014-03-05 19:52:45]

https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
File ui/display/display.gyp (right):

https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
ui/display/display.gyp:66: 'target_name': 'display_unittests',
I'm wondering if this needs to be added to a list or target to be picked up by
try-bots?

[oshima, 2014-03-05 20:00:40]

I haven't looked into code yet (pretty busy memory sheriff rotation + 34/P1
bugs) but
I'd like to avoid having ui dependency in chromeos if possible, and that's one
of reason why we're moving
code to ui/display?

Or do we want to keep some of the code under chrmoeos/ ?


On 2014/03/05 17:29:24, dnicoara wrote:
> + oshima@ for opinion on inclusion of ui/gfx/geometry in chromeos/.
> 
> https://codereview.chromium.org/187073002/diff/1/ash/ash.gyp
> File ash/ash.gyp (right):
> 
> https://codereview.chromium.org/187073002/diff/1/ash/ash.gyp#newcode826
> ash/ash.gyp:826: '../ui/display/display.gyp:display',
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: keep this list alphabetized
> 
> Done.
> 
> https://codereview.chromium.org/187073002/diff/1/chromeos/chromeos.gyp
> File chromeos/chromeos.gyp (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/chromeos.gyp#newcode30
> chromeos/chromeos.gyp:30: '../ui/gfx/gfx.gyp:gfx_geometry',
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > i don't think that ui/gfx is currently permitted by chromeos/DEPS, but
> hopefully
> > it's okay to add...
> 
> Last time I spoke with oshima@ about this, he said it should be fine to add a
> dependency on gfx_geometry. I'm adding him to the reviews to confirm.
> 
> Also, I should talk to ben@ about moving ui/gfx/geometry out since it is just
a
> bunch of utility classes with no dependencies to ui/gfx.
> 
> Also, I think I found a loop-hole since presubmit didn't complain since
> ui/display depends on ui/gfx/geometry and including display_mode.h and
> display_snapshot.h brings in the geometry includes.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> File chromeos/display/output_configurator.cc (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator.cc:63: // Returns a string representation
> of ui::DisplayMode.
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: maybe add a TODO to move this to DisplayMode
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator.cc:701: for (size_t i = 0; i <
> cached_outputs_.size(); ++i) {
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: remove curly brackets
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> File chromeos/display/output_configurator.h (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator.h:42: typedef std::vector<const
> ui::DisplayMode*> DisplayModeList;
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > doesn't look like you're using this anywhere in the header -- did you mean
for
> > it to be an std::vector<InternalDisplayState> instead?
> 
> Done.
> 
> I was using this in some of the ash/ files that depend on this. But on further
> analysis it makes more sense to typedef as you've mentioned since it covers
more
> of the ash/ changes.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator.h:54: struct InternalDisplayState {
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > i think that the word "internal" here may be confusing, as it could imply
that
> > this only applies to internal displays. "AdditionalDisplayState"?
> > "ExtraDisplayState"? or maybe just "DisplayState" is good enough?
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator.h:97: // Queries the resolution
> (|width|x|height|) in pixels
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: update this comment
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator.h:330: // |width| and |height| are the
> width and height of the combined framebuffer.
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: update this comment
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> File chromeos/display/output_configurator_unittest.cc (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator_unittest.cc:39: std::string
> DisplayModeDescription(const ui::DisplayMode& mode) {
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: maybe rename to DisplayModeToString() to make it a bit more clear that
> this
> > identifier is a function rather than a class? ditto for
> > DisplaySnapshotDescription -> DisplaySnapshotToString.
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator_unittest.cc:155: : log_(log),
> configure_touchscreens_(false) {}
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: one member per line
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator_unittest.cc:170: if
> (configure_touchscreens_)
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: add curly brackets to this outer if statement
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator_unittest.cc:384: static const
> ui::DisplayMode kBigMode;
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > not sure how much this matters in tests, but we usually avoid non-POD static
> > members. these could just be made non-static and initialized in the c'tor, i
> > think.
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/chromeos/display/output_conf...
> chromeos/display/output_configurator_unittest.cc:463:
> GetFramebufferAction(kSmallMode.size(), &outputs_[0], NULL).c_str(),
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > it is cool to see these lines get unwrapped :-)
> 
> :) Yes, looks much nicer.
> 
>
https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/display_...
> File ui/display/chromeos/display_mode.h (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/display_...
> ui/display/chromeos/display_mode.h:14: class DISPLAY_EXPORT DisplayMode {
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: add a short comment describing what this corresponds to (and maybe
> > mentioning that there are implementation-specific derived classes)
> 
> Done.
> 
>
https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/x11/disp...
> File ui/display/chromeos/x11/display_mode_x11.cc (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/ui/display/chromeos/x11/disp...
> ui/display/chromeos/x11/display_mode_x11.cc:13: : DisplayMode(size,
interlaced,
> refresh_rate), mode_id_(mode_id) {}
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > nit: one member per line
> 
> Done.
> 
> https://codereview.chromium.org/187073002/diff/1/ui/display/display_util.h
> File ui/display/display_util.h (right):
> 
>
https://codereview.chromium.org/187073002/diff/1/ui/display/display_util.h#ne...
> ui/display/display_util.h:16: OutputType GetOutputTypeFromName(const
> std::string& name);
> On 2014/03/05 01:56:12, Daniel Erat wrote:
> > should this be chrome os (or X11)-specific?
> 
> I moved it under ui/display/chromeos for now. If we needed it for other builds
> we can move it back to ui/display. Luckily this will be useful for Ozone as
> well, so not X11 specific.

[dnicoara, 2014-03-05 20:13:33]

On 2014/03/05 20:00:40, oshima wrote:
> I haven't looked into code yet (pretty busy memory sheriff rotation + 34/P1
> bugs) but
> I'd like to avoid having ui dependency in chromeos if possible, and that's one
> of reason why we're moving
> code to ui/display?
> 
> Or do we want to keep some of the code under chrmoeos/ ?
> 

I was thinking that we could leave OutputConfigurator since it is chromeos
specific logic. Otherwise I might as well move everything to ui/display now and
finish the remaining refactoring bits there.

[Daniel Erat, 2014-03-06 00:55:03]

lgtm

https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
File ui/display/display.gyp (right):

https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
ui/display/display.gyp:66: 'target_name': 'display_unittests',
On 2014/03/05 19:52:46, dnicoara wrote:
> I'm wondering if this needs to be added to a list or target to be picked up by
> try-bots?

i don't know anything about this, but maybe build/all.gyp?

[sadrul, 2014-03-06 01:00:30]

https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
File ui/display/display.gyp (right):

https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
ui/display/display.gyp:66: 'target_name': 'display_unittests',
On 2014/03/06 00:55:04, Daniel Erat wrote:
> On 2014/03/05 19:52:46, dnicoara wrote:
> > I'm wondering if this needs to be added to a list or target to be picked up
by
> > try-bots?
> 
> i don't know anything about this, but maybe build/all.gyp?

build/all.gyp can be useful to make sure that this target gets built if you add
this as a dependency to aura_builder/chrome_builder_tests targets.

To actually run the tests on the bots, you need something like this:
https://codereview.chromium.org/66343003/ (this may have changed recently, I am
not sure).

[oshima, 2014-03-06 01:17:22]

On 2014/03/06 01:00:30, sadrul wrote:
> https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
> File ui/display/display.gyp (right):
> 
>
https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
> ui/display/display.gyp:66: 'target_name': 'display_unittests',
> On 2014/03/06 00:55:04, Daniel Erat wrote:
> > On 2014/03/05 19:52:46, dnicoara wrote:
> > > I'm wondering if this needs to be added to a list or target to be picked
up
> by
> > > try-bots?
> > 
> > i don't know anything about this, but maybe build/all.gyp?
> 
> build/all.gyp can be useful to make sure that this target gets built if you
add
> this as a dependency to aura_builder/chrome_builder_tests targets.
> 
> To actually run the tests on the bots, you need something like this:
> https://codereview.chromium.org/66343003/ (this may have changed recently, I
am
> not sure).

Adding new test means a lot more than just adding exeutable (CQ, waterfall,
memory bots etc)
I'd recommend to add them to ui_unittests (like gfx) unless there is strong
reason not to do so.

[oshima, 2014-03-06 01:19:06]

On 2014/03/05 20:13:33, dnicoara wrote:
> On 2014/03/05 20:00:40, oshima wrote:
> > I haven't looked into code yet (pretty busy memory sheriff rotation + 34/P1
> > bugs) but
> > I'd like to avoid having ui dependency in chromeos if possible, and that's
one
> > of reason why we're moving
> > code to ui/display?
> > 
> > Or do we want to keep some of the code under chrmoeos/ ?
> > 
> 
> I was thinking that we could leave OutputConfigurator since it is chromeos
> specific logic. Otherwise I might as well move everything to ui/display now
and
> finish the remaining refactoring bits there.

I assume ozone impl will be under ui/display/ozone (not ozone/display), so I'm
ok to move them to ui/display/chromeos. 
dan, what is your opinion?

[Daniel Erat, 2014-03-06 01:20:38]

On 2014/03/06 01:17:22, oshima wrote:
> On 2014/03/06 01:00:30, sadrul wrote:
> > https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
> > File ui/display/display.gyp (right):
> > 
> >
>
https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
> > ui/display/display.gyp:66: 'target_name': 'display_unittests',
> > On 2014/03/06 00:55:04, Daniel Erat wrote:
> > > On 2014/03/05 19:52:46, dnicoara wrote:
> > > > I'm wondering if this needs to be added to a list or target to be picked
> up
> > by
> > > > try-bots?
> > > 
> > > i don't know anything about this, but maybe build/all.gyp?
> > 
> > build/all.gyp can be useful to make sure that this target gets built if you
> add
> > this as a dependency to aura_builder/chrome_builder_tests targets.
> > 
> > To actually run the tests on the bots, you need something like this:
> > https://codereview.chromium.org/66343003/ (this may have changed recently, I
> am
> > not sure).
> 
> Adding new test means a lot more than just adding exeutable (CQ, waterfall,
> memory bots etc)
> I'd recommend to add them to ui_unittests (like gfx) unless there is strong
> reason not to do so.

i was wondering about this. i looked at ui/gfx/gfx.gyp as an example and saw the
following comment above gfx_geometry_unittests (which is currently built as a
static_library):

    # TODO(beng): This should either generate its own executable or be part of
    #             a gfx_unittests executable. Currently it's built as part of
    #             ui_unittests.

i'm not sure of the reasoning behind this, though. (there does appear to be a
gfx_unittests executable target that builds only range/range_unittest.cc, for
what it's worth)

[Daniel Erat, 2014-03-06 01:23:53]

On 2014/03/06 01:20:38, Daniel Erat wrote:
> On 2014/03/06 01:17:22, oshima wrote:
> > On 2014/03/06 01:00:30, sadrul wrote:
> > >
https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
> > > File ui/display/display.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
> > > ui/display/display.gyp:66: 'target_name': 'display_unittests',
> > > On 2014/03/06 00:55:04, Daniel Erat wrote:
> > > > On 2014/03/05 19:52:46, dnicoara wrote:
> > > > > I'm wondering if this needs to be added to a list or target to be
picked
> > up
> > > by
> > > > > try-bots?
> > > > 
> > > > i don't know anything about this, but maybe build/all.gyp?
> > > 
> > > build/all.gyp can be useful to make sure that this target gets built if
you
> > add
> > > this as a dependency to aura_builder/chrome_builder_tests targets.
> > > 
> > > To actually run the tests on the bots, you need something like this:
> > > https://codereview.chromium.org/66343003/ (this may have changed recently,
I
> > am
> > > not sure).
> > 
> > Adding new test means a lot more than just adding exeutable (CQ, waterfall,
> > memory bots etc)
> > I'd recommend to add them to ui_unittests (like gfx) unless there is strong
> > reason not to do so.
> 
> i was wondering about this. i looked at ui/gfx/gfx.gyp as an example and saw
the
> following comment above gfx_geometry_unittests (which is currently built as a
> static_library):
> 
>     # TODO(beng): This should either generate its own executable or be part of
>     #             a gfx_unittests executable. Currently it's built as part of
>     #             ui_unittests.
> 
> i'm not sure of the reasoning behind this, though. (there does appear to be a
> gfx_unittests executable target that builds only range/range_unittest.cc, for
> what it's worth)

(i'm fine with ui/display/chromeos)

[oshima, 2014-03-06 01:26:11]

On 2014/03/06 01:20:38, Daniel Erat wrote:
> On 2014/03/06 01:17:22, oshima wrote:
> > On 2014/03/06 01:00:30, sadrul wrote:
> > >
https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp
> > > File ui/display/display.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/187073002/diff/50001/ui/display/display.gyp#n...
> > > ui/display/display.gyp:66: 'target_name': 'display_unittests',
> > > On 2014/03/06 00:55:04, Daniel Erat wrote:
> > > > On 2014/03/05 19:52:46, dnicoara wrote:
> > > > > I'm wondering if this needs to be added to a list or target to be
picked
> > up
> > > by
> > > > > try-bots?
> > > > 
> > > > i don't know anything about this, but maybe build/all.gyp?
> > > 
> > > build/all.gyp can be useful to make sure that this target gets built if
you
> > add
> > > this as a dependency to aura_builder/chrome_builder_tests targets.
> > > 
> > > To actually run the tests on the bots, you need something like this:
> > > https://codereview.chromium.org/66343003/ (this may have changed recently,
I
> > am
> > > not sure).
> > 
> > Adding new test means a lot more than just adding exeutable (CQ, waterfall,
> > memory bots etc)
> > I'd recommend to add them to ui_unittests (like gfx) unless there is strong
> > reason not to do so.
> 
> i was wondering about this. i looked at ui/gfx/gfx.gyp as an example and saw
the
> following comment above gfx_geometry_unittests (which is currently built as a
> static_library):
> 
>     # TODO(beng): This should either generate its own executable or be part of
>     #             a gfx_unittests executable. Currently it's built as part of
>     #             ui_unittests.
> 
> i'm not sure of the reasoning behind this, though. (there does appear to be a
> gfx_unittests executable target that builds only range/range_unittest.cc, for
> what it's worth)

Yep, it's ideal to have separate executable, but requires a lot more work, and
that's why gfx is still a part of ui_unittests.

I have no objection if we're going to fix all bot related issues.

[dnicoara, 2014-03-07 16:38:50]

I'll try to answer all the pending questions. Hopefully I don't miss any.

1) Where will the Ozone impl go?

The plan is to put it in ui/display/chromeos/ozone.

2) Running the display_unittests.

For simplicity, I've added the display_unittests to the ui_unittests in patchset
8. Note, I've done it differently from gfx_geometry_unittests since I discovered
that the linker doesn't include them in the final binary, so they aren't being
executed. Will talk to ben@ to fix that issue separately.

3) Moving OutputConfigurator to ui/display/chromeos.

OK, I'll send out a CL to extract TouchscreenDelegate and move it somewhere in
ui/ (not sure on the specific location). After that's checked-in, I'll update
this CL.

4) Misc changes.

Patchset 5 & 7 are small changes that fix functionality after some manual
testing.

[dnicoara, 2014-03-07 16:46:06]

Actually, before making any changes, does it make sense to keep the touchscreen
delegate under ui/display or should it be part of some different ui/ component?

[Daniel Erat, 2014-03-07 22:02:07]

On 2014/03/07 16:46:06, dnicoara wrote:
> Actually, before making any changes, does it make sense to keep the
touchscreen
> delegate under ui/display or should it be part of some different ui/
component?

do you mean chromeos/display (where it appears to be currently), or are you
proposing moving it to ui/display? i don't have strong opinions about this, but
the current location seems fine to me.

[dnicoara, 2014-03-07 22:08:40]

On 2014/03/07 22:02:07, Daniel Erat wrote:
> On 2014/03/07 16:46:06, dnicoara wrote:
> > Actually, before making any changes, does it make sense to keep the
> touchscreen
> > delegate under ui/display or should it be part of some different ui/
> component?
> 
> do you mean chromeos/display (where it appears to be currently), or are you
> proposing moving it to ui/display? i don't have strong opinions about this,
but
> the current location seems fine to me.

Perhaps I misunderstood oshima@'s recommendation. I thought he prefers moving
OutputConfigurator to ui/display/chromeos. If I'm moving OutputConfigurator I'd
like to also move TouchscreenDelegate as well since keeping it under
chromeos/display wouldn't change the need for ui/gfx in DEPS. I would also like
to move it since the plan is to provide an Ozone version if it.

If I'm not moving OutputConfigurator then there isn't a rush on moving
TouchscreenDelegate, so I can defer that to after this CL lands.

[oshima, 2014-03-07 23:34:15]

yes, I meant moving things in chromeos/display to ui/display/chromeos




On Fri, Mar 7, 2014 at 2:08 PM, <dnicoara@chromium.org> wrote:

> On 2014/03/07 22:02:07, Daniel Erat wrote:
>
>> On 2014/03/07 16:46:06, dnicoara wrote:
>> > Actually, before making any changes, does it make sense to keep the
>> touchscreen
>> > delegate under ui/display or should it be part of some different ui/
>> component?
>>
>
>  do you mean chromeos/display (where it appears to be currently), or are
>> you
>> proposing moving it to ui/display? i don't have strong opinions about
>> this,
>>
> but
>
>> the current location seems fine to me.
>>
>
> Perhaps I misunderstood oshima@'s recommendation. I thought he prefers
> moving
> OutputConfigurator to ui/display/chromeos. If I'm moving
> OutputConfigurator I'd
> like to also move TouchscreenDelegate as well since keeping it under
> chromeos/display wouldn't change the need for ui/gfx in DEPS. I would also
> like
> to move it since the plan is to provide an Ozone version if it.
>
> If I'm not moving OutputConfigurator then there isn't a rush on moving
> TouchscreenDelegate, so I can defer that to after this CL lands.
>
> https://codereview.chromium.org/187073002/
>

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

[dnicoara, 2014-03-12 18:16:47]

I've rebased this CL on top of the renaming CL
(https://codereview.chromium.org/192483007/).

[dnicoara, 2014-03-13 18:30:31]

+CC marcheu@chromium.org

[dnicoara, 2014-03-18 13:42:03]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-03-18 13:42:25]

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

[commit-bot: I haz the power, 2014-03-18 18:45:32]

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

[commit-bot: I haz the power, 2014-03-18 18:45:33]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[dnicoara, 2014-03-18 18:46:30]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-03-18 18:52:12]

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

[commit-bot: I haz the power, 2014-03-18 21:16:07]

Change committed as 257757