Issue 22264004: Remove ConnectivityStateHelper

Issue 22264004: Remove ConnectivityStateHelper (Closed)

Created:
7 years, 4 months ago by gauravsh

Modified:
7 years, 4 months ago

Reviewers:
stevenjb, Nikita (slow), ygorshenin1

CC:
chromium-reviews, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, nkostylev+watch_chromium.org, gspencer+watch_chromium.org, gauravsh+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Remove ConnectivityStateHelper Instead, directly use NetworkHandler (which is the default). Also, remove the escape-hatch flag (to use the old NetworkLibrary path). BUG=245756 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216637

Patch Set 1 : WIP: NetworkScreen browser tests broken #

Patch Set 2 : Remove more unnecessary files #

Patch Set 3 : refactor NetworkScreen browser tests #

Patch Set 4 : rebase #

Total comments: 10

Patch Set 5 : address stevenjb comments #

Total comments: 3

Patch Set 6 : fix clang errors + fix nits #

Total comments: 12

Patch Set 7 : remove redundant IsConnected() check #

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+220 lines, -767 lines)			Patch
M	chrome/browser/chromeos/chrome_browser_main_chromeos.cc	View	1 2 3	6 chunks	+4 lines, -26 lines	0 comments	Download
M	chrome/browser/chromeos/extensions/file_manager/event_router.h	View	1 2 3 4 5	3 chunks	+9 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/extensions/file_manager/event_router.cc	View	1 2 3 4 5	5 chunks	+10 lines, -8 lines	0 comments	Download
M	chrome/browser/chromeos/login/auth_prewarmer.h	View	1 2 3 4 5	3 chunks	+6 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/login/auth_prewarmer.cc	View	1 2 3 4 5	5 chunks	+13 lines, -6 lines	0 comments	Download
M	chrome/browser/chromeos/login/captive_portal_view.cc	View		2 chunks	+7 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/login/existing_user_controller.h	View	1 2	2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/existing_user_controller.cc	View	1 2 3	5 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/browser/chromeos/login/helper.h	View	1 2	2 chunks	+23 lines, -7 lines	0 comments	Download
M	chrome/browser/chromeos/login/helper.cc	View	1 2 3 4 5	3 chunks	+44 lines, -26 lines	0 comments	Download
M	chrome/browser/chromeos/login/login_utils_browsertest.cc	View		4 chunks	+0 lines, -7 lines	0 comments	Download
M	chrome/browser/chromeos/login/oauth2_login_verifier.cc	View	1 2 3 4	2 chunks	+8 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/login/oauth2_token_fetcher.cc	View	1 2 3 4 5 6	3 chunks	+12 lines, -5 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/network_screen.h	View	1 2 3 4 5	4 chunks	+15 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/network_screen.cc	View	1 2 3 4 5	9 chunks	+22 lines, -12 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/network_screen_browsertest.cc	View	1 2	6 chunks	+28 lines, -140 lines	0 comments	Download
M	chrome/browser/chromeos/login/webui_screen_locker.h	View	1 2 3 4	2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/webui_screen_locker.cc	View	1 2	2 chunks	+2 lines, -1 line	0 comments	Download
D	chrome/browser/chromeos/net/connectivity_state_helper.h	View		1 chunk	+0 lines, -72 lines	0 comments	Download
M	chrome/browser/chromeos/net/connectivity_state_helper.cc	View		1 chunk	+0 lines, -287 lines	0 comments	Download
D	chrome/browser/chromeos/net/connectivity_state_helper_observer.h	View		1 chunk	+0 lines, -24 lines	0 comments	Download
D	chrome/browser/chromeos/net/cros_network_change_notifier_factory.h	View	1	1 chunk	+0 lines, -32 lines	0 comments	Download
D	chrome/browser/chromeos/net/cros_network_change_notifier_factory.cc	View	1	1 chunk	+0 lines, -28 lines	0 comments	Download
D	chrome/browser/chromeos/net/mock_connectivity_state_helper.h	View		1 chunk	+0 lines, -37 lines	0 comments	Download
D	chrome/browser/chromeos/net/mock_connectivity_state_helper.cc	View		1 chunk	+0 lines, -12 lines	0 comments	Download
M	chrome/chrome_browser_chromeos.gypi	View	1 2 3	1 chunk	+0 lines, -7 lines	0 comments	Download
M	chrome/chrome_tests_unit.gypi	View	1 2 3	1 chunk	+0 lines, -2 lines	0 comments	Download
M	chromeos/chromeos_switches.h	View	1 2 3	1 chunk	+0 lines, -1 line	0 comments	Download
M	chromeos/chromeos_switches.cc	View	1 2 3	1 chunk	+0 lines, -4 lines	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

