Issue 872633008: Migrate (Network)ErrorScreen to ScreenContext

Issue 872633008: Migrate (Network)ErrorScreen to ScreenContext (Closed)

Created:
5 years, 10 months ago by Nikita (slow)

Modified:
5 years, 9 months ago

Reviewers:
Denis Kuznetsov (DE-MUC), Dmitry Polukhin

CC:
chromium-reviews, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Migrate (Network)ErrorScreen to ScreenContext BUG=455705 TEST=Existing tests, manual. Committed: https://crrev.com/92701406a5811e0f53c06585be71b6d167d6aefb Cr-Commit-Position: refs/heads/master@{#316209}

Patch Set 1 #

Patch Set 2 : NetworkError #

Patch Set 3 : iter #

Patch Set 4 : rebase #

Patch Set 5 : JS, remove actor #

Patch Set 6 : cleanup #

Total comments: 30

Patch Set 7 : review #

Patch Set 8 : rebase #

Patch Set 9 : subscribe to context changes in UI #

Patch Set 10 : rebase #

Total comments: 2

Patch Set 11 : nit #

Total comments: 1

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1005 lines, -814 lines)			Patch
M	chrome/browser/chromeos/extensions/file_manager/event_router.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/app_launch_controller.cc	View	1 2 3 4	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/chromeos/login/app_launch_signin_screen.cc	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/enable_debugging_browsertest.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.h	View	1	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc	View	1	4 chunks	+8 lines, -6 lines	0 comments	Download
M	chrome/browser/chromeos/login/error_screens_histogram_helper.h	View	1	3 chunks	+8 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/login/error_screens_histogram_helper.cc	View	1	5 chunks	+19 lines, -18 lines	0 comments	Download
M	chrome/browser/chromeos/login/error_screens_histogram_helper_unittest.cc	View	1	6 chunks	+21 lines, -20 lines	0 comments	Download
M	chrome/browser/chromeos/login/login_manager_test.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/reset_browsertest.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/error_screen.h	View	1 2 3 4 5 6	2 chunks	+56 lines, -67 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/error_screen.cc	View	1 2 3 4 5 6 7 8	2 chunks	+195 lines, -53 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/error_screen_actor.h	View	1 2	4 chunks	+7 lines, -24 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/error_screen_actor.cc	View	1 2	1 chunk	+4 lines, -44 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/mock_error_screen.h	View	1 2 3 4 5 6	2 chunks	+31 lines, -22 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/mock_error_screen.cc	View	1 2 3 4 5 6	1 chunk	+37 lines, -4 lines	0 comments	Download
A	chrome/browser/chromeos/login/screens/network_error.h	View	1 2	1 chunk	+57 lines, -0 lines	0 comments	Download
A +	chrome/browser/chromeos/login/screens/network_error.cc	View	1 2	2 chunks	+14 lines, -22 lines	0 comments	Download
A	chrome/browser/chromeos/login/screens/network_error_model.h	View	1 2 3 4 5 6	1 chunk	+91 lines, -0 lines	0 comments	Download
A	chrome/browser/chromeos/login/screens/network_error_model.cc	View	2 3 4 5 6	1 chunk	+44 lines, -0 lines	0 comments	Download
A	chrome/browser/chromeos/login/screens/network_error_view.h	View	1 2 3 4	1 chunk	+42 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/update_screen.cc	View	1 2 3 4 5 6 7 8 9	5 chunks	+7 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/update_screen_browsertest.cc	View	1 2 3 4 5 6	9 chunks	+32 lines, -42 lines	0 comments	Download
M	chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc	View	1 3 4 5 6	3 chunks	+5 lines, -6 lines	0 comments	Download
M	chrome/browser/chromeos/login/ui/captive_portal_window_browsertest.cc	View	1	2 chunks	+6 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/login/ui/oobe_display.h	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/wizard_controller.cc	View	1 2 3 4 5 6 7	5 chunks	+8 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/login/wizard_controller_browsertest.cc	View	1 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/screen_error_message.js	View	1 2 3 4 5 6 7 8 9 10	12 chunks	+75 lines, -29 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h	View	1 2 3 4 5 6 7	4 chunks	+7 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc	View	1 2 3 4 5 6 7	6 chunks	+22 lines, -21 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h	View	1 2 3 4 5 6 7	5 chunks	+8 lines, -8 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc	View	1 2 3 4 5 6 7	9 chunks	+39 lines, -41 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/error_screen_handler.h	View	1 2 3 4 5 6 7	2 chunks	+12 lines, -56 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc	View	1 2 3 4 5 6 7 8	4 chunks	+25 lines, -197 lines	1 comment	Download
M	chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc	View	1 2 3 4 5 6 7	5 chunks	+6 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.h	View	1 2 3 4	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc	View	1 2	3 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/network_state_informer.h	View	1 2 3 4	3 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/network_state_informer.cc	View	1 2	3 chunks	+5 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/oobe_ui.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/oobe_ui.cc	View	1 2 3 4	3 chunks	+15 lines, -13 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h	View	1 2 3 4 5 6 7	6 chunks	+7 lines, -10 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc	View	1 2 3 4 5 6 7	16 chunks	+63 lines, -69 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/update_screen_handler.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/chrome_browser_chromeos.gypi	View	1 2 3 4 5 6 7	3 chunks	+6 lines, -2 lines	0 comments	Download

