Allow storing display preferences per device.

Allow storing display preferences per device.


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

[Jun Mukai, 2012-08-27 11:06:33]

oshima-san, could you take a look at this?

[oshima, 2012-08-27 23:00:46]

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.cc:51: }
log if the position is not one of value above.

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.cc:62: : position(p), offset(o) {}
you probably want to check the range of both position as they can be read from
file.

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.cc:85: break;
default:
  return false;

is better.

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.cc:87: if (position_value.empty())
if you're worrying about fall through, it should be CHECK/DCHECK.

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
File ash/display/display_controller.h (right):

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.h:48: DisplayLayout(Position p, int o);
p -> position, o ->offset

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.h:51: bool GetValue(base::DictionaryValue*
value);
GetValue is a bit confusing. How about

SetValueTo or SaveValueTo ?

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.h:88: }
since DisplayLayout is POD, it'd be better to simply return pod, and let client
pick the member.

https://chromiumcodereview.appspot.com/10870036/diff/5001/ash/display/display...
ash/display/display_controller.h:99: int* layout, int* offset);
document these methods

https://chromiumcodereview.appspot.com/10870036/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/display_preferences.cc (right):

https://chromiumcodereview.appspot.com/10870036/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/display_preferences.cc:91: }
nuke {}

https://chromiumcodereview.appspot.com/10870036/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/display_preferences.cc:109:
display_controller->SetSecondaryDisplayOffset(offset);
sanity check these values.

https://chromiumcodereview.appspot.com/10870036/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/display_preferences.cc:115: const base::Value* value =
NULL;
how this can be const?

https://chromiumcodereview.appspot.com/10870036/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/display_preferences.h (right):

https://chromiumcodereview.appspot.com/10870036/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/display_preferences.h:8: #include
"base/compiler_specific.h"
do you need this?

[Jun Mukai, 2012-08-28 08:12:42]

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.cc:51: }
On 2012/08/27 23:00:46, oshima wrote:
> log if the position is not one of value above.

Done.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.cc:62: : position(p), offset(o) {}
On 2012/08/27 23:00:46, oshima wrote:
> you probably want to check the range of both position as they can be read from
> file.

Added DCHECK for position.  The offset value validity changes based on the
resolutions of both primary/secondary displays, so it's not possible to check
the validity here.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.cc:85: break;
On 2012/08/27 23:00:46, oshima wrote:
> default:
>   return false;
> 
> is better.

Done.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.cc:87: if (position_value.empty())
On 2012/08/27 23:00:46, oshima wrote:
> if you're worrying about fall through, it should be CHECK/DCHECK.

removed.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.h:48: DisplayLayout(Position p, int o);
On 2012/08/27 23:00:46, oshima wrote:
> p -> position, o ->offset

Done.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.h:51: bool GetValue(base::DictionaryValue*
value);
On 2012/08/27 23:00:46, oshima wrote:
> GetValue is a bit confusing. How about
> 
> SetValueTo or SaveValueTo ?

Changed to ConvertToValue().  What do you think?  What this method does is to
convert itself into a base::Value.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.h:88: }
On 2012/08/27 23:00:46, oshima wrote:
> since DisplayLayout is POD, it'd be better to simply return pod, and let
client
> pick the member.

Done.

http://codereview.chromium.org/10870036/diff/5001/ash/display/display_control...
ash/display/display_controller.h:99: int* layout, int* offset);
On 2012/08/27 23:00:46, oshima wrote:
> document these methods

Done.

http://codereview.chromium.org/10870036/diff/5001/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/display_preferences.cc (right):

http://codereview.chromium.org/10870036/diff/5001/chrome/browser/chromeos/dis...
chrome/browser/chromeos/display_preferences.cc:91: }
On 2012/08/27 23:00:46, oshima wrote:
> nuke {}

Done.

http://codereview.chromium.org/10870036/diff/5001/chrome/browser/chromeos/dis...
chrome/browser/chromeos/display_preferences.cc:109:
display_controller->SetSecondaryDisplayOffset(offset);
On 2012/08/27 23:00:46, oshima wrote:
> sanity check these values.

