Add "panel_fitting" GPU feature type and use it for mirror mode.

Add "panel_fitting" GPU feature type and use it for mirror mode.

Panel fitting is used by mirror display mode on Chrome OS.
This GPU feature type will tell if it is supported by GPU hardware.
Initially enable it only for Ivy Bridge GPU via blacklisting everything else.
Also add a disable_panel_fitting command flag,
to be able to quickly turn the feature off.

When hardware supports panel fitting,
and there are no common modes between the internal and the external displays,
configure the internal display to the external's resolution,
and let the panel fitter take care of displaying it correctly.

BUG=chromium-os:34442
TEST=Panel Fitting should appear as Hardware Accelerated in about:gpu
     on IVB Chrome OS, Unavailable everywhere else.
TEST=Tested mirror mode on device.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161980

[ynovikov, 2012-09-17 16:13:59]

Hi, please review this fix for mirror mode we want to land for M23.
Zhenyao, could you review the parts related to GPU feature type - everything
except output_configurator?
Oshima, could you review the rest - output configurator and shell?
Thank you.

[cwolfe, 2012-09-18 13:27:47]

Various thoughts. I believe they're almost entirely nits from my perspective,
but the actual residents of the code may have stronger opinions :)

http://codereview.chromium.org/10909242/diff/2001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10909242/diff/2001/ash/shell.cc#newcode199
ash/shell.cc:199:
(content::GpuDataManager::GetInstance()->GetBlacklistedFeatures() &
I'd find this clearer if (as elsewhere in the code) the blacklisted features
were stashed in a variable first (see CodeSearch for GetBlacklistedFeatures).

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
File chrome/browser/resources/software_rendering_list.json (right):

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
chrome/browser/resources/software_rendering_list.json:673: "description":
"Enable panel fitting capability on ChromeOS only on Ivy Bridge Graphics
Controller.",
Suggestion: "Use panel fitting for Chrome OS mirror mode. Initially only enabled
on IVB."

technically IVB is a processor family, which just happens to have the integrated
GPU we're expecting :)

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:137: RROutput out_one,
Would prefer output_* for these names. I initially interpreted them as output
parameters, which only two of them actually are.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:148: bool one_is_internal =
OutputConfigurator::IsInternalOutputName(
Make this a member of OutputConfigurator to shorten that? (or alternatively,
does the utility function need to be a member?)

Could also shorten it to IsInternalOutput(output_one_info)

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:156: bool there_is_one_internal_output =
true;
found_internal_output with a comment that it will be set if there is exactly one
internal output?

Also, you could check internal_info != NULL to avoid this flag entirely.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:158: // Make sure exactly one of the
outputs is internal
Since it would simplify this, I suggest we ignore the two-internal-outputs case
and just prefer the first output.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:188: *out_one_mode = *out_two_mode =
external_ideal_mode_id;
Merge those parameters?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:289: bool is_panel_fitting_supported) {
Would it be clearer to make these functions members of OutputConfigurator to
avoid the extra parameter?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:357: can_add_mirror_mode =
added_mirror_mode?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:674: GetDualOutputs(display, screen,
&outputs[0], &outputs[1],
Yeah, I'd prefer making GetDualOutputs a non-static member of
OutputConfigurator.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:918: RRMode
OutputConfigurator::GetOutputNativeMode(
Seems like this should be "GetOutputPreferredMode"

Unsure whether we need to check |output_info->npreferred| as well. If that is
regularly non-one, we might want to actually look for the highest-resolution
preferred mode.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:922: if (output_info->nmode > 0) {
This is one of the places I actually like early returns. How about:

if (output_info->nmode == 0)
  return None;

return output_info->modes[0];

I assume there is actually a case where nmode *can* be zero?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.h:110: bool is_panel_fitting_supported_;
Weak preference for something like use_panel_fitting_ (since it can be
supported, but still disabled)

[oshima, 2012-09-18 14:31:11]

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:158: // Make sure exactly one of the
outputs is internal
On 2012/09/18 13:27:47, cwolfe wrote:
> Since it would simplify this, I suggest we ignore the two-internal-outputs
case
> and just prefer the first output.
 
with some log message in case it indeed detects two internal displays
(due to a bug), and probably DCHECK.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:922: if (output_info->nmode > 0) {
On 2012/09/18 13:27:47, cwolfe wrote:
> This is one of the places I actually like early returns. How about:
> 
> if (output_info->nmode == 0)
>   return None;
> 
> return output_info->modes[0];
> 
> I assume there is actually a case where nmode *can* be zero?
+1 This matches chrome preferred style.

[ynovikov, 2012-09-18 15:16:21]

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
File chrome/browser/resources/software_rendering_list.json (right):

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
chrome/browser/resources/software_rendering_list.json:673: "description":
"Enable panel fitting capability on ChromeOS only on Ivy Bridge Graphics
Controller.",
Actually, panel fitting GPU feature is not tightly coupled with mirror mode.
Right now mirror mode is the only usage, but I can think of other usages, like
magnification. So, I wouldn't want to mention mirror mode here.
The origin of "Ivy Bridge Graphics Controller" is the name I got for the device
id 0x0166 in lspci, and I believe it also appears in about:gpu.
So, I believe my original comment is a better description of my intentions.

On 2012/09/18 13:27:47, cwolfe wrote:
> Suggestion: "Use panel fitting for Chrome OS mirror mode. Initially only
enabled
> on IVB."
> 
> technically IVB is a processor family, which just happens to have the
integrated
> GPU we're expecting :)

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:148: bool one_is_internal =
OutputConfigurator::IsInternalOutputName(
I like the idea of IsInternalOutput(output_one_info), Oshima, what do you think?

On 2012/09/18 13:27:47, cwolfe wrote:
> Make this a member of OutputConfigurator to shorten that? (or alternatively,
> does the utility function need to be a member?)
> 
> Could also shorten it to IsInternalOutput(output_one_info)

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:156: bool there_is_one_internal_output =
true;
I think your suggestion will make the code more obscure, I'd like to describe
clearly what I am trying to do.

On 2012/09/18 13:27:47, cwolfe wrote:
> found_internal_output with a comment that it will be set if there is exactly
one
> internal output?
> 
> Also, you could check internal_info != NULL to avoid this flag entirely.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:158: // Make sure exactly one of the
outputs is internal
I agree with Oshima that two-internal-outputs case should not be ignored, so it
is not possible to simplify the code - both internal_info and external_info
should be set to correct values.

But, I can think of a case where two internal displays is not a bug - I remember
some cell phones having a display on both sides of the lid, and I think I saw a
computer which also had something like this, behaving as a tablet with closed
lid and as a notebook with opened lid.

So, I'd treat the "else" case as "we didn't decide yet what the correct
behaviour is in this case". Oshima, what is the appropriate thing to do for
this?

On 2012/09/18 14:31:11, oshima wrote:
> On 2012/09/18 13:27:47, cwolfe wrote:
> > Since it would simplify this, I suggest we ignore the two-internal-outputs
> case
> > and just prefer the first output.
>  
> with some log message in case it indeed detects two internal displays
> (due to a bug), and probably DCHECK.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:188: *out_one_mode = *out_two_mode =
external_ideal_mode_id;
Their identity is implementation dependent, if in the future we decide that we
should add a new mode which has the same resolution but a different refresh
rate, different values will be returned in these two parameters.

On 2012/09/18 13:27:47, cwolfe wrote:
> Merge those parameters?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:289: bool is_panel_fitting_supported) {
Perhaps it would, I am not sure why these functions were not members of
OutputConfigurator in the first place (maybe to hide them from the .h file?).
Oshima, what do you think?

On 2012/09/18 13:27:47, cwolfe wrote:
> Would it be clearer to make these functions members of OutputConfigurator to
> avoid the extra parameter?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:357: can_add_mirror_mode =
Sounds good to me, how about mirror_mode_found for the first one and
mirror_mode_added for the second one?

On 2012/09/18 13:27:47, cwolfe wrote:
> added_mirror_mode?

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:918: RRMode
OutputConfigurator::GetOutputNativeMode(
No, what this function should return is the Native mode, not a Preferred mode.
And from what we've seen in Xrandr specification, it is possible that there are
several preferred modes.
This code is just a refactoring, the heuristic to use modes[0] as the native
mode worked before.
If we do find out cases where it is wrong, we can have a more complex algorithm,
like if npreferred is valid, search for highest resolution in modes up to
npreferred, otherwise search for highest resolution in all modes.

On 2012/09/18 13:27:47, cwolfe wrote:
> Seems like this should be "GetOutputPreferredMode"
> 
> Unsure whether we need to check |output_info->npreferred| as well. If that is
> regularly non-one, we might want to actually look for the highest-resolution
> preferred mode.

[Zhenyao Mo, 2012-09-18 19:28:55]

LGTM with blacklist and about:gpu related code

http://codereview.chromium.org/10909242/diff/2001/content/browser/gpu/gpu_uti...
File content/browser/gpu/gpu_util.cc (right):

http://codereview.chromium.org/10909242/diff/2001/content/browser/gpu/gpu_uti...
content/browser/gpu/gpu_util.cc:134:
matches.push_back(kGpuFeatureNamePanelFitting);
Please also update the unit test.

[cwolfe, 2012-09-18 20:55:50]

Believe we're just waiting for oshima's opinion on a few things, all of which
feel pretty minor.

LGTM with the pending tweaks.

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
File chrome/browser/resources/software_rendering_list.json (right):

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
chrome/browser/resources/software_rendering_list.json:673: "description":
"Enable panel fitting capability on ChromeOS only on Ivy Bridge Graphics
Controller.",
Sounds good.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:156: bool there_is_one_internal_output =
true;
Fair enough. I was first going to suggest a bool flag of found_internal_output,
which made internal_info != NULL pretty obvious.

On 2012/09/18 15:16:21, ynovikov wrote:
> I think your suggestion will make the code more obscure, I'd like to describe
> clearly what I am trying to do.
> 
> On 2012/09/18 13:27:47, cwolfe wrote:
> > found_internal_output with a comment that it will be set if there is exactly
> one
> > internal output?
> > 
> > Also, you could check internal_info != NULL to avoid this flag entirely.
>

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:158: // Make sure exactly one of the
outputs is internal
To (possibly) clarify, I was leaning toward "if there are two internal inputs,
use the first one" as that simplifies the logic to:

if (one_is_internal) { ... }
else if (two_is_internal) { ... }
else { /* zero internal */ }

If we genuinely have a device with two internal displays, we're probably going
to be back here adding support for *three* displays. :)

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:188: *out_one_mode = *out_two_mode =
external_ideal_mode_id;
Fair enough.

On 2012/09/18 15:16:21, ynovikov wrote:
> Their identity is implementation dependent, if in the future we decide that we
> should add a new mode which has the same resolution but a different refresh
> rate, different values will be returned in these two parameters.
> 
> On 2012/09/18 13:27:47, cwolfe wrote:
> > Merge those parameters?
>

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:357: can_add_mirror_mode =
SGTM

On 2012/09/18 15:16:21, ynovikov wrote:
> Sounds good to me, how about mirror_mode_found for the first one and
> mirror_mode_added for the second one?
> 
> On 2012/09/18 13:27:47, cwolfe wrote:
> > added_mirror_mode?
>

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:918: RRMode
OutputConfigurator::GetOutputNativeMode(
I dislike the implication that we know anything about the native mode, but don't
feel that strongly. What this returns is a single mode that we figure the user
will prefer.

Your call.

On 2012/09/18 15:16:21, ynovikov wrote:
> No, what this function should return is the Native mode, not a Preferred mode.
> And from what we've seen in Xrandr specification, it is possible that there
are
> several preferred modes.
> This code is just a refactoring, the heuristic to use modes[0] as the native
> mode worked before.
> If we do find out cases where it is wrong, we can have a more complex
algorithm,
> like if npreferred is valid, search for highest resolution in modes up to
> npreferred, otherwise search for highest resolution in all modes.
> 
> On 2012/09/18 13:27:47, cwolfe wrote:
> > Seems like this should be "GetOutputPreferredMode"
> > 
> > Unsure whether we need to check |output_info->npreferred| as well. If that
is
> > regularly non-one, we might want to actually look for the highest-resolution
> > preferred mode.
>

[oshima, 2012-09-18 21:11:31]

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:148: bool one_is_internal =
OutputConfigurator::IsInternalOutputName(
On 2012/09/18 15:16:21, ynovikov wrote:
> I like the idea of IsInternalOutput(output_one_info), Oshima, what do you
think?


Fine with me.

> 
> On 2012/09/18 13:27:47, cwolfe wrote:
> > Make this a member of OutputConfigurator to shorten that? (or alternatively,
> > does the utility function need to be a member?)
> > 
> > Could also shorten it to IsInternalOutput(output_one_info)
>

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:158: // Make sure exactly one of the
outputs is internal
On 2012/09/18 20:55:50, cwolfe wrote:
> To (possibly) clarify, I was leaning toward "if there are two internal inputs,
> use the first one" as that simplifies the logic to:
> 
> if (one_is_internal) { ... }
> else if (two_is_internal) { ... }
> else { /* zero internal */ }
> 
> If we genuinely have a device with two internal displays, we're probably going
> to be back here adding support for *three* displays. :)

I don't think we have to support two internal displays case (so agree with
@cwolfe),
but I'd also like to know it if it ever happens, mainly for
troubleshooting/debugging purpose.

Please remember that the person who'll work on new feature or troubleshoot this
may or may
not be the same person, so extra information can be helpful.

[ynovikov, 2012-09-21 18:48:27]

I believe I have addressed all the review comments, please check out the
results.
I've also moved the disable switch from content into ash, since it gave a link
warning. Oshima, can you review that as well?

http://codereview.chromium.org/10909242/diff/2001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10909242/diff/2001/ash/shell.cc#newcode199
ash/shell.cc:199:
(content::GpuDataManager::GetInstance()->GetBlacklistedFeatures() &
On 2012/09/18 13:27:47, cwolfe wrote:
> I'd find this clearer if (as elsewhere in the code) the blacklisted features
> were stashed in a variable first (see CodeSearch for GetBlacklistedFeatures).

Done.

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
File chrome/browser/resources/software_rendering_list.json (right):

http://codereview.chromium.org/10909242/diff/2001/chrome/browser/resources/so...
chrome/browser/resources/software_rendering_list.json:673: "description":
"Enable panel fitting capability on ChromeOS only on Ivy Bridge Graphics
Controller.",
On 2012/09/18 20:55:50, cwolfe wrote:
> Sounds good.

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:137: RROutput out_one,
On 2012/09/18 13:27:47, cwolfe wrote:
> Would prefer output_* for these names. I initially interpreted them as output
> parameters, which only two of them actually are.

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:148: bool one_is_internal =
OutputConfigurator::IsInternalOutputName(
On 2012/09/18 21:11:31, oshima wrote:
> On 2012/09/18 15:16:21, ynovikov wrote:
> > I like the idea of IsInternalOutput(output_one_info), Oshima, what do you
> think?
> 
> 
> Fine with me.
> 
> > 
> > On 2012/09/18 13:27:47, cwolfe wrote:
> > > Make this a member of OutputConfigurator to shorten that? (or
alternatively,
> > > does the utility function need to be a member?)
> > > 
> > > Could also shorten it to IsInternalOutput(output_one_info)
> > 
> 

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:156: bool there_is_one_internal_output =
true;
On 2012/09/18 20:55:50, cwolfe wrote:
> Fair enough. I was first going to suggest a bool flag of
found_internal_output,
> which made internal_info != NULL pretty obvious.
> 
> On 2012/09/18 15:16:21, ynovikov wrote:
> > I think your suggestion will make the code more obscure, I'd like to
describe
> > clearly what I am trying to do.
> > 
> > On 2012/09/18 13:27:47, cwolfe wrote:
> > > found_internal_output with a comment that it will be set if there is
exactly
> > one
> > > internal output?
> > > 
> > > Also, you could check internal_info != NULL to avoid this flag entirely.
> > 
> 

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:158: // Make sure exactly one of the
outputs is internal
On 2012/09/18 21:11:31, oshima wrote:
> On 2012/09/18 20:55:50, cwolfe wrote:
> > To (possibly) clarify, I was leaning toward "if there are two internal
inputs,
> > use the first one" as that simplifies the logic to:
> > 
> > if (one_is_internal) { ... }
> > else if (two_is_internal) { ... }
> > else { /* zero internal */ }
> > 
> > If we genuinely have a device with two internal displays, we're probably
going
> > to be back here adding support for *three* displays. :)
> 
> I don't think we have to support two internal displays case (so agree with
> @cwolfe),
> but I'd also like to know it if it ever happens, mainly for
> troubleshooting/debugging purpose.
> 
> Please remember that the person who'll work on new feature or troubleshoot
this
> may or may
> not be the same person, so extra information can be helpful.

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:188: *out_one_mode = *out_two_mode =
external_ideal_mode_id;
On 2012/09/18 20:55:50, cwolfe wrote:
> Fair enough.
> 
> On 2012/09/18 15:16:21, ynovikov wrote:
> > Their identity is implementation dependent, if in the future we decide that
we
> > should add a new mode which has the same resolution but a different refresh
> > rate, different values will be returned in these two parameters.
> > 
> > On 2012/09/18 13:27:47, cwolfe wrote:
> > > Merge those parameters?
> > 
> 

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:289: bool is_panel_fitting_supported) {
On 2012/09/18 15:16:21, ynovikov wrote:
> Perhaps it would, I am not sure why these functions were not members of
> OutputConfigurator in the first place (maybe to hide them from the .h file?).
> Oshima, what do you think?
> 
> On 2012/09/18 13:27:47, cwolfe wrote:
> > Would it be clearer to make these functions members of OutputConfigurator to
> > avoid the extra parameter?
> 

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:357: can_add_mirror_mode =
On 2012/09/18 20:55:50, cwolfe wrote:
> SGTM
> 
> On 2012/09/18 15:16:21, ynovikov wrote:
> > Sounds good to me, how about mirror_mode_found for the first one and
> > mirror_mode_added for the second one?
> > 
> > On 2012/09/18 13:27:47, cwolfe wrote:
> > > added_mirror_mode?
> > 
> 

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:674: GetDualOutputs(display, screen,
&outputs[0], &outputs[1],
On 2012/09/18 13:27:47, cwolfe wrote:
> Yeah, I'd prefer making GetDualOutputs a non-static member of
> OutputConfigurator.

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:918: RRMode
OutputConfigurator::GetOutputNativeMode(
On 2012/09/18 20:55:50, cwolfe wrote:
> I dislike the implication that we know anything about the native mode, but
don't
> feel that strongly. What this returns is a single mode that we figure the user
> will prefer.
> 
> Your call.
> 
> On 2012/09/18 15:16:21, ynovikov wrote:
> > No, what this function should return is the Native mode, not a Preferred
mode.
> > And from what we've seen in Xrandr specification, it is possible that there
> are
> > several preferred modes.
> > This code is just a refactoring, the heuristic to use modes[0] as the native
> > mode worked before.
> > If we do find out cases where it is wrong, we can have a more complex
> algorithm,
> > like if npreferred is valid, search for highest resolution in modes up to
> > npreferred, otherwise search for highest resolution in all modes.
> > 
> > On 2012/09/18 13:27:47, cwolfe wrote:
> > > Seems like this should be "GetOutputPreferredMode"
> > > 
> > > Unsure whether we need to check |output_info->npreferred| as well. If that
> is
> > > regularly non-one, we might want to actually look for the
highest-resolution
> > > preferred mode.
> > 
> 

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.cc:922: if (output_info->nmode > 0) {
On 2012/09/18 14:31:11, oshima wrote:
> On 2012/09/18 13:27:47, cwolfe wrote:
> > This is one of the places I actually like early returns. How about:
> > 
> > if (output_info->nmode == 0)
> >   return None;
> > 
> > return output_info->modes[0];
> > 
> > I assume there is actually a case where nmode *can* be zero?
> +1 This matches chrome preferred style.

Done.

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10909242/diff/2001/chromeos/display/output_con...
chromeos/display/output_configurator.h:110: bool is_panel_fitting_supported_;
On 2012/09/18 13:27:47, cwolfe wrote:
> Weak preference for something like use_panel_fitting_ (since it can be
> supported, but still disabled)

Done.

http://codereview.chromium.org/10909242/diff/2001/content/browser/gpu/gpu_uti...
File content/browser/gpu/gpu_util.cc (right):

http://codereview.chromium.org/10909242/diff/2001/content/browser/gpu/gpu_uti...
content/browser/gpu/gpu_util.cc:134:
matches.push_back(kGpuFeatureNamePanelFitting);
On 2012/09/18 19:28:55, Zhenyao Mo wrote:
> Please also update the unit test.

Done.

[cwolfe, 2012-09-21 19:17:21]

LGTM

(just including a function diff)

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:834: int
OutputConfigurator::GetDualOutputs(Display* display,
Just for my sanity, here's a diff -b between the versions of GetDualOutputs:

|1c1
|< static int GetDualOutputs(Display* display,
|---
|> int OutputConfigurator::GetDualOutputs(Display* display,
|42,43c42
|<       if (output_info->nmode > 0)
|<         to_populate->native_mode = output_info->modes[0];
|---
|>       to_populate->native_mode = GetOutputNativeMode(output_info);
|47,49c46
|<       to_populate->is_internal =
|<           OutputConfigurator::IsInternalOutputName(
|<               std::string(output_info->name));
|---
|>       to_populate->is_internal = IsInternalOutput(output_info);
|63c60
|<     bool can_mirror = FindMirrorModeForOutputs(display,
|---
|>     bool mirror_mode_found = FindMirrorModeForOutputs(display,
|69c66,73
|<     if (!can_mirror) {
|---
|>     if (!mirror_mode_found) {
|>       bool mirror_mode_added = AddMirrorModeToInternalOutput(display,
|>                                                              screen,
|>                                                              one->output,
|>                                                              two->output,
|>                                                             
&one->mirror_mode,
|>                                                             
&two->mirror_mode);
|>       if (!mirror_mode_added) {

[oshima, 2012-09-21 19:38:12]

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:755: }
nuke {}

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:774: }
nuke {}

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:885: ++found_count;
you probably want to increment this before VLOG.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.h:33: typedef struct OutputSnapshot
OutputSnapshot;
you don't need this

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.h:107: static bool IsInternalOutput(const
XRROutputInfo* output_info);
move static after private instance methods
also please keep the order of methods in header and .cc the same.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.h:114: OutputSnapshot* two);
move static methods before (or after) private instance methods.

[Zhenyao Mo, 2012-09-21 20:04:11]

FYI: software_rendering_list.json has been moved to content/browser/gpu

[ynovikov, 2012-09-21 23:05:36]

Addressed Oshima's review and rebased, getting updated
software_rendering_list.json.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:755: }
On 2012/09/21 19:38:12, oshima wrote:
> nuke {}

Done.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:774: }
On 2012/09/21 19:38:12, oshima wrote:
> nuke {}

Done.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:834: int
OutputConfigurator::GetDualOutputs(Display* display,
On 2012/09/21 19:17:21, cwolfe wrote:
> Just for my sanity, here's a diff -b between the versions of GetDualOutputs:
> 
> |1c1
> |< static int GetDualOutputs(Display* display,
> |---
> |> int OutputConfigurator::GetDualOutputs(Display* display,
> |42,43c42
> |<       if (output_info->nmode > 0)
> |<         to_populate->native_mode = output_info->modes[0];
> |---
> |>       to_populate->native_mode = GetOutputNativeMode(output_info);
> |47,49c46
> |<       to_populate->is_internal =
> |<           OutputConfigurator::IsInternalOutputName(
> |<               std::string(output_info->name));
> |---
> |>       to_populate->is_internal = IsInternalOutput(output_info);
> |63c60
> |<     bool can_mirror = FindMirrorModeForOutputs(display,
> |---
> |>     bool mirror_mode_found = FindMirrorModeForOutputs(display,
> |69c66,73
> |<     if (!can_mirror) {
> |---
> |>     if (!mirror_mode_found) {
> |>       bool mirror_mode_added = AddMirrorModeToInternalOutput(display,
> |>                                                              screen,
> |>                                                              one->output,
> |>                                                              two->output,
> |>                                                             
> &one->mirror_mode,
> |>                                                             
> &two->mirror_mode);
> |>       if (!mirror_mode_added) {

Done.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:885: ++found_count;
I didn't change this code, just moved it.
It does zero-based counting now, I see nothing wrong with it.

On 2012/09/21 19:38:12, oshima wrote:
> you probably want to increment this before VLOG.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.h:33: typedef struct OutputSnapshot
OutputSnapshot;
On 2012/09/21 19:38:12, oshima wrote:
> you don't need this

Done.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.h:107: static bool IsInternalOutput(const
XRROutputInfo* output_info);
On 2012/09/21 19:38:12, oshima wrote:
> move static after private instance methods
> also please keep the order of methods in header and .cc the same.

Done.

http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
chromeos/display/output_configurator.h:114: OutputSnapshot* two);
On 2012/09/21 19:38:12, oshima wrote:
> move static methods before (or after) private instance methods.

Done.

[oshima, 2012-09-21 23:14:09]

Can you hold this until Monday? We're punting extended desktop 
to 24 due to driver issue, and are going to discuss exactly what to do
on Monday.

We probably add simple display switching (cycling A->B->mirror), and we
need to land that change fist before this. We still may want to land this,
(but can't tell you how much likely right now), and I'll coordinate with you
next week before landing that change.


On 2012/09/21 23:05:36, ynovikov wrote:
> Addressed Oshima's review and rebased, getting updated
> software_rendering_list.json.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> File chromeos/display/output_configurator.cc (right):
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.cc:755: }
> On 2012/09/21 19:38:12, oshima wrote:
> > nuke {}
> 
> Done.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.cc:774: }
> On 2012/09/21 19:38:12, oshima wrote:
> > nuke {}
> 
> Done.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.cc:834: int
> OutputConfigurator::GetDualOutputs(Display* display,
> On 2012/09/21 19:17:21, cwolfe wrote:
> > Just for my sanity, here's a diff -b between the versions of GetDualOutputs:
> > 
> > |1c1
> > |< static int GetDualOutputs(Display* display,
> > |---
> > |> int OutputConfigurator::GetDualOutputs(Display* display,
> > |42,43c42
> > |<       if (output_info->nmode > 0)
> > |<         to_populate->native_mode = output_info->modes[0];
> > |---
> > |>       to_populate->native_mode = GetOutputNativeMode(output_info);
> > |47,49c46
> > |<       to_populate->is_internal =
> > |<           OutputConfigurator::IsInternalOutputName(
> > |<               std::string(output_info->name));
> > |---
> > |>       to_populate->is_internal = IsInternalOutput(output_info);
> > |63c60
> > |<     bool can_mirror = FindMirrorModeForOutputs(display,
> > |---
> > |>     bool mirror_mode_found = FindMirrorModeForOutputs(display,
> > |69c66,73
> > |<     if (!can_mirror) {
> > |---
> > |>     if (!mirror_mode_found) {
> > |>       bool mirror_mode_added = AddMirrorModeToInternalOutput(display,
> > |>                                                              screen,
> > |>                                                              one->output,
> > |>                                                              two->output,
> > |>                                                             
> > &one->mirror_mode,
> > |>                                                             
> > &two->mirror_mode);
> > |>       if (!mirror_mode_added) {
> 
> Done.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.cc:885: ++found_count;
> I didn't change this code, just moved it.
> It does zero-based counting now, I see nothing wrong with it.
> 
> On 2012/09/21 19:38:12, oshima wrote:
> > you probably want to increment this before VLOG.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> File chromeos/display/output_configurator.h (right):
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.h:33: typedef struct OutputSnapshot
> OutputSnapshot;
> On 2012/09/21 19:38:12, oshima wrote:
> > you don't need this
> 
> Done.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.h:107: static bool IsInternalOutput(const
> XRROutputInfo* output_info);
> On 2012/09/21 19:38:12, oshima wrote:
> > move static after private instance methods
> > also please keep the order of methods in header and .cc the same.
> 
> Done.
> 
>
http://codereview.chromium.org/10909242/diff/12001/chromeos/display/output_co...
> chromeos/display/output_configurator.h:114: OutputSnapshot* two);
> On 2012/09/21 19:38:12, oshima wrote:
> > move static methods before (or after) private instance methods.
> 
> Done.

[ynovikov, 2012-09-21 23:20:27]

No problem, I can hold.
But you probably want to land this very much, since on some platforms there
won't be a mirror mode without this.

[oshima, 2012-10-04 17:04:23]

On 2012/09/21 23:20:27, ynovikov wrote:
> No problem, I can hold.
> But you probably want to land this very much, since on some platforms there
> won't be a mirror mode without this.

lgtm (sorry I somehow thought i've already done)

[commit-bot: I haz the power, 2012-10-09 20:18:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ynovikov@chromium.org/10909242/31001

[commit-bot: I haz the power, 2012-10-09 20:18:30]

Presubmit check for 10909242-31001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui
    ash
    content/public
    chrome/browser/resources/gpu_internals

Presubmit checks took 2.2s to calculate.

[ynovikov, 2012-10-09 20:44:29]

Hi,

I need an OWNERS review.

sky, could you cover chrome/browser/ui/webui and ash?
joi, can you look at content/public?
nduca, can you look at chrome/browser/resources/gpu_internals?

Thanks.

[Jói, 2012-10-09 21:09:46]

LGTM for content/public.

[nduca, 2012-10-09 21:19:38]

LGTM if it needs it.

[sky, 2012-10-09 22:52:42]

ash LGTM. I'm not a good reviewer for webui though. I added James.

[James Hawkins, 2012-10-12 21:18:45]

webui lgtm

[commit-bot: I haz the power, 2012-10-15 19:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ynovikov@chromium.org/10909242/37003

[commit-bot: I haz the power, 2012-10-15 22:19:34]

Change committed as 161980