Messages

Total messages: 15 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Denis Kuznetsov (DE-MUC)

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos/login/screens/error_screen.cc File chrome/browser/chromeos/login/screens/error_screen.cc (right): https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos/login/screens/error_screen.cc#newcode80 chrome/browser/chromeos/login/screens/error_screen.cc:80: LOG(WARNING) << "Network error screen message is shown"; Is ...

5 years, 10 months ago (2015-02-10 17:03:26 UTC) #2

Nikita (slow)

5 years, 10 months ago (2015-02-11 12:18:58 UTC) #3

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/error_screen.cc (right):

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:80: LOG(WARNING) <<
"Network error screen message is shown";
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Is that a good logging level?

Just leaving as is. INFO level will be filtrated.

Now that captive portal (incl. overall network info) logging has been disabled
this may be useful to investigate any network related issues. Otherwise we don't
have any signals in logs on what's happening in UI.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:93:
PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN);
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Is it always a "Login Screen" strategy?

Yes, that's name may be a bit too specific but basically it is either
STRATEGY_ID_ERROR_SCREEN - small intervals between network checks
STRATEGY_ID_LOGIN_SCREEN - reasonable intervals between network checks
STRATEGY_ID_SESSION - increased intervals since there're other ways to
troubleshoot such errors when tabbed browser is available.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:100:
HandleConfigureCerts();
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Let's agree on a naming scheme here.
> I'd prefer OnSomeEvent rather than HandleSomeEvent.

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:113:
NetworkErrorModel::OnContextKeyUpdated(key);
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Why do we have fallback here and don't have similar fallback in OnUserAction?

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:196:
ProfileManager::GetActiveUserProfile(), NULL, native_window);
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Why the "Active user"?

In this case ActiveUserProfile() will always return signin profile, changed.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:218:
LoginDisplayHostImpl::default_host()->Finalize();
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> What if this will happen inside user session (something like adding user in
> multiprofile?).

There're no plans to support this use case since it is only used for Kiosk apps
launch use case.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/error_screen.cc:223:
base::Bind(&ErrorScreen::StartGuestSessionAfterOwnershipCheck,
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Same question here.

Not possible since this is very specific case (reflected by the name):
* out-of-box is not finished
* launching guest session after being approved by forced enrollment checks.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_error_model.cc (right):

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_error_model.cc:52: }
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Why is not it purely  abstract?
> I mean, I believe Model should implement only GetName method (as all
> implementations need that) and declare methods that would be called from
handler
> (aka View).

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/update_screen_browsertest.cc (right):

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_browsertest.cc:249: //      
       SetErrorState(NetworkError::ERROR_STATE_PORTAL,
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Todo?

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_browsertest.cc:297: //
EXPECT_CALL(*mock_error_screen_actor_, FixCaptivePortal())
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> Todo?

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_browsertest.cc:314: //      
       SetErrorState(NetworkError::ERROR_STATE_PROXY, std::string()))
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> //TODO?

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_browsertest.cc:333: //
EXPECT_CALL(*mock_error_screen_actor_,
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> //TODO?

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_browsertest.cc:345: //      
       SetErrorState(NetworkError::ERROR_STATE_OFFLINE,
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> //TODO?

Done.

https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/update_screen_browsertest.cc:371: //      
       std::string()))