Thanks.  I changed the code to accept "SetDefaultDisplayLayout" by specifying
DisplayLayout struct, rather than accepting each field of the struct.  Then the
DisplayLayout constructor checks the validity, so I think it's unnecessary to
check here.

http://codereview.chromium.org/10870036/diff/5001/chrome/browser/chromeos/dis...
chrome/browser/chromeos/display_preferences.cc:115: const base::Value* value =
NULL;
On 2012/08/27 23:00:46, oshima wrote:
> how this can be const?

 DictionaryValue::Get needs this const-ness.  I guess DictionaryValue returns
its internal pointer here and that's why.

http://codereview.chromium.org/10870036/diff/5001/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/5001/chrome/browser/chromeos/dis...
chrome/browser/chromeos/display_preferences.h:8: #include
"base/compiler_specific.h"
On 2012/08/27 23:00:46, oshima wrote:
> do you need this?

removed.

[oshima, 2012-08-28 08:55:49]

http://codereview.chromium.org/10870036/diff/10001/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10870036/diff/10001/ash/display/display_contro...
ash/display/display_controller.cc:65: DCHECK_GE(DisplayLayout::LEFT, position);
Since this can be read from file, it's probably better to default to some value
in release build in addition to DCHECK.

I'd still DCHECK offset, say it's within abs(10000) range. (shouldn't be
bigger/samller than that). I believe offset's outlier is correctly handled, so
DCHECK should be enough.

http://codereview.chromium.org/10870036/diff/10001/ash/display/display_contro...
ash/display/display_controller.cc:334: // TODO(oshima|mukai): Implement more
flexible layout.
we don't need this TODO any more, correct?

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.cc (right):

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.cc:25: // Replaces dot "." by "%2E"
since it's the path separater of base::Value.
it also converts % -> %25, correct?

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.cc:125: UnescapeDisplayName(*it),
layout);
unescaped
(or you can just use UnescapeDisplayName(*it) here.

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.h:18: const gfx::Display& display,
int layout, int offset);
one argument per line

[Jun Mukai, 2012-08-28 09:51:00]

http://codereview.chromium.org/10870036/diff/10001/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10870036/diff/10001/ash/display/display_contro...
ash/display/display_controller.cc:65: DCHECK_GE(DisplayLayout::LEFT, position);
On 2012/08/28 08:55:50, oshima wrote:
> Since this can be read from file, it's probably better to default to some
value
> in release build in addition to DCHECK.
> 
> I'd still DCHECK offset, say it's within abs(10000) range. (shouldn't be
> bigger/samller than that). I believe offset's outlier is correctly handled, so
> DCHECK should be enough.

Done.

http://codereview.chromium.org/10870036/diff/10001/ash/display/display_contro...
ash/display/display_controller.cc:334: // TODO(oshima|mukai): Implement more
flexible layout.
On 2012/08/28 08:55:50, oshima wrote:
> we don't need this TODO any more, correct?

Removed.

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.cc (right):

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.cc:25: // Replaces dot "." by "%2E"
since it's the path separater of base::Value.
On 2012/08/28 08:55:50, oshima wrote:
> it also converts % -> %25, correct?

Added

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.cc:125: UnescapeDisplayName(*it),
layout);
On 2012/08/28 08:55:50, oshima wrote:
> unescaped
> (or you can just use UnescapeDisplayName(*it) here.

Remove unescaped.

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/10001/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.h:18: const gfx::Display& display,
int layout, int offset);
On 2012/08/28 08:55:50, oshima wrote:
> one argument per line

Done.

[oshima, 2012-08-29 00:10:43]

lgth wit nits.

https://chromiumcodereview.appspot.com/10870036/diff/14004/ash/display/displa...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/10870036/diff/14004/ash/display/displa...
ash/display/display_controller.cc:71: if (TOP > position || LEFT < position)
Add comment why we're doing this.

https://chromiumcodereview.appspot.com/10870036/diff/14004/ash/display/displa...
ash/display/display_controller.cc:74: DCHECK_GE(kMaxValidOffset, offset);
DCHECK_GE(kMaxValidOffsetAbs, abs(offset))

