Work In Progess

Work In Progess

[jonross, 2017-03-16 23:59:16]

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

[jonross, 2017-03-16 23:59:17]

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_ozone_internal.cc (right):

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:220: connector_ =
service_manager::Connector::Create(&connector_request);
You should not be trying to create a new Connector here. This is an asynchronous
process.

Since we expect the display code to be ran in both classic ash, along within the
window server, you should rely on the preexisting Connector for the process.
Have both places which instantiate this pass in the Connector for their process.

Then use that to connect to the preferences service.

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:229:
pref_client_store_->Subscribe({prefs::kDisplayPowerState});
These should be enqueued in a vector and only one call to subscribe

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:231:
pref_client_store_->AddObserver(this);
This should be done before subscribing.

See the example code in:
https://codereview.chromium.org/2474653003/diff/280001/ash/common/shelf/shelf...

[thanhph, 2017-03-17 19:11:43]

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_ozone_internal.cc (right):

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:220: connector_ =
service_manager::Connector::Create(&connector_request);
On 2017/03/16 23:59:16, jonross wrote:
> You should not be trying to create a new Connector here. This is an
asynchronous
> process.
> 
> Since we expect the display code to be ran in both classic ash, along within
the
> window server, you should rely on the preexisting Connector for the process.
> Have both places which instantiate this pass in the Connector for their
process.
> 
> Then use that to connect to the preferences service.

Thanks! I use connector() in window_manager and pass |connector| to this Init()
as an argument.

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:229:
pref_client_store_->Subscribe({prefs::kDisplayPowerState});
On 2017/03/16 23:59:16, jonross wrote:
> These should be enqueued in a vector and only one call to subscribe

Done.

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:231:
pref_client_store_->AddObserver(this);
On 2017/03/16 23:59:16, jonross wrote:
> This should be done before subscribing.
> 
> See the example code in:
>
https://codereview.chromium.org/2474653003/diff/280001/ash/common/shelf/shelf...

Thanks! I realize preferences::PrefObserverStore no longer exists though.

[jonross, 2017-03-17 19:18:34]

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
File services/ui/display/screen_manager_ozone_internal.cc (right):

https://codereview.chromium.org/2760453003/diff/1/services/ui/display/screen_...
services/ui/display/screen_manager_ozone_internal.cc:231:
pref_client_store_->AddObserver(this);
On 2017/03/17 19:11:43, thanhph wrote:
> On 2017/03/16 23:59:16, jonross wrote:
> > This should be done before subscribing.
> > 
> > See the example code in:
> >
>
https://codereview.chromium.org/2474653003/diff/280001/ash/common/shelf/shelf...
> 
> Thanks! I realize preferences::PrefObserverStore no longer exists though.

PrefObserverStore was renamed PrefClientStore