Bug fix to toggle Network scanning indicator for WiFi

Bug fix to toggle Network scanning indicator for WiFi

BUG=617831
COMPONENT=Chrome OS UI, Network

Committed: https://crrev.com/d35fb84631dfe8b92e69418dc270af487a0701af
Cr-Commit-Position: refs/heads/master@{#416744}

[malaykeshav, 2016-09-06 19:07:00]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 19:07:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2016-09-06 19:07:34]

malaykeshav@chromium.org changed reviewers:
+ jdufault@chromium.org, stevenjb@chromium.org

[malaykeshav, 2016-09-06 19:07:35]

PTAL.
Minor bug fix to fix network scanning indicator.

[commit-bot: I haz the power, 2016-09-06 20:00:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 20:00:41]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-09-06 20:13:52]

Thanks for looking into this.

https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:577: if
(scanning && list_type_ != LIST_TYPE_VPN) {
This logic somehow got convoluted along the way. The list_type test is redundant
with line 569. Also, the test for IsSystemTrayMenuMaterial() on line 575 should
be moved after line 576, otherwise someone might use wifi_scanning_ elsewhere in
the MD code without it updating correctly.

[malaykeshav, 2016-09-06 21:22:47]

Removed the redundant check for list_type_

https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:577: if
(scanning && list_type_ != LIST_TYPE_VPN) {
On 2016/09/06 at 20:13:52, stevenjb wrote:
> This logic somehow got convoluted along the way. The list_type test is
redundant with line 569. Also, the test for IsSystemTrayMenuMaterial() on line
575 should be moved after line 576, otherwise someone might use wifi_scanning_
elsewhere in the MD code without it updating correctly.

Just checked, the list_type_ test is indeed redundant. The value doesnt change
once the class object has been initialized. Removing the second check to resolve
this.

wifi_scanning_ is _only_ used to check whether the scanning state of the device
has changed(line 574). It is not being used anywhere else in the code. Should be
okay to leave it as is.

[stevenjb, 2016-09-06 21:27:17]

On 2016/09/06 21:22:47, malaykeshav wrote:
> Removed the redundant check for list_type_
> 
>
https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
> File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
> (right):
> 
>
https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
> ash/common/system/chromeos/network/network_state_list_detailed_view.cc:577: if
> (scanning && list_type_ != LIST_TYPE_VPN) {
> On 2016/09/06 at 20:13:52, stevenjb wrote:
> > This logic somehow got convoluted along the way. The list_type test is
> redundant with line 569. Also, the test for IsSystemTrayMenuMaterial() on line
> 575 should be moved after line 576, otherwise someone might use wifi_scanning_
> elsewhere in the MD code without it updating correctly.
> 
> Just checked, the list_type_ test is indeed redundant. The value doesnt change
> once the class object has been initialized. Removing the second check to
resolve
> this.
> 
> wifi_scanning_ is _only_ used to check whether the scanning state of the
device
> has changed(line 574). It is not being used anywhere else in the code. Should
be
> okay to leave it as is.

It may be OK for now, but if someone else decides to use it elsewhere in the MD
version and makes the (not unreasonable) assumption that it accurately reflects
the scanning state, we could end up with a subtle bug. I would really prefer
that we always just set it accurately.

(If we really didn't want to bother setting it we should move all of the logic
to a helper function and just not call that for MD, that would at least be more
clear).

Thanks!

[malaykeshav, 2016-09-06 21:42:43]

On 2016/09/06 at 21:27:17, stevenjb wrote:
> On 2016/09/06 21:22:47, malaykeshav wrote:
> > Removed the redundant check for list_type_
> > 
> >
https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
> > File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
> > (right):
> > 
> >
https://codereview.chromium.org/2311093004/diff/1/ash/common/system/chromeos/...
> > ash/common/system/chromeos/network/network_state_list_detailed_view.cc:577:
if
> > (scanning && list_type_ != LIST_TYPE_VPN) {
> > On 2016/09/06 at 20:13:52, stevenjb wrote:
> > > This logic somehow got convoluted along the way. The list_type test is
> > redundant with line 569. Also, the test for IsSystemTrayMenuMaterial() on
line
> > 575 should be moved after line 576, otherwise someone might use
wifi_scanning_
> > elsewhere in the MD code without it updating correctly.
> > 
> > Just checked, the list_type_ test is indeed redundant. The value doesnt
change
> > once the class object has been initialized. Removing the second check to
resolve
> > this.
> > 
> > wifi_scanning_ is _only_ used to check whether the scanning state of the
device
> > has changed(line 574). It is not being used anywhere else in the code.
Should be
> > okay to leave it as is.
> 
> It may be OK for now, but if someone else decides to use it elsewhere in the
MD version and makes the (not unreasonable) assumption that it accurately
reflects the scanning state, we could end up with a subtle bug. I would really
prefer that we always just set it accurately.

Makes sense. Although I feel it would be better to rename the variable to
something that suits its function. (The correct way to get the current
wifi_scanning state is to use the NetworkHandler singleton)
Renaming the variable to `prev_wifi_scanning_state_` should resolve all of the
invalid assumptions?

> 
> (If we really didn't want to bother setting it we should move all of the logic
to a helper function and just not call that for MD, that would at least be more
clear).
> 
> Thanks!

[stevenjb, 2016-09-06 21:46:30]

Renaming the variable makes sense. Also why don't we just move
the MaterialDesignController::IsSystemTrayMenuMaterial() to the if at line
569 and skip the GetScanningByType() call entirely.


On Tue, Sep 6, 2016 at 2:42 PM, <malaykeshav@chromium.org> wrote:

> On 2016/09/06 at 21:27:17, stevenjb wrote:
> > On 2016/09/06 21:22:47, malaykeshav wrote:
> > > Removed the redundant check for list_type_
> > >
> > >
> https://codereview.chromium.org/2311093004/diff/1/ash/
> common/system/chromeos/network/network_state_list_detailed_view.cc
> > > File ash/common/system/chromeos/network/network_state_list_
> detailed_view.cc
> > > (right):
> > >
> > >
> https://codereview.chromium.org/2311093004/diff/1/ash/
> common/system/chromeos/network/network_state_list_
> detailed_view.cc#newcode577
> > > ash/common/system/chromeos/network/network_state_list_
> detailed_view.cc:577:
> if
> > > (scanning && list_type_ != LIST_TYPE_VPN) {
> > > On 2016/09/06 at 20:13:52, stevenjb wrote:
> > > > This logic somehow got convoluted along the way. The list_type test
> is
> > > redundant with line 569. Also, the test for IsSystemTrayMenuMaterial()
> on
> line
> > > 575 should be moved after line 576, otherwise someone might use
> wifi_scanning_
> > > elsewhere in the MD code without it updating correctly.
> > >
> > > Just checked, the list_type_ test is indeed redundant. The value doesnt
> change
> > > once the class object has been initialized. Removing the second check
> to
> resolve
> > > this.
> > >
> > > wifi_scanning_ is _only_ used to check whether the scanning state of
> the
> device
> > > has changed(line 574). It is not being used anywhere else in the code.
> Should be
> > > okay to leave it as is.
> >
> > It may be OK for now, but if someone else decides to use it elsewhere in
> the
> MD version and makes the (not unreasonable) assumption that it accurately
> reflects the scanning state, we could end up with a subtle bug. I would
> really
> prefer that we always just set it accurately.
>
> Makes sense. Although I feel it would be better to rename the variable to
> something that suits its function. (The correct way to get the current
> wifi_scanning state is to use the NetworkHandler singleton)
> Renaming the variable to `prev_wifi_scanning_state_` should resolve all of
> the
> invalid assumptions?
>
> >
> > (If we really didn't want to bother setting it we should move all of the
> logic
> to a helper function and just not call that for MD, that would at least be
> more
> clear).
> >
> > Thanks!
>
>
>
> https://codereview.chromium.org/2311093004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[malaykeshav, 2016-09-06 22:02:16]

On 2016/09/06 at 21:46:30, stevenjb wrote:
> Renaming the variable makes sense. Also why don't we just move
> the MaterialDesignController::IsSystemTrayMenuMaterial() to the if at line
> 569 and skip the GetScanningByType() call entirely.
> 
> 
> On Tue, Sep 6, 2016 at 2:42 PM, <malaykeshav@chromium.org> wrote:
> 
> > On 2016/09/06 at 21:27:17, stevenjb wrote:
> > > On 2016/09/06 21:22:47, malaykeshav wrote:
> > > > Removed the redundant check for list_type_
> > > >
> > > >
> > https://codereview.chromium.org/2311093004/diff/1/ash/
> > common/system/chromeos/network/network_state_list_detailed_view.cc
> > > > File ash/common/system/chromeos/network/network_state_list_
> > detailed_view.cc
> > > > (right):
> > > >
> > > >
> > https://codereview.chromium.org/2311093004/diff/1/ash/
> > common/system/chromeos/network/network_state_list_
> > detailed_view.cc#newcode577
> > > > ash/common/system/chromeos/network/network_state_list_
> > detailed_view.cc:577:
> > if
> > > > (scanning && list_type_ != LIST_TYPE_VPN) {
> > > > On 2016/09/06 at 20:13:52, stevenjb wrote:
> > > > > This logic somehow got convoluted along the way. The list_type test
> > is
> > > > redundant with line 569. Also, the test for IsSystemTrayMenuMaterial()
> > on
> > line
> > > > 575 should be moved after line 576, otherwise someone might use
> > wifi_scanning_
> > > > elsewhere in the MD code without it updating correctly.
> > > >
> > > > Just checked, the list_type_ test is indeed redundant. The value doesnt
> > change
> > > > once the class object has been initialized. Removing the second check
> > to
> > resolve
> > > > this.
> > > >
> > > > wifi_scanning_ is _only_ used to check whether the scanning state of
> > the
> > device
> > > > has changed(line 574). It is not being used anywhere else in the code.
> > Should be
> > > > okay to leave it as is.
> > >
> > > It may be OK for now, but if someone else decides to use it elsewhere in
> > the
> > MD version and makes the (not unreasonable) assumption that it accurately
> > reflects the scanning state, we could end up with a subtle bug. I would
> > really
> > prefer that we always just set it accurately.
> >
> > Makes sense. Although I feel it would be better to rename the variable to
> > something that suits its function. (The correct way to get the current
> > wifi_scanning state is to use the NetworkHandler singleton)
> > Renaming the variable to `prev_wifi_scanning_state_` should resolve all of
> > the
> > invalid assumptions?
> >
> > >
> > > (If we really didn't want to bother setting it we should move all of the
> > logic
> > to a helper function and just not call that for MD, that would at least be
> > more
> > clear).
> > >
> > > Thanks!
> >
> >
> >
> > https://codereview.chromium.org/2311093004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Done

[malaykeshav, 2016-09-06 22:02:20]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 22:03:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-09-06 22:13:54]

Cheers. LGTM

[jdufault, 2016-09-06 22:19:19]

lgtm

https://codereview.chromium.org/2311093004/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2311093004/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:312:
prev_wifi_scanning_state_(false),
Since you're already in this file, can you initialize all of these constant
values in the header?

[commit-bot: I haz the power, 2016-09-06 22:47:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 22:47:28]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2016-09-06 22:49:06]

The CQ bit was checked by malaykeshav@chromium.org

[commit-bot: I haz the power, 2016-09-06 22:49:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 22:57:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-06 23:00:40]

Description was changed from

==========
Bug fix to toggle Network scanning indicator for WiFi

BUG=617831
COMPONENT=Chrome OS UI, Network
==========

to

==========
Bug fix to toggle Network scanning indicator for WiFi

BUG=617831
COMPONENT=Chrome OS UI, Network

Committed: https://crrev.com/d35fb84631dfe8b92e69418dc270af487a0701af
Cr-Commit-Position: refs/heads/master@{#416744}
==========

[commit-bot: I haz the power, 2016-09-06 23:00:41]

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