Clean up NetworkState members

Clean up NetworkState members

This includes updating the (already asynchronous) calls in MobileActivator and MobileSetupUI to retrieve the complete set of service properties so that we do not need to cache some infrequently used properties.

BUG=252553
For test files:
TBR=armansito@chromium.org, pneubeck@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221785

[stevenjb, 2013-09-03 23:23:06]

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:355: if
(!cellular->payment_url().empty())
This check shouldn't be necessary; MobileSetupUI should handle that case after
requesting properties.

[armansito, 2013-09-03 23:52:06]

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:355: if
(!cellular->payment_url().empty())
On 2013/09/03 23:23:07, stevenjb (chromium) wrote:
> This check shouldn't be necessary; MobileSetupUI should handle that case after
> requesting properties.

I think the main reason for doing this check here is to prevent an empty
activation dialog from temporarily popping up. Is this something that could be
handled in MobileSetupUI without displaying the dialog?

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:282: const
base::DictionaryValue& properties) {
This method is really meant for the initial property load at the start of the
state machine rather than a generic success callback for GetProperties, so could
you rename this method to reflect that? Maybe something like,
"InitialPropertiesReceived".

[stevenjb, 2013-09-04 00:04:50]

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:355: if
(!cellular->payment_url().empty())
On 2013/09/03 23:52:06, armansito wrote:
> On 2013/09/03 23:23:07, stevenjb (chromium) wrote:
> > This check shouldn't be necessary; MobileSetupUI should handle that case
after
> > requesting properties.
> 
> I think the main reason for doing this check here is to prevent an empty
> activation dialog from temporarily popping up. Is this something that could be
> handled in MobileSetupUI without displaying the dialog?

Is this a scenario that would ever actually happen outside of an error
condition? I see that MobileSetupUI checks for either payment_url or usage_url,
so this check seemed somewhat arbitrary and inconsistent. If this produces an
actual bug then we should fix it in MobileSetupUI instead of here.

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:282: const
base::DictionaryValue& properties) {
On 2013/09/03 23:52:06, armansito wrote:
> This method is really meant for the initial property load at the start of the
> state machine rather than a generic success callback for GetProperties, so
could
> you rename this method to reflect that? Maybe something like,
> "InitialPropertiesReceived".

Actually it's called for ContinueActivation(), so renamed
GetPropertiesAndContinueActivation.

[gauravsh, 2013-09-04 00:06:52]

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:285: if
(!properties.GetStringWithoutPathExpansion(
It shouldn't happen, but you should check for service_path == service_path_
here.

[gauravsh, 2013-09-04 00:09:48]

Also, I'd recommend testing the activation flow (3g and 4g lte) with this change
manually as well. The test coverage isn't very good, and the code has some
subtle gotchas (e.g. service path changes).

[armansito, 2013-09-04 00:18:14]

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:355: if
(!cellular->payment_url().empty())
On 2013/09/04 00:04:50, stevenjb (chromium) wrote:
> On 2013/09/03 23:52:06, armansito wrote:
> > On 2013/09/03 23:23:07, stevenjb (chromium) wrote:
> > > This check shouldn't be necessary; MobileSetupUI should handle that case
> after
> > > requesting properties.
> > 
> > I think the main reason for doing this check here is to prevent an empty
> > activation dialog from temporarily popping up. Is this something that could
be
> > handled in MobileSetupUI without displaying the dialog?
> 
> Is this a scenario that would ever actually happen outside of an error
> condition? I see that MobileSetupUI checks for either payment_url or
usage_url,
> so this check seemed somewhat arbitrary and inconsistent. If this produces an
> actual bug then we should fix it in MobileSetupUI instead of here.

Yes, it is actually possible for shill not to show the payment URL on occasion.
This is rare, but we should make sure that this is handled correctly by
MobileSetupDialog to prevent any regressions, as such an issue has been seen in
the past.

We can address that for 31 if the test seem runs into anything.

[stevenjb, 2013-09-04 01:03:27]

On 2013/09/04 00:09:48, gauravsh wrote:
> Also, I'd recommend testing the activation flow (3g and 4g lte) with this
change
> manually as well. The test coverage isn't very good, and the code has some
> subtle gotchas (e.g. service path changes).

I'm hoping to get this in early enough so that we (by which I mostly mean Arman
:) ) can test this as thoroughly as possible. I can test with LTE but that's
about it.

[stevenjb, 2013-09-04 01:04:10]

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:355: if
(!cellular->payment_url().empty())
On 2013/09/04 00:18:14, armansito wrote:
> On 2013/09/04 00:04:50, stevenjb (chromium) wrote:
> > On 2013/09/03 23:52:06, armansito wrote:
> > > On 2013/09/03 23:23:07, stevenjb (chromium) wrote:
> > > > This check shouldn't be necessary; MobileSetupUI should handle that case
> > after
> > > > requesting properties.
> > > 
> > > I think the main reason for doing this check here is to prevent an empty
> > > activation dialog from temporarily popping up. Is this something that
could
> be
> > > handled in MobileSetupUI without displaying the dialog?
> > 
> > Is this a scenario that would ever actually happen outside of an error
> > condition? I see that MobileSetupUI checks for either payment_url or
> usage_url,
> > so this check seemed somewhat arbitrary and inconsistent. If this produces
an
> > actual bug then we should fix it in MobileSetupUI instead of here.
> 
> Yes, it is actually possible for shill not to show the payment URL on
occasion.
> This is rare, but we should make sure that this is handled correctly by
> MobileSetupDialog to prevent any regressions, as such an issue has been seen
in
> the past.
> 
> We can address that for 31 if the test seem runs into anything.

I'm not really sure that I understand. It doesn't seem like having a button
click do nothing (which is what would happen if we return here) is any better
than showing an empty dialog, or briefly showing and closing the dialog.

However, if we would prefer that, we could probably have
MobileSetupDialogDelegate::ShowDialog call GetProperties() and check
payment_url_ before calling chrome::ShowWebDialog? I really would like to avoid
caching this value for this one scenario.

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/23441025/diff/4001/chrome/browser/chromeos/mo...
chrome/browser/chromeos/mobile/mobile_activator.cc:285: if
(!properties.GetStringWithoutPathExpansion(
On 2013/09/04 00:06:52, gauravsh wrote:
> It shouldn't happen, but you should check for service_path == service_path_
> here.
Done.

[armansito, 2013-09-04 01:35:31]

Just one nit. I would like to test the activation portion before you submit
this, so I'll approve once I confirm that activation works with this patch.

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23441025/diff/4001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:355: if
(!cellular->payment_url().empty())
On 2013/09/04 01:04:11, stevenjb (chromium) wrote:
> On 2013/09/04 00:18:14, armansito wrote:
> > On 2013/09/04 00:04:50, stevenjb (chromium) wrote:
> > > On 2013/09/03 23:52:06, armansito wrote:
> > > > On 2013/09/03 23:23:07, stevenjb (chromium) wrote:
> > > > > This check shouldn't be necessary; MobileSetupUI should handle that
case
> > > after
> > > > > requesting properties.
> > > > 
> > > > I think the main reason for doing this check here is to prevent an empty
> > > > activation dialog from temporarily popping up. Is this something that
> could
> > be
> > > > handled in MobileSetupUI without displaying the dialog?
> > > 
> > > Is this a scenario that would ever actually happen outside of an error
> > > condition? I see that MobileSetupUI checks for either payment_url or
> > usage_url,
> > > so this check seemed somewhat arbitrary and inconsistent. If this produces
> an
> > > actual bug then we should fix it in MobileSetupUI instead of here.
> > 
> > Yes, it is actually possible for shill not to show the payment URL on
> occasion.
> > This is rare, but we should make sure that this is handled correctly by
> > MobileSetupDialog to prevent any regressions, as such an issue has been seen
> in
> > the past.
> > 
> > We can address that for 31 if the test seem runs into anything.
> 
> I'm not really sure that I understand. It doesn't seem like having a button
> click do nothing (which is what would happen if we return here) is any better
> than showing an empty dialog, or briefly showing and closing the dialog.
> 
> However, if we would prefer that, we could probably have
> MobileSetupDialogDelegate::ShowDialog call GetProperties() and check
> payment_url_ before calling chrome::ShowWebDialog? I really would like to
avoid
> caching this value for this one scenario.

Yes, I agree that this shouldn't be cached. We can talk about a work around for
the case when the payment URL hasn't loaded yet, but I think the current change
is OK as far as this CL is concerned.

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:284: return;  // Edge case;
abort.
nit: Please add a log message here.

[armansito, 2013-09-04 02:28:54]

lgtm for mobile activator. Please address the one nit I pointed out.

[pneubeck (no reviews), 2013-09-04 16:32:55]

lgtm

I looked at all but mobile_*
lgtm, except one comment and one optional nit.

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/network.html (right):

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/resources/...
chrome/browser/resources/chromeos/network.html:34: <td>Path</td>
nit: could you add GUID ?

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1572:
flimflam::kPriorityProperty, &auto_connect);
priority -> AutoConnect

[stevenjb, 2013-09-04 18:47:52]

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:284: return;  // Edge case;
abort.
On 2013/09/04 01:35:31, armansito wrote:
> nit: Please add a log message here.

Done.

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/network.html (right):

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/resources/...
chrome/browser/resources/chromeos/network.html:34: <td>Path</td>
On 2013/09/04 16:32:55, pneubeck wrote:
> nit: could you add GUID ?

Done.

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/23441025/diff/19001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1572:
flimflam::kPriorityProperty, &auto_connect);
On 2013/09/04 16:32:55, pneubeck wrote:
> priority -> AutoConnect
DOH. Thanks. Done.

[commit-bot: I haz the power, 2013-09-05 16:27:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23441025/2002

[commit-bot: I haz the power, 2013-09-05 16:42:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-05 17:06:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23441025/2002

[commit-bot: I haz the power, 2013-09-05 17:11:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23441025/2002

[commit-bot: I haz the power, 2013-09-05 17:14:06]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[stevenjb, 2013-09-06 21:31:23]

Committed patchset #12 manually as r221785 (presubmit successful).