Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(65)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2839643002: Have OobeUI handle display observation (Closed)

Created:
3 years, 8 months ago by Felix Ekblom
Modified:
3 years, 7 months ago
CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Have OobeUI handle display observation This is a refactoring CL. Move LoginDisplayHostImpl's DisplayObserver implementation to OobeUI since the implementation is only doing OobeUI related stuff. BUG=668449

Patch Set 1 #

Patch Set 2 : Remove leftover include #

Patch Set 3 : And also remove the other leftover header #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -38 lines) Patch
M chrome/browser/chromeos/login/ui/login_display_host_impl.h View 1 3 chunks +0 lines, -8 lines 0 comments Download
M chrome/browser/chromeos/login/ui/login_display_host_impl.cc View 1 2 4 chunks +0 lines, -30 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/oobe_ui.h View 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/oobe_ui.cc View 3 chunks +22 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (3 generated)
Felix Ekblom
3 years, 8 months ago (2017-04-24 14:35:17 UTC) #3
Alexander Alekseev
Taking into account https://crbug.com/714411 we will probably not approve this (at least before fixing 714411), ...
3 years, 7 months ago (2017-04-25 23:20:52 UTC) #5
Felix Ekblom
3 years, 7 months ago (2017-04-26 07:33:59 UTC) #6 
On 2017/04/25 23:20:52, Alexander Alekseev wrote:
> Taking into account https://crbug.com/714411 we will probably not approve this
> (at least before fixing 714411), but thank you very much for pointing to the
> possible issue ;)

Thanks for making me aware of that bug - I completely missed it!

> If you need to move move this code out of LoginDisplayHost for some other
> reasons than refactoring, please explain.

It was only for refactoring reasons. My solution attempt for 668449 (not yet in
review) pings OobeUI from another of DisplayObserver's events. With the  growing
OobeUI focus in the listener it felt like a good thing at the time to move the
observation responsibility to OobeUI.

I'll close this review and pipe the event through LogisDisplayHost in the
upcoming review. Thanks for the feedback!

Powered by Google App Engine
This is Rietveld 408576698