Issue 1028243007: Call Device.SetCarrier when the ONC Carrier property is specified.

Issue 1028243007: Call Device.SetCarrier when the ONC Carrier property is specified. (Closed)

Created:
5 years, 9 months ago by stevenjb

Modified:
5 years, 8 months ago

Reviewers:
asargent_no_longer_on_chrome, pneubeck (no reviews)

CC:
chromium-reviews, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org, extensions-reviews_chromium.org

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

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Call Device.SetCarrier when the ONC Carrier property is specified. BUG=464036 For minor change to .idl: TBR=asargent@chromium.org Committed: https://crrev.com/d812b831fcee2b5eceffd9ecc62d37402874470c Cr-Commit-Position: refs/heads/master@{#322951}

Patch Set 6 : Fix components_unittests #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+139 lines, -42 lines)			Patch
M	chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc	View	1 2	11 chunks	+21 lines, -15 lines	0 comments	Download
M	chrome/test/data/extensions/api_test/networking_private/chromeos/test.js	View	1 2 3 4	3 chunks	+33 lines, -4 lines	0 comments	Download
M	chromeos/dbus/fake_shill_device_client.h	View	1 2	1 chunk	+6 lines, -0 lines	0 comments	Download
M	chromeos/dbus/fake_shill_device_client.cc	View	1 2	4 chunks	+27 lines, -10 lines	0 comments	Download
M	chromeos/network/managed_network_configuration_handler.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chromeos/network/managed_network_configuration_handler_impl.h	View		2 chunks	+7 lines, -1 line	0 comments	Download
M	chromeos/network/managed_network_configuration_handler_impl.cc	View	1 2	2 chunks	+28 lines, -1 line	0 comments	Download
M	chromeos/network/mock_managed_network_configuration_handler.h	View		1 chunk	+5 lines, -6 lines	0 comments	Download
M	chromeos/network/onc/onc_signature.cc	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chromeos/network/onc/onc_translation_tables.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	components/onc/onc_constants.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/onc/onc_constants.cc	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	extensions/common/api/networking_private.idl	View	1 2 4	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 16 (7 generated)

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

stevenjb

Simplified solution for https://codereview.chromium.org/978923003/

5 years, 9 months ago (2015-03-25 21:11:02 UTC) #2

pneubeck (no reviews)

lgtm with comments https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extensions/api_test/networking_private/chromeos/test.js File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js (right): https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extensions/api_test/networking_private/chromeos/test.js#newcode429 chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:429: // Make sure we test ? ...

5 years, 9 months ago (2015-03-27 18:26:46 UTC) #3

stevenjb

https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extensions/api_test/networking_private/chromeos/test.js File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js (right): https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extensions/api_test/networking_private/chromeos/test.js#newcode429 chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:429: // Make sure we test On 2015/03/27 18:26:46, pneubeck ...

5 years, 9 months ago (2015-03-27 23:18:05 UTC) #4

https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:429: //
Make sure we test
On 2015/03/27 18:26:46, pneubeck wrote:
> ?

Oops. Fixed.

https://codereview.chromium.org/1028243007/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:435:
assertFalse(result.Cellular && result.Cellular.Carrier == newCarrier);
On 2015/03/27 18:26:46, pneubeck wrote:
> assertTrue(result.Cellular && result.Cellular.Carrier != newCarrier) ?
> otherwise you accept a !result.Cellular
> 
> maybe complete it to:
> assertTrue(result.Cellular);
> assertTrue(result.Cellular.Carrier != newCarrier);
> assertTrue(result.Priority != 1);
> 
> (note that you usually tried to use
>   'abc' in XY
> and XY['abc']
> )

We don't actually care if Cellular exists, we just need to ensure that it isn't
initially 'new_carrier', otherwise we wouldn't actually be testing whether or
not it was set. I inverted the logic, hopefully that is more readable.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_device_client.cc (right):

https://codereview.chromium.org/1028243007/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_device_client.cc:40: FROM_HERE,
base::Bind(error_callback, error, "Failed"));
On 2015/03/27 18:26:46, pneubeck wrote:
> I don't feel particularly confident in Bind + const char*
> Can you keep the argument wrapped into a std::string?
I'm pretty sure it will get converted to a string, but went ahead and made it a
file scoped const.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_device_client.h (right):

https://codereview.chromium.org/1028243007/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_device_client.h:126: // SetProperty helper method.
On 2015/03/27 18:26:46, pneubeck wrote:
> The function name doesn't really explain what's this for nor is the comment.
> What means 'Always'?
> 
> Looking at the implementation, maybe call it ...Internal? or
Unconditionally...

In english, 'always' and 'unconditionally' are pretty much synonymous :P
Renamed to SetPropertyInternal.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:308: // 'Carrier'
needs to be handled specially if set.
On 2015/03/27 18:26:46, pneubeck wrote:
> BIG NOTE:
> you do this now only for SetProperties but not for the initial CreateNetwork
> 
> btw. in the current form, you can also considering moving it to NCH, which
might
> catch more uses and might be cleaner.

I prefer keeping this as an ONC special case instead of a Shill special case.

Yeah, I'm not sure what to do about createConfiguration. It doesn't make sense
to have CreateConfiguration fail if SetCarrier fails since the configuration
will be created, but I don't like not providing feedback either. For now I added
documentation to the property in networking_private.idl specifying that it can
be specified in setProperties but not createConfiguration.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:309:
base::DictionaryValue* cellular;
On 2015/03/27 18:26:46, pneubeck wrote:
> pls initialize to nullptr

We always disagree on this. It is really not necessary checking the result of
Value::Get*. I'll set it as always because it's not worthy of discussion.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:316:
state->device_path(), carrier,
On 2015/03/27 18:26:46, pneubeck wrote:
> do you have to check whether device_path is valid?

device_path must always be valid for NetworkState.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:318:
&ManagedNetworkConfigurationHandlerImpl::SetShillProperties,
On 2015/03/27 18:26:46, pneubeck wrote:
> We really need a helper to collect callbacks of several >>parallel<<
callbacks.
> These callback sequences are a pain ...

Agreed.

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/1028243007/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_signature.cc:267: {::onc::cellular::kAutoConnect,
&kBoolSignature},
On 2015/03/27 18:26:46, pneubeck wrote:
> please also fix all usages, e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/o...
> 
> (probably the only one)

Done.

commit-bot: I haz the power

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

5 years, 8 months ago (2015-03-30 22:05:22 UTC) #8

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/22525)

5 years, 8 months ago (2015-03-30 22:58:55 UTC) #11

commit-bot: I haz the power

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

5 years, 8 months ago (2015-03-30 23:12:57 UTC) #14

commit-bot: I haz the power

5 years, 8 months ago (2015-03-31 02:09:36 UTC) #16

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/d812b831fcee2b5eceffd9ecc62d37402874470c
Cr-Commit-Position: refs/heads/master@{#322951}

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