gauravsh

Most of this should be straightforward - the CL replaces ConnectivityStateHelper calls with the equivalent ...

7 years, 4 months ago (2013-08-06 22:01:11 UTC) #1

stevenjb

https://codereview.chromium.org/22264004/diff/13001/chrome/browser/chromeos/login/helper.cc File chrome/browser/chromeos/login/helper.cc (right): https://codereview.chromium.org/22264004/diff/13001/chrome/browser/chromeos/login/helper.cc#newcode58 chrome/browser/chromeos/login/helper.cc:58: return UTF8ToUTF16(network->name()); I believe the above 4 could be ...

7 years, 4 months ago (2013-08-07 01:43:26 UTC) #2

gauravsh

7 years, 4 months ago (2013-08-07 17:28:19 UTC) #3

stevenjb

lgtm https://codereview.chromium.org/22264004/diff/22001/chrome/browser/chromeos/login/helper.cc File chrome/browser/chromeos/login/helper.cc (right): https://codereview.chromium.org/22264004/diff/22001/chrome/browser/chromeos/login/helper.cc#newcode52 chrome/browser/chromeos/login/helper.cc:52: NetworkStateHandler::kMatchTypeNonVirtual); move this below the first if https://codereview.chromium.org/22264004/diff/22001/chrome/browser/chromeos/login/helper.cc#newcode59 ...

7 years, 4 months ago (2013-08-07 19:10:19 UTC) #4

gauravsh

nkostylev/ygorshenin: ping for network_screen OWNERS? I'd like to get this in before the branch point.

7 years, 4 months ago (2013-08-07 22:14:19 UTC) #5

Nikita (slow)

lgtm if these comments are addressed. https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_login_verifier.cc File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right): https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_login_verifier.cc#newcode59 chrome/browser/chromeos/login/oauth2_login_verifier.cc:59: default_network->connection_state() == flimflam::kStatePortal) ...

7 years, 4 months ago (2013-08-08 10:47:28 UTC) #7

gauravsh

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_login_verifier.cc File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right): https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_login_verifier.cc#newcode59 chrome/browser/chromeos/login/oauth2_login_verifier.cc:59: default_network->connection_state() == flimflam::kStatePortal) { On 2013/08/08 10:47:28, Nikita Kostylev ...