On 2015/02/10 17:03:26, Denis Kuznetsov wrote:
> //TODO?

Done.

Denis Kuznetsov (DE-MUC)

looks good so far https://chromiumcodereview.appspot.com/872633008/diff/100001/chrome/browser/chromeos/login/screens/error_screen.cc File chrome/browser/chromeos/login/screens/error_screen.cc (right): https://chromiumcodereview.appspot.com/872633008/diff/100001/chrome/browser/chromeos/login/screens/error_screen.cc#newcode113 chrome/browser/chromeos/login/screens/error_screen.cc:113: NetworkErrorModel::OnContextKeyUpdated(key); On 2015/02/11 12:18:57, Nikita ...

5 years, 10 months ago (2015-02-11 16:16:25 UTC) #4

Nikita (slow)

Please take a look. https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos/login/screens/error_screen.cc File chrome/browser/chromeos/login/screens/error_screen.cc (right): https://codereview.chromium.org/872633008/diff/100001/chrome/browser/chromeos/login/screens/error_screen.cc#newcode113 chrome/browser/chromeos/login/screens/error_screen.cc:113: NetworkErrorModel::OnContextKeyUpdated(key); On 2015/02/11 16:16:25, Denis ...

5 years, 10 months ago (2015-02-12 13:23:31 UTC) #5

Denis Kuznetsov (DE-MUC)

lgtm, see comment https://chromiumcodereview.appspot.com/872633008/diff/180001/chrome/browser/resources/chromeos/login/screen_error_message.js File chrome/browser/resources/chromeos/login/screen_error_message.js (right): https://chromiumcodereview.appspot.com/872633008/diff/180001/chrome/browser/resources/chromeos/login/screen_error_message.js#newcode413 chrome/browser/resources/chromeos/login/screen_error_message.js:413: setNetwork: function(network) { Why do we ...

5 years, 10 months ago (2015-02-12 16:14:12 UTC) #6

Nikita (slow)

https://chromiumcodereview.appspot.com/872633008/diff/180001/chrome/browser/resources/chromeos/login/screen_error_message.js File chrome/browser/resources/chromeos/login/screen_error_message.js (right): https://chromiumcodereview.appspot.com/872633008/diff/180001/chrome/browser/resources/chromeos/login/screen_error_message.js#newcode413 chrome/browser/resources/chromeos/login/screen_error_message.js:413: setNetwork: function(network) { On 2015/02/12 16:14:12, Denis Kuznetsov wrote: ...

5 years, 10 months ago (2015-02-13 12:17:56 UTC) #8

commit-bot: I haz the power

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

5 years, 10 months ago (2015-02-13 12:19:28 UTC) #11

commit-bot: I haz the power

Patchset 11 (id:??) landed as https://crrev.com/92701406a5811e0f53c06585be71b6d167d6aefb Cr-Commit-Position: refs/heads/master@{#316209}

5 years, 10 months ago (2015-02-13 13:13:11 UTC) #13

Dmitry Polukhin

5 years, 9 months ago (2015-03-05 09:56:54 UTC) #15

Message was sent while issue was closed.

https://codereview.chromium.org/872633008/diff/200001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://codereview.chromium.org/872633008/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:48: void
ErrorScreenHandler::Hide() {
It seems that ErrorScreen is not hiding on restoring network connectivity
because new Hide method does nothing.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages