New issue
Advanced search Search tips

Issue 721961 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

PrefChangeRegistrar doesn't work in mash

Project Member Reported by afakhry@chromium.org, May 13 2017

Issue description

Example failure of a test I added for NightLight.

The test changes some pref values and expects that the NightLightController sees these changes through its PrefChangeRegistrar. This works fine in ash_unittests, but not in mash_unittests.


[ RUN      ] NightLightTest.TestOutsidePreferencesChangesAreApplied
[2493:2493:0512/153318.371361:26234130575:ERROR:window_tree_host_mus.cc(211)] Not implemented reached in virtual void aura::WindowTreeHostMus::MoveCursorToScreenLocationInPixels(const gfx::Point &)
../../ash/system/night_light/night_light_controller_unittest.cc:219: Failure
Value of: controller->enabled()
  Actual: false
Expected: true
../../ash/system/night_light/night_light_controller_unittest.cc:222: Failure
      Expected: temperature1
      Which is: 65
To be equal to: controller->color_temperature()
      Which is: 50
[2493:2493:0512/153318.372256:26234131468:ERROR:night_light_controller_unittest.cc(39)] Unexpected layer temperature (0.65 vs. 0).
../../ash/system/night_light/night_light_controller_unittest.cc:223: Failure
Value of: TestLayersTemperature(temperature1 / 100.0f)
  Actual: false
Expected: true
../../ash/system/night_light/night_light_controller_unittest.cc:226: Failure
      Expected: temperature2
      Which is: 23
To be equal to: controller->color_temperature()
      Which is: 50
[2493:2493:0512/153318.372318:26234131530:ERROR:night_light_controller_unittest.cc(39)] Unexpected layer temperature (0.23 vs. 0).
../../ash/system/night_light/night_light_controller_unittest.cc:227: Failure
Value of: TestLayersTemperature(temperature2 / 100.0f)
  Actual: false
Expected: true

I will skip that in mash for now.
 

Comment 1 by sky@chromium.org, May 14 2017

Shell::Init() is responsible for doing the pref wiring. The code is conditional based on whether ShellDelegate::GetShellConnector() returns non-null. I believe we don't have a real connector in tests, and using a real connector is likely to have other ramifications we don't want. Where is PrefChangeRegistrar created for classic ash tests?

Comment 2 by sky@chromium.org, May 14 2017

Components: Tests>Disabled
Labels: Proj-Mustash-Mash
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8532dbc1799380b044b950372e23296259d267e0

commit 8532dbc1799380b044b950372e23296259d267e0
Author: James Cook <jamescook@chromium.org>
Date: Mon Jul 31 21:54:51 2017

mash: Connect to the correct user profile pref service on startup

Previously the pref service was connected to the temporary login screen
profile prefs. Now it connects to the user profile on login and after
user switch.

Rename RegisterPrefs() to RegisterProfilePrefs() in ash to emphasize
that these are profile-associated and not local state.

Add ShellObserver notification of PrefService change. The lifetime is
tricky because the old PrefService must continue to exist so shell
observers can unregister their pref observers. Also, there is a window
using user switch where the user pref service is null.

This is based on https://chromium-review.googlesource.com/c/580115
and is a prerequisite for moving some system tray code out of chrome
into ash.

Bug:  747019 ,  721961 , 736169
Test: added to ash_unittests
Change-Id: I186e55c1d88e0e2052b9cd76a490e26804e4b6d3
Reviewed-on: https://chromium-review.googlesource.com/590356
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490804}
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/shell.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/shell.h
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/shell_observer.h
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/shell_unittest.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/system/night_light/night_light_controller.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/system/night_light/night_light_controller.h
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/system/night_light/night_light_controller_unittest.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/ash/system/night_light/tray_night_light_unittest.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/chrome/browser/prefs/active_profile_pref_service.cc
[modify] https://crrev.com/8532dbc1799380b044b950372e23296259d267e0/chrome/browser/prefs/active_profile_pref_service.h

Owner: jamescook@chromium.org
Status: Assigned (was: Untriaged)
Status: Fixed (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 2 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b2cd05166ee2fa3399d8351a65bc3599a8c5122

commit 2b2cd05166ee2fa3399d8351a65bc3599a8c5122
Author: James Cook <jamescook@chromium.org>
Date: Wed Aug 02 01:10:24 2017

M61 backport: mash: Connect to the correct user profile pref service on startup

Previously the pref service was connected to the temporary login screen
profile prefs. Now it connects to the user profile on login and after
user switch.

Rename RegisterPrefs() to RegisterProfilePrefs() in ash to emphasize
that these are profile-associated and not local state.

Add ShellObserver notification of PrefService change. The lifetime is
tricky because the old PrefService must continue to exist so shell
observers can unregister their pref observers. Also, there is a window
using user switch where the user pref service is null.

This is based on https://chromium-review.googlesource.com/c/580115
and is a prerequisite for moving some system tray code out of chrome
into ash.

TBR=jamescook@chromium.org

(cherry picked from commit 8532dbc1799380b044b950372e23296259d267e0)

Bug:  747019 ,  721961 , 736169
Test: added to ash_unittests
Change-Id: I186e55c1d88e0e2052b9cd76a490e26804e4b6d3
Reviewed-on: https://chromium-review.googlesource.com/590356
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490804}
Reviewed-on: https://chromium-review.googlesource.com/597313
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#227}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/shell.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/shell.h
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/shell_observer.h
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/shell_unittest.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/system/night_light/night_light_controller.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/system/night_light/night_light_controller.h
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/system/night_light/night_light_controller_unittest.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/ash/system/night_light/tray_night_light_unittest.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/chrome/browser/prefs/active_profile_pref_service.cc
[modify] https://crrev.com/2b2cd05166ee2fa3399d8351a65bc3599a8c5122/chrome/browser/prefs/active_profile_pref_service.h

Comment 7 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment