Prevent DisplayPreferences from saving incorrect rotations.

Refactor DisplayInfo::rotation_ to track different sources of rotations

Currently DisplayInfo only tracks the active rotation for the given display.
DisplayPreferences however saves a user rotation, as well as an accelerometer
rotation. DisplayController::Observer::OnDisplayConfigurationChanged triggers
the saving of display preferences.

This has been leading to active accelerometer rotations being saved as user
preferences, and being re-applied upon reboot.

This change refactors DisplayInfo to track one rotation per source of rotation
changes. DisplayPreferences has been updated to save based on these states.

TEST=DisplayPreferencesTest.DontSaveMaximizeModeControllerRotations, also ran
ash_unittests, and unit_tests
BUG=chrome-os-partner:37555, 469752, 466861
Committed: https://crrev.com/d01de7f9b271d29dc1fe9126357a36c5563f0ff0
Cr-Commit-Position: refs/heads/master@{#326614}

[jonross, 2015-04-10 17:45:29]

Patchset #1 (id:1) has been deleted

[jonross, 2015-04-10 17:54:42]

jonross@chromium.org changed reviewers:
+ tdanderson@chromium.org

[jonross, 2015-04-10 17:54:44]

Hi Terry,

This change is the first that is a part of the Display Rotation refactoring that
we discussed.

This change adds tracking of one rotation per source per display. So that
display preferences is able to properly save both the user preference, along
with the accelerometer option.

Could you provide an initial review?

Thanks.

https://codereview.chromium.org/1071353003/diff/20001/ash/content/display/scr...
File ash/content/display/screen_orientation_controller_chromeos_unittest.cc
(left):

https://codereview.chromium.org/1071353003/diff/20001/ash/content/display/scr...
ash/content/display/screen_orientation_controller_chromeos_unittest.cc:44:
gfx::Display::Rotation GetInternalDisplayRotation() {
This method is duplicated on 51, was missed in a previous change.

In general this pattern is throughout the test classes. I'm thinking of moving
it to ash_test_helper, or a more appropriate location.

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

https://codereview.chromium.org/1071353003/diff/20001/ash/display/display_inf...
ash/display/display_info.h:144: gfx::Display::Rotation
Rotation(gfx::Display::RotationSource source) const;
Getting the active rotation is a common action.

Considering adding:

gfx::Display::Rotation ActiveRotation() const;

https://codereview.chromium.org/1071353003/diff/20001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):

https://codereview.chromium.org/1071353003/diff/20001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:165:
gfx::Display::Rotation GetInternalDisplayRotation() const {
No longer used. Their removal was missed in a different change.

https://codereview.chromium.org/1071353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/1071353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:256:
static_cast<int>(info.Rotation(gfx::Display::ROTATION_SOURCE_USER)));
This is not just a refactor, but addresses the bug of the wrong orientation
being saved.

https://codereview.chromium.org/1071353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/1071353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences_unittest.cc:712: //
rotation.
Test for incorrect orientation being saved.

[tdanderson, 2015-04-13 22:44:46]

Hey Jon, I have some nits for you below, but otherwise LGTM.

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ash/display/disp...
File ash/display/display_info.cc (right):

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ash/display/disp...
ash/display/display_info.cc:268: gfx::Display::Rotation DisplayInfo::Rotation(
Maybe name this GetRotation() or GetRotationForSource() ?

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ash/display/disp...
File ash/display/display_info.h (right):

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ash/display/disp...
ash/display/display_info.h:144: gfx::Display::Rotation
Rotation(gfx::Display::RotationSource source) const;
On 2015/04/10 17:54:43, jonross wrote:
> Getting the active rotation is a common action.
> 
> Considering adding:
> 
> gfx::Display::Rotation ActiveRotation() const;

+1 to adding that.

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ash/rotator/scre...
File ash/rotator/screen_rotation_animator.h (right):

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ash/rotator/scre...
ash/rotator/screen_rotation_animator.h:20: // Rotates |display_| to the
|new_rotation| orientation.
nit: Update documentation here to mention what |source| means. Same comment for
display_manager.h and screen_orientation_controller_chromeos.h.

https://chromiumcodereview.appspot.com/1071353003/diff/20001/chrome/browser/c...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://chromiumcodereview.appspot.com/1071353003/diff/20001/chrome/browser/c...
chrome/browser/chromeos/display/display_preferences.cc:256:
static_cast<int>(info.Rotation(gfx::Display::ROTATION_SOURCE_USER)));
On 2015/04/10 17:54:43, jonross wrote:
> This is not just a refactor, but addresses the bug of the wrong orientation
> being saved.

nit: Consider changing your CL title to mention the bug you're actually fixing.
Someone looking for regression suspects in a git log may easily overlook this CL
if the title leads them to believe it's a pure refactoring CL.

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://chromiumcodereview.appspot.com/1071353003/diff/20001/ui/gfx/display.h...
ui/gfx/display.h:33: // The cause of a change in display rotation.
Nit: For the ease of someone reading this for the first time, consider giving a
bit more context in the comment regarding what each of these mean.

[jonross, 2015-04-15 17:39:59]

Patchset #2 (id:40001) has been deleted

[jonross, 2015-04-15 17:43:12]

https://codereview.chromium.org/1071353003/diff/20001/ash/display/display_inf...
File ash/display/display_info.cc (right):

https://codereview.chromium.org/1071353003/diff/20001/ash/display/display_inf...
ash/display/display_info.cc:268: gfx::Display::Rotation DisplayInfo::Rotation(
On 2015/04/13 22:44:46, tdanderson wrote:
> Maybe name this GetRotation() or GetRotationForSource() ?

Done.

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

https://codereview.chromium.org/1071353003/diff/20001/ash/display/display_inf...
ash/display/display_info.h:144: gfx::Display::Rotation
Rotation(gfx::Display::RotationSource source) const;
On 2015/04/13 22:44:46, tdanderson wrote:
> On 2015/04/10 17:54:43, jonross wrote:
> > Getting the active rotation is a common action.
> > 
> > Considering adding:
> > 
> > gfx::Display::Rotation ActiveRotation() const;
> 
> +1 to adding that.

Done.

https://codereview.chromium.org/1071353003/diff/20001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator.h (right):

https://codereview.chromium.org/1071353003/diff/20001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.h:20: // Rotates |display_| to the
|new_rotation| orientation.
On 2015/04/13 22:44:46, tdanderson wrote:
> nit: Update documentation here to mention what |source| means. Same comment
for
> display_manager.h and screen_orientation_controller_chromeos.h.

Done.

https://codereview.chromium.org/1071353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/1071353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:256:
static_cast<int>(info.Rotation(gfx::Display::ROTATION_SOURCE_USER)));
On 2015/04/13 22:44:46, tdanderson wrote:
> On 2015/04/10 17:54:43, jonross wrote:
> > This is not just a refactor, but addresses the bug of the wrong orientation
> > being saved.
> 
> nit: Consider changing your CL title to mention the bug you're actually
fixing.
> Someone looking for regression suspects in a git log may easily overlook this
CL
> if the title leads them to believe it's a pure refactoring CL.

Done.

https://codereview.chromium.org/1071353003/diff/20001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1071353003/diff/20001/ui/gfx/display.h#newcode33
ui/gfx/display.h:33: // The cause of a change in display rotation.
On 2015/04/13 22:44:46, tdanderson wrote:
> Nit: For the ease of someone reading this for the first time, consider giving
a
> bit more context in the comment regarding what each of these mean.

Done.

[jonross, 2015-04-15 17:45:22]

jonross@chromium.org changed reviewers:
+ oshima@chromium.org

[jonross, 2015-04-15 17:45:22]

oshima@chromium.org: Please review changes in 

ash/*
chrome/browser/chromeos/display/
ui/gfx/display.h

This change is the first stage in my proposed display rotation refactor. Where
DisplayInfo tracks one rotation per source.

I also update DisplayPreferences to correctly save the desired rotation sources.

Thanks,
Jon

[oshima, 2015-04-17 17:52:44]

lgtm with nits

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

https://codereview.chromium.org/1071353003/diff/60001/ash/display/display_inf...
ash/display/display_info.h:146: gfx::Display::Rotation ActiveRotation() const;
GetActiveRotation()

https://codereview.chromium.org/1071353003/diff/60001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1071353003/diff/60001/ui/gfx/display.h#newcode39
ui/gfx/display.h:39: ROTATION_SOURCE_ACTIVE,
document what ACTIVE means.

[jonross, 2015-04-17 19:40:03]

jonross@chromium.org changed reviewers:
+ mukai@chromium.org

[jonross, 2015-04-17 19:40:04]

mukai@chromium.org: Please review changes in 

chrome/browser/ui/webui/options/chromeos/display_options_handler.cc

I have updated it as a part of a refactoring of DisplayInfo.

Thanks.

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

https://codereview.chromium.org/1071353003/diff/60001/ash/display/display_inf...
ash/display/display_info.h:146: gfx::Display::Rotation ActiveRotation() const;
On 2015/04/17 17:52:44, oshima wrote:
> GetActiveRotation()

Done.

https://codereview.chromium.org/1071353003/diff/60001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/1071353003/diff/60001/ui/gfx/display.h#newcode39
ui/gfx/display.h:39: ROTATION_SOURCE_ACTIVE,
On 2015/04/17 17:52:44, oshima wrote:
> document what ACTIVE means.

Done.

[jonross, 2015-04-17 19:41:49]

jonross@chromium.org changed reviewers:
+ asargent@chromium.org

[jonross, 2015-04-17 19:41:50]

asargent@chromium.org: Please review changes in 

chrome/browser/extensions/display_info_provider_chromeos*

I have updated them as a part of a refactor of DisplayInfo.

Thanks.

[jonross, 2015-04-17 19:44:36]

jonross@chromium.org changed reviewers:
- asargent@chromium.org

[jonross, 2015-04-17 19:44:56]

jonross@chromium.org changed reviewers:
+ kalman@chromium.org

[jonross, 2015-04-17 19:44:56]

kalman@chromium.org: Please review changes in 

chrome/browser/extensions/display_info_provider_chromeos*

I have updated them as a part of a refactor of DisplayInfo.

Thanks.

[Jun Mukai, 2015-04-17 19:51:19]

c/b/ui/webui/options/chromeos/display_options_handler.cc lgtm.

[not at google - send to devlin, 2015-04-17 19:53:26]

kalman@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[not at google - send to devlin, 2015-04-17 19:53:26]

pkotwicz@ could you check the extensions change? I will lg after that.

[pkotwicz, 2015-04-17 20:33:28]

Jon, is there a reason that this issue is closed?

[jonross, 2015-04-17 20:41:09]

On 2015/04/17 20:33:28, pkotwicz wrote:
> Jon, is there a reason that this issue is closed?

I miss clicked earlier when trying to edit the issue. I reopened it

[pkotwicz, 2015-04-17 21:11:38]

kaman@ who are the users of chrome.system.display.setInfo(). It looks like the
API was added here: https://code.google.com/p/chromium/issues/detail?id=237314

[pkotwicz, 2015-04-17 21:12:13]

kalman@ who are the users of chrome.system.display.setInfo(). It looks like the
API was added here: https://code.google.com/p/chromium/issues/detail?id=237314

[pkotwicz, 2015-04-17 21:13:01]

Looks like the extension API name is actually
chrome.system.display.setDisplayProperties()

[Jun Mukai, 2015-04-17 21:23:21]

mukai@chromium.org changed reviewers:
+ hshi@chromium.org

[Jun Mukai, 2015-04-17 21:23:51]

On 2015/04/17 21:13:01, pkotwicz wrote:
> Looks like the extension API name is actually
> chrome.system.display.setDisplayProperties()

IIRC extension API for display settings was added by hshi.

[jonross, 2015-04-17 21:27:48]

On 2015/04/17 21:23:51, Jun Mukai wrote:
> On 2015/04/17 21:13:01, pkotwicz wrote:
> > Looks like the extension API name is actually
> > chrome.system.display.setDisplayProperties()
> 
> IIRC extension API for display settings was added by hshi.

When I did a code search the only current caller appeared to be the unittest:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[jonross, 2015-04-20 14:21:39]

hshi@

Would you be able to review the changes to:
chrome/browser/extensions/display_info_provider_chromeos.cc
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc

Thanks,
Jon

[hshi1, 2015-04-20 15:23:17]

On 2015/04/20 14:21:39, jonross wrote:
> hshi@
> 
> Would you be able to review the changes to:
> chrome/browser/extensions/display_info_provider_chromeos.cc
> chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
> 
> Thanks,
> Jon

display_info_provider_chromeos(_unittest?).cc LGTM

[pkotwicz, 2015-04-20 15:31:56]

hshi@ For the sake of interest, in which cases is the API
chrome.system.display.setDisplayProperties() used?

[hshi1, 2015-04-20 16:26:30]

On 2015/04/20 15:31:56, pkotwicz wrote:
> hshi@ For the sake of interest, in which cases is the API
> chrome.system.display.setDisplayProperties() used?

pkotwicz@ I don't know. This was added by tbarzic@ in http://crrev.com/206576

[pkotwicz, 2015-04-20 17:19:52]

pkotwicz@chromium.org changed reviewers:
+ tbarzic@chromium.org

[pkotwicz, 2015-04-20 17:21:28]

tbarzic@, do you know in which cases the API
chrome.system.display.setDisplayProperties() is used?

[tbarzic, 2015-04-20 17:31:57]

On 2015/04/20 17:21:28, pkotwicz wrote:
> tbarzic@, do you know in which cases the API
> chrome.system.display.setDisplayProperties() is used?

If I remember correctly, the primary use case for the API is to enable kiosk
apps to adjust display properties (e.g. adjusting overscan).

[jonross, 2015-04-21 13:33:59]

pkotwicz@ & kalman@

Any concerns regarding the extensions changes?

[jonross, 2015-04-21 14:56:54]

tbarzic@ in the code review that landed the change:
https://chromiumcodereview.appspot.com/16817006
it mentions manual testing on daisy.

Do you have a sample application that was used for testing that I could use to
confirm this portion of the change?

[tbarzic, 2015-04-21 18:07:27]

On 2015/04/21 14:56:54, jonross wrote:
> tbarzic@ in the code review that landed the change:
> https://chromiumcodereview.appspot.com/16817006
> it mentions manual testing on daisy.
> 
> Do you have a sample application that was used for testing that I could use to
> confirm this portion of the change?

No, I don't. I had a simple app for testing, but I can't find it anymore.

[jonross, 2015-04-21 21:02:38]

On 2015/04/21 18:07:27, tbarzic wrote:
> On 2015/04/21 14:56:54, jonross wrote:
> > tbarzic@ in the code review that landed the change:
> > https://chromiumcodereview.appspot.com/16817006
> > it mentions manual testing on daisy.
> > 
> > Do you have a sample application that was used for testing that I could use
to
> > confirm this portion of the change?
> 
> No, I don't. I had a simple app for testing, but I can't find it anymore.

I checked all kiosk-only apps that use 'chrome.system.display', along with the
top 20 kiosk-enabled apps which also use that.

None refer to setDisplayProperties.

With tbarzic@ without a test application, that leaves the unittest as the only
current caller.

[pkotwicz, 2015-04-22 01:01:29]

chrome/browser/extensions LGTM

Thanks a lot for doing the digging Jon!

[jonross, 2015-04-22 21:27:19]

On 2015/04/22 01:01:29, pkotwicz wrote:
> chrome/browser/extensions LGTM
> 
> Thanks a lot for doing the digging Jon!

No problem, thanks for the assistance with that.

kalman@ would you be able to provide the owners review for:
chrome/browser/extensions/display_info_provider_chromeos*.cc

Thanks,
Jon

[not at google - send to devlin, 2015-04-22 21:34:17]

lgtm

[jonross, 2015-04-22 21:39:13]

The CQ bit was checked by jonross@chromium.org

[jonross, 2015-04-22 21:39:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1071353003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-04-22 21:40:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071353003/100001

[commit-bot: I haz the power, 2015-04-22 23:03:10]

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

[commit-bot: I haz the power, 2015-04-22 23:03:11]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jonross, 2015-04-23 18:10:06]

The CQ bit was checked by jonross@chromium.org

[jonross, 2015-04-23 18:10:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, pkotwicz@chromium.org, oshima@chromium.org,
mukai@chromium.org, hshi@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1071353003/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-23 18:12:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071353003/120001

[commit-bot: I haz the power, 2015-04-23 19:52:20]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-04-23 19:53:21]

Patchset 5 (id:??) landed as
https://crrev.com/d01de7f9b271d29dc1fe9126357a36c5563f0ff0
Cr-Commit-Position: refs/heads/master@{#326614}