Introduce user customization of external HighDPI mode for 4K monitor.

Introduce user customization of external HighDPI mode for 4K monitor.

This CL adds the options of customizing external display's device
scale factor from the options page. There are two TODOs though:
- the configured device scale factor should be stored
- not possible to specify device scale factor and resolution at the
  same time; from 1600x900, it can't enter to 1920x1080(2x). Instead
  it enters to 3840x2160, then the user needs to select 2x again.

Actually the former would solve the latter; this CL sets the new
resolution and the DSF at the same time but resolution change is
asynchronous, so changed DSF will be overwritten by the resolution
change.

I will address these issues in further CL(s).

BUG=396704
R=oshima@chromium.org, stevenjb@chromium.org
TBR=asvitkine@chromium.org
TEST=manually

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

[Jun Mukai, 2014-07-28 18:38:49]

ping?

[oshima, 2014-07-28 21:37:35]

discussed offline:

We agreed that it'd be better for DisplayManager to return the list of
DisplayInfo that it can accept, instead of having the
settings page to construct the list.

[stevenjb, 2014-07-28 21:44:37]

https://codereview.chromium.org/417113012/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/417113012/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/display_options.js:182: }
I'm not sure if this is what oshima meant, but it would be nice if we used a
single chrome method here and passed an object/dictionary to it so that the
controller logic can be moved there.

[Jun Mukai, 2014-07-28 21:51:00]

https://codereview.chromium.org/417113012/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/417113012/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/display_options.js:182: }
On 2014/07/28 21:44:37, stevenjb wrote:
> I'm not sure if this is what oshima meant, but it would be nice if we used a
> single chrome method here and passed an object/dictionary to it so that the
> controller logic can be moved there.

That is same as my understanding.

[Jun Mukai, 2014-07-30 01:17:23]

I think I've done the refactoring.  PTAL.

[oshima, 2014-07-30 02:09:45]

Just looked at first a few files. I'll review the rest later today.

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
File ash/display/resolution_notification_controller.cc (right):

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
ash/display/resolution_notification_controller.cc:115: ash::DisplayMode
old_resolution;
remove ash::

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
ash/display/resolution_notification_controller.h:47: const ash::DisplayMode&
new_resolution,
remove ash::

[Jun Mukai, 2014-07-30 02:18:31]

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
File ash/display/resolution_notification_controller.cc (right):

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
ash/display/resolution_notification_controller.cc:115: ash::DisplayMode
old_resolution;
On 2014/07/30 02:09:45, oshima wrote:
> remove ash::

Done.

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/417113012/diff/40001/ash/display/resolution_n...
ash/display/resolution_notification_controller.h:47: const ash::DisplayMode&
new_resolution,
On 2014/07/30 02:09:45, oshima wrote:
> remove ash::

Done.

[stevenjb, 2014-07-30 17:49:59]

I definitely like this better. A couple of suggestions.

https://codereview.chromium.org/417113012/diff/60001/ash/display/display_chan...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/417113012/diff/60001/ash/display/display_chan...
ash/display/display_change_observer_chromeos.cc:143: if
(native_mode.size.width() >= 3840.0f) {
nit: define 3840.0f as a constant at the top of the file

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:83: return
double_result;
nit: explicit static_cast<int>

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:95: *result
= double_result;
nit: explicit static_cast<float>

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:284: }
Does it make sense to move this logic to DisplayMode or DisplayInfo, e.g.
resolution->SetBoolean("selected", DisplayInfo.IsSelected(mode, display))?

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:409: }
Similarly, can this logic be moved into DisplayManager?

https://codereview.chromium.org/417113012/diff/60001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/417113012/diff/60001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8358: </action>
I don't think we actually need to rename this, just update the comment.

[Jun Mukai, 2014-07-30 23:50:51]

https://codereview.chromium.org/417113012/diff/60001/ash/display/display_chan...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/417113012/diff/60001/ash/display/display_chan...
ash/display/display_change_observer_chromeos.cc:143: if
(native_mode.size.width() >= 3840.0f) {
On 2014/07/30 17:49:59, stevenjb wrote:
> nit: define 3840.0f as a constant at the top of the file

Done.

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:83: return
double_result;
On 2014/07/30 17:49:59, stevenjb wrote:
> nit: explicit static_cast<int>

Done.

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:95: *result
= double_result;
On 2014/07/30 17:49:59, stevenjb wrote:
> nit: explicit static_cast<float>

Done.

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:284: }
On 2014/07/30 17:49:59, stevenjb wrote:
> Does it make sense to move this logic to DisplayMode or DisplayInfo, e.g.
> resolution->SetBoolean("selected", DisplayInfo.IsSelected(mode, display))?
> 
> 