https://chromiumcodereview.appspot.com/10870036/diff/14004/ash/display/displa...
ash/display/display_controller.cc:220: return default_display_layout_;
just return (no else)

https://chromiumcodereview.appspot.com/10870036/diff/14004/chrome/browser/chr...
File chrome/browser/chromeos/display_preferences.h (right):

https://chromiumcodereview.appspot.com/10870036/diff/14004/chrome/browser/chr...
chrome/browser/chromeos/display_preferences.h:22: void
NotifyDisplayPrefChanged(PrefService* pref_service);
document these

https://chromiumcodereview.appspot.com/10870036/diff/14004/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/10870036/diff/14004/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:129: if (i >
0) {
I assume we're dropping 0 because 0 is primary and it's always centered?

[Jun Mukai, 2012-08-29 02:15:03]

OWNERS -- please review this from the owner's point of view:
sky -- ash/, chrome/
satorux -- chrome/browser/chromeos
nkostylev -- chrome/ui/webui/options/chromeos

thanks!

http://codereview.chromium.org/10870036/diff/14004/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10870036/diff/14004/ash/display/display_contro...
ash/display/display_controller.cc:71: if (TOP > position || LEFT < position)
On 2012/08/29 00:10:44, oshima wrote:
> Add comment why we're doing this.

Done.

http://codereview.chromium.org/10870036/diff/14004/ash/display/display_contro...
ash/display/display_controller.cc:74: DCHECK_GE(kMaxValidOffset, offset);
On 2012/08/29 00:10:44, oshima wrote:
> DCHECK_GE(kMaxValidOffsetAbs, abs(offset))

Done.

http://codereview.chromium.org/10870036/diff/14004/ash/display/display_contro...
ash/display/display_controller.cc:220: return default_display_layout_;
On 2012/08/29 00:10:44, oshima wrote:
> just return (no else)

Done.

http://codereview.chromium.org/10870036/diff/14004/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/14004/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.h:22: void
NotifyDisplayPrefChanged(PrefService* pref_service);
On 2012/08/29 00:10:44, oshima wrote:
> document these

Done.

http://codereview.chromium.org/10870036/diff/14004/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10870036/diff/14004/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:129: if (i >
0) {
On 2012/08/29 00:10:44, oshima wrote:
> I assume we're dropping 0 because 0 is primary and it's always centered?

Hmm, I'm afraid no.  the layout and offset are specified for the secondary
display.  The display controller would somehow returns these values, but it
might be wrong.

Fixed the code to export this from the for-loop, since I just wanted to obtain
these values from the secondary display.

[satorux1, 2012-08-29 04:06:09]

http://codereview.chromium.org/10870036/diff/20005/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/20005/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
we no longer want to add new files in chrome/browser/chromeos. Can you create
chrome/browser/chromeos/preferences directory and put preferences stuff there?
you can also add OWNERS file

[Jun Mukai, 2012-08-29 05:38:51]

http://codereview.chromium.org/10870036/diff/20005/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/20005/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/08/29 04:06:09, satorux1 wrote:
> we no longer want to add new files in chrome/browser/chromeos. Can you create
> chrome/browser/chromeos/preferences directory and put preferences stuff there?
> you can also add OWNERS file

Then I would rather want to put it chrome/browser/chromeos/display.  As far as I
see in that directory, the preference files are separated by its functions;
login/user_manager controls user list, audio/audio_handler does the audio
preferences, or something like that.

[sky, 2012-08-29 15:05:56]

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.cc:64: : position(RIGHT), offset(0) {}
When you wrap, each member on its own line (same on 67)

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.cc:216: std::map<std::string,
DisplayLayout>::const_iterator it =
Does this map ever need to be pruned?

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.h:47: DisplayLayout();
structs follow the same layout as classes, so methods before fields.

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.h:49: static void RegisterJSONConverter(
Move this to a standalone function, and ConvertToValue should be a standlone
function that takes a DisplayLayout.

http://codereview.chromium.org/10870036/diff/16007/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10870036/diff/16007/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:107:
ash::internal::DisplayController* display_controller =
Classes that are needed outside of ash shouldn't be in the internal namespace.

[satorux1, 2012-08-29 17:55:07]

http://codereview.chromium.org/10870036/diff/20005/chrome/browser/chromeos/di...
File chrome/browser/chromeos/display_preferences.h (right):

http://codereview.chromium.org/10870036/diff/20005/chrome/browser/chromeos/di...
chrome/browser/chromeos/display_preferences.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/08/29 05:38:51, Jun Mukai wrote:
> On 2012/08/29 04:06:09, satorux1 wrote:
> > we no longer want to add new files in chrome/browser/chromeos. Can you
create
> > chrome/browser/chromeos/preferences directory and put preferences stuff
there?
> > you can also add OWNERS file
> 
> Then I would rather want to put it chrome/browser/chromeos/display.  As far as
I
> see in that directory, the preference files are separated by its functions;
> login/user_manager controls user list, audio/audio_handler does the audio
> preferences, or something like that.

sounds good. please go ahead.

[Jun Mukai, 2012-08-30 02:08:04]

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.cc:64: : position(RIGHT), offset(0) {}
On 2012/08/29 15:05:57, sky wrote:
> When you wrap, each member on its own line (same on 67)

Done.

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.cc:216: std::map<std::string,
DisplayLayout>::const_iterator it =
On 2012/08/29 15:05:57, sky wrote:
> Does this map ever need to be pruned?

I can't find the necessity to prune it.  It's the mapping of remembered display
model names to its layout, so pruning means forgetting the display explicitly. 
This isn't supported, and I don't think we need to support it.

Please talk to oshima if you think I'm wrong.

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.h:47: DisplayLayout();
On 2012/08/29 15:05:57, sky wrote:
> structs follow the same layout as classes, so methods before fields.

Done.

http://codereview.chromium.org/10870036/diff/16007/ash/display/display_contro...
ash/display/display_controller.h:49: static void RegisterJSONConverter(
On 2012/08/29 15:05:57, sky wrote:
> Move this to a standalone function, and ConvertToValue should be a standlone
> function that takes a DisplayLayout.

Sorry, this is needed to be a static method since JSONValueConverter needs that,
see
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/json/json_value_conve...

Wrote converter functions and put comment there instead.

http://codereview.chromium.org/10870036/diff/16007/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10870036/diff/16007/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:107:
ash::internal::DisplayController* display_controller =
On 2012/08/29 15:05:57, sky wrote:
> Classes that are needed outside of ash shouldn't be in the internal namespace.

Done.

[Nikita (slow), 2012-08-30 14:18:51]

chrome/browser/ui/webui/options/chromeos lgtm

http://codereview.chromium.org/10870036/diff/26001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10870036/diff/26001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:139:
mirroring, displays, *layout_value, *offset_value);
layout_value.get()
offset_value.get()

[Jun Mukai, 2012-08-31 09:19:05]

http://codereview.chromium.org/10870036/diff/26001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10870036/diff/26001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:139:
mirroring, displays, *layout_value, *offset_value);
On 2012/08/30 14:18:52, Nikita Kostylev wrote:
> layout_value.get()
> offset_value.get()

Done.

[Jun Mukai, 2012-08-31 09:19:18]

sky, please take another look at this.

[sky, 2012-08-31 15:46:22]

LGTM

[commit-bot: I haz the power, 2012-09-03 01:16:43]

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

[commit-bot: I haz the power, 2012-09-03 01:16:53]

Failed to apply patch for ash/display/display_controller.cc:
While running patch -p1 --forward --force;
patching file ash/display/display_controller.cc
Hunk #2 FAILED at 27.
Hunk #3 FAILED at 127.
Hunk #4 succeeded at 218 (offset -21 lines).
Hunk #5 succeeded at 244 (offset -21 lines).
Hunk #6 succeeded at 264 (offset -21 lines).
2 out of 6 hunks FAILED -- saving rejects to file
ash/display/display_controller.cc.rej