7 years, 4 months ago (2013-08-08 16:59:20 UTC) #8

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:59:
default_network->connection_state() == flimflam::kStatePortal) {
On 2013/08/08 10:47:28, Nikita Kostylev wrote:
> Previous implementation was:
>  173 bool ConnectivityStateHelperImpl::DefaultNetworkOnline() {	
>  174   const NetworkState* network =
network_state_handler_->DefaultNetwork();	
>  175   if (!network)	
>  176     return false;	
>  177   if (!network->IsConnectedState())	
>  178     return false;	
>  179   if (network->connection_state() == flimflam::kStatePortal)	
>  180     return false;	
>  181   return true;	
>  182 }
> 
> Are you missing 
> 
> (!network->IsConnectedState())	check here?

The check was redundant. A non-NULL DefaultNetwork() is always in connected
state.

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:50:
default_network->connection_state() == flimflam::kStatePortal) {
On 2013/08/08 10:47:28, Nikita Kostylev wrote:
> Same comment about (!network->IsConnectedState()).
> 
> See also lines 71..73.
> 
> I would prefer to have the same DefaultNetworkOnline() which will cover
captive
> portal case as well.

See my previous comment. It was a redundant check since a DefaultNetwork is
always in connected state.

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/screens/network_screen_browsertest.cc (left):

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:127:
IN_PROC_BROWSER_TEST_F(NetworkScreenTest, Ethernet) {
On 2013/08/08 10:47:28, Nikita Kostylev wrote:
> nit: How are these different connection types are tested now?

tl;dr: The previous tests were redundant, effectively testing the same thing.

The previous version of the test was overspecifying the mocks and effectively
testing the implementation of IsConnected/IsConnecting and GetCurrentNetworkName
in helper.cc Those are trivially correct.

The actual NetworkScreen logic only cares about what's returned by IsConnected()
and IsConnecting() calls, which is what the new test exercises via the mock
object.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gauravsh@chromium.org/22264004/11006

7 years, 4 months ago (2013-08-08 17:26:56 UTC) #9

Nikita (slow)

7 years, 4 months ago (2013-08-08 17:50:01 UTC) #10

Nikita (slow)

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_token_fetcher.cc File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right): https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_token_fetcher.cc#newcode50 chrome/browser/chromeos/login/oauth2_token_fetcher.cc:50: default_network->connection_state() == flimflam::kStatePortal) { On 2013/08/08 16:59:20, gauravsh wrote: ...

7 years, 4 months ago (2013-08-08 17:50:42 UTC) #11

gauravsh

Discussed with nkostylev over IM. Replied to the comments here as well. https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/login/oauth2_login_verifier.cc File chrome/browser/chromeos/login/oauth2_login_verifier.cc ...

7 years, 4 months ago (2013-08-08 18:02:41 UTC) #12

Discussed with nkostylev over IM. Replied to the comments here as well.

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:59:
default_network->connection_state() == flimflam::kStatePortal) {
On 2013/08/08 17:50:01, Nikita Kostylev wrote:
> On 2013/08/08 16:59:20, gauravsh wrote:
> > On 2013/08/08 10:47:28, Nikita Kostylev wrote:
> > > Previous implementation was:
> > >  173 bool ConnectivityStateHelperImpl::DefaultNetworkOnline() {	
> > >  174   const NetworkState* network =
> > network_state_handler_->DefaultNetwork();	
> > >  175   if (!network)	
> > >  176     return false;	
> > >  177   if (!network->IsConnectedState())	
> > >  178     return false;	
> > >  179   if (network->connection_state() == flimflam::kStatePortal)	
> > >  180     return false;	
> > >  181   return true;	
> > >  182 }
> > > 
> > > Are you missing 
> > > 
> > > (!network->IsConnectedState())	check here?
> > 
> > The check was redundant. A non-NULL DefaultNetwork() is always in connected
> > state.
> 
> This block should be executed in 3 cases:
> 1. Network is disconnected (default_network == NULL)
> 2. Network is in connecting state (how is this covered?)
> 3. Captive portal (connection_state() == flimflam::kStatePortal)

DefaultNetwork() already does the IsConnected() check, so Case 2 is covered.

https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/n...

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:50:
default_network->connection_state() == flimflam::kStatePortal) {
On 2013/08/08 17:50:43, Nikita Kostylev wrote:
> On 2013/08/08 16:59:20, gauravsh wrote:
> > On 2013/08/08 10:47:28, Nikita Kostylev wrote:
> > > Same comment about (!network->IsConnectedState()).
> > > 
> > > See also lines 71..73.
> > > 
> > > I would prefer to have the same DefaultNetworkOnline() which will cover
> > captive
> > > portal case as well.
> > 
> > See my previous comment. It was a redundant check since a DefaultNetwork is
> > always in connected state.
> 
> Why check in lines 71..73 is different then?

Oops, yeah, it should be the same there as well. I will upload a new PS to fix
that.

stevenjb

7 years, 4 months ago (2013-08-08 18:10:23 UTC) #13

commit-bot: I haz the power

Failed to trigger a try job on win_x64_rel HTTP Error 400: Bad Request

7 years, 4 months ago (2013-08-08 19:39:54 UTC) #14

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gauravsh@chromium.org/22264004/45002

7 years, 4 months ago (2013-08-08 19:44:23 UTC) #15

commit-bot: I haz the power

Step "update" is always a major failure. Look at the try server FAQ for more ...

7 years, 4 months ago (2013-08-08 20:28:11 UTC) #16

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gauravsh@chromium.org/22264004/45002

7 years, 4 months ago (2013-08-08 20:40:17 UTC) #17

Nikita (slow)

7 years, 4 months ago (2013-08-09 09:43:59 UTC) #19

Message was sent while issue was closed.

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/22264004/diff/11006/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:59:
default_network->connection_state() == flimflam::kStatePortal) {
On 2013/08/08 18:10:23, stevenjb (chromium) wrote:
> On 2013/08/08 17:50:01, Nikita Kostylev wrote:
> 
> > 2. Network is in connecting state (how is this covered?)
> 
> default_network is still NULL.
> 
> With the new NetworkStateHandlers, we switched to using Shill's idea of
"Default
> Network" - it is the network that is handling traffic (and thus must be
> connected), and there is only one. This eliminates a lot of redundant
> "IsConnected" checks in the calling code.
> 
> This is different from the NetworkLibrary concept of "Active Network" which
> might be connecting. For that we now use "FirstNetworkByType" (with
convenience
> methods of "ConnectedNetworkByType" and "ConnectingNetworkByType" which just
> check the state of the first network of that type). "First" is shorthand for
> "Highest Priority" - Shill orders networks by priority. "Type" can be
> kMatchTypeDefault which indicates the highest priority network of any type.
> DefaultNetwork() is shorthand for ConnectedNetworkByType(kMatchTypeDefault).

Thanks for explanation.

Expand Messages | Collapse Messages