Done.

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:409: }
On 2014/07/30 17:49:59, stevenjb wrote:
> Similarly, can this logic be moved into DisplayManager?

mostly done, calling resolution_notification_controller should be accessed from
here, because its callback should be formed here (and I don't want to put the
callback to SetDisplayMode).
Instead, moved the logic whether it should create resolution notification or
not, which simplifies here.  WDYT?

https://codereview.chromium.org/417113012/diff/60001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/417113012/diff/60001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8358: </action>
On 2014/07/30 17:49:59, stevenjb wrote:
> I don't think we actually need to rename this, just update the comment.

Done.

[stevenjb, 2014-07-31 00:03:58]

lgtm

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/417113012/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:409: }
On 2014/07/30 23:50:51, Jun Mukai wrote:
> On 2014/07/30 17:49:59, stevenjb wrote:
> > Similarly, can this logic be moved into DisplayManager?
> 
> mostly done, calling resolution_notification_controller should be accessed
from
> here, because its callback should be formed here (and I don't want to put the
> callback to SetDisplayMode).
> Instead, moved the logic whether it should create resolution notification or
> not, which simplifies here.  WDYT?
Looks great, cheers.

https://codereview.chromium.org/417113012/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/417113012/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:125: int64
display_id, const ash::DisplayMode& mode) {
nit: one arg per line

[Jun Mukai, 2014-07-31 00:31:35]

thanks. Then, I'd like to get reviewed by oshima...

https://codereview.chromium.org/417113012/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/417113012/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:125: int64
display_id, const ash::DisplayMode& mode) {
On 2014/07/31 00:03:58, stevenjb wrote:
> nit: one arg per line

Done.

[oshima, 2014-07-31 02:16:52]

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:87: true);
why you're iterating the most list? Can't you just use
output.display->native_mode() ?

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.h:30: ASH_EXPORT static
std::vector<DisplayMode> GetDisplayModeList(
GetExternalModeList ?

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_inf...
ash/display/display_info.h:30: bool operator==(const DisplayMode& other) const;
can you add comment that this is necessary for stl compatibility ? (style guide
still recommends not to use operator overloading in general)

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_inf...
ash/display/display_info.h:37: // TODO(mukai|oshima): merge to
device_scale_factor.
should we?

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.cc:545: 
can you add comment that this is looking for "default" mode?

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.cc:551: if (info.configured_ui_scale() ==
display_modes[i].ui_scale)
I believe the ui scale should always be 1.0f in this case? If so, can you add
DCHECK? (in outs of the loop)

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.h:169: // TODO(mukai): remove this and merge into
SetDisplayMode.
or you can just move them to private

[Jun Mukai, 2014-07-31 04:57:52]

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:87: true);
On 2014/07/31 02:16:52, oshima wrote:
> why you're iterating the most list? Can't you just use
> output.display->native_mode() ?

removed the loop. good point.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.h:30: ASH_EXPORT static
std::vector<DisplayMode> GetDisplayModeList(
On 2014/07/31 02:16:52, oshima wrote:
> GetExternalModeList ?

Done.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_inf...
ash/display/display_info.h:30: bool operator==(const DisplayMode& other) const;
On 2014/07/31 02:16:52, oshima wrote:
> can you add comment that this is necessary for stl compatibility ? (style
guide
> still recommends not to use operator overloading in general)

This was added for options handler (check if an option is 'selected' or not).  I
missed the style guide. Renamed to a normal method.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_inf...
ash/display/display_info.h:37: // TODO(mukai|oshima): merge to
device_scale_factor.
On 2014/07/31 02:16:52, oshima wrote:
> should we?

removed because you're suspicious.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.cc:545: 
On 2014/07/31 02:16:52, oshima wrote:
> can you add comment that this is looking for "default" mode?

Done.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.cc:551: if (info.configured_ui_scale() ==
display_modes[i].ui_scale)
On 2014/07/31 02:16:52, oshima wrote:
> I believe the ui scale should always be 1.0f in this case? If so, can you add
> DCHECK? (in outs of the loop)

This case, ui scale comes from the pref store, can be different from 1.0f if the
user changes the ui-scale and reboot.

First I thought to tweak RegisterDisplayProperty() to remember restored ui scale
as a DisplayMode, but I gave up because display's original size is unknown,
which will introduce complication to GetSelectedModeForDisplayId.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.h:169: // TODO(mukai): remove this and merge into
SetDisplayMode.
On 2014/07/31 02:16:52, oshima wrote:
> or you can just move them to private

this method is still used by display_preferences_unittest.cc.  Better to keep
this for now.

[oshima, 2014-07-31 18:52:38]

lgtm

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_inf...
ash/display/display_info.h:30: bool operator==(const DisplayMode& other) const;
On 2014/07/31 04:57:52, Jun Mukai wrote:
> On 2014/07/31 02:16:52, oshima wrote:
> > can you add comment that this is necessary for stl compatibility ? (style
> guide
> > still recommends not to use operator overloading in general)
> 
> This was added for options handler (check if an option is 'selected' or not). 
I
> missed the style guide. Renamed to a normal method.

I'm personally fine with this as you used this in std::find(. I'll leave this to
 you.

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.cc:551: if (info.configured_ui_scale() ==
display_modes[i].ui_scale)
On 2014/07/31 04:57:52, Jun Mukai wrote:
> On 2014/07/31 02:16:52, oshima wrote:
> > I believe the ui scale should always be 1.0f in this case? If so, can you
add
> > DCHECK? (in outs of the loop)
> 
> This case, ui scale comes from the pref store, can be different from 1.0f if
the
> user changes the ui-scale and reboot.
> 
> First I thought to tweak RegisterDisplayProperty() to remember restored ui
scale
> as a DisplayMode, but I gave up because display's original size is unknown,
> which will introduce complication to GetSelectedModeForDisplayId.

Discussed offline and understood the issue. please add comment in
RegisterDisplayProperty so that we can address it correctly (if we ever want to
fix it)

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.h:169: // TODO(mukai): remove this and merge into
SetDisplayMode.
On 2014/07/31 04:57:52, Jun Mukai wrote:
> On 2014/07/31 02:16:52, oshima wrote:
> > or you can just move them to private
> 
> this method is still used by display_preferences_unittest.cc.  Better to keep
> this for now.

you can expose it via DisplayManagerTestApi (don't have to be this CL)

[Jun Mukai, 2014-07-31 20:32:50]

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/417113012/diff/120001/ash/display/display_man...
ash/display/display_manager.cc:551: if (info.configured_ui_scale() ==
display_modes[i].ui_scale)
On 2014/07/31 18:52:37, oshima wrote:
> On 2014/07/31 04:57:52, Jun Mukai wrote:
> > On 2014/07/31 02:16:52, oshima wrote:
> > > I believe the ui scale should always be 1.0f in this case? If so, can you
> add
> > > DCHECK? (in outs of the loop)
> > 
> > This case, ui scale comes from the pref store, can be different from 1.0f if
> the
> > user changes the ui-scale and reboot.
> > 
> > First I thought to tweak RegisterDisplayProperty() to remember restored ui
> scale
> > as a DisplayMode, but I gave up because display's original size is unknown,
> > which will introduce complication to GetSelectedModeForDisplayId.
> 
> Discussed offline and understood the issue. please add comment in
> RegisterDisplayProperty so that we can address it correctly (if we ever want
to
> fix it)

Done.

[Jun Mukai, 2014-07-31 20:33:38]

TBR=asvitkine for the description change in actions.xml (approved by the item
owner, stevenjb)

[Jun Mukai, 2014-07-31 20:33:42]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-31 20:36:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/417113012/160001

[Alexei Svitkine (slow), 2014-07-31 20:45:33]

actions.xml lgtm

[commit-bot: I haz the power, 2014-08-01 00:52:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 14:33:00]

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

[commit-bot: I haz the power, 2014-08-01 14:33:01]

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

[Jun Mukai, 2014-08-05 22:42:41]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-08-05 22:44:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/417113012/160001

[commit-bot: I haz the power, 2014-08-06 04:38:34]

Change committed as 287677