cros - Show progress bar in network detailed view when scanning for

cros - Show progress bar in network detailed view when scanning for
wifi networks

BUG=649799
Review-Url: https://codereview.chromium.org/2722373002
Cr-Commit-Position: refs/heads/master@{#454905}
Committed: https://chromium.googlesource.com/chromium/src/+/462cc9a6edabe3c44c90ca52a9d3452aed70ee78

[Evan Stade, 2017-03-02 02:02:11]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[tdanderson, 2017-03-02 21:10:28]

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

https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:541:
NetworkHandler::Get()->network_state_handler()->GetScanningByType(
TrayNetworkStateObserver::DeviceListChanged()

https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:545:
scanning ? IDS_ASH_STATUS_TRAY_WIFI_SCANNING_MESSAGE
Is this really the right thing to do? I think the loader is indication enough
that scanning is in progress, and for a ChromeVox user, hearing "Scanning is in
progress - Button" could be confusing; I think "Network Info - Button" is better
in both cases since it indicates the purpose of the button.

If you end up making this change to the tooltip then I think you should also
make it at the time when |info_button_md_| is instantiated.

[tdanderson, 2017-03-02 21:11:15]

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

https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:541:
NetworkHandler::Get()->network_state_handler()->GetScanningByType(
On 2017/03/02 21:10:28, tdanderson wrote:
> TrayNetworkStateObserver::DeviceListChanged()

^ Ignore this comment, sorry.

[Evan Stade, 2017-03-02 22:16:39]

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

https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:545:
scanning ? IDS_ASH_STATUS_TRAY_WIFI_SCANNING_MESSAGE
On 2017/03/02 21:10:28, tdanderson wrote:
> Is this really the right thing to do?

this is what the pre-md code used to do before we deleted it (without replacing
it with the md version till now).

> I think the loader is indication enough
> that scanning is in progress, and for a ChromeVox user, hearing "Scanning is
in
> progress - Button" could be confusing;

for the non chromevox user, I don't think it's obvious what is being scanned.
The string is "Searching for Wi-Fi networks...", not "Scanning is in progress".

For chromevox users, depending on their vision, they probably can't even see the
progress bar so they'd have no idea what's going on without this change.

> I think "Network Info - Button" is better
> in both cases since it indicates the purpose of the button.
> 
> If you end up making this change to the tooltip then I think you should also
> make it at the time when |info_button_md_| is instantiated.

I don't understand this suggestion. Don't we already set TRAY_NETWORK_INFO at
instantiation time?

[Evan Stade, 2017-03-03 00:21:35]

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

[commit-bot: I haz the power, 2017-03-03 00:22:10]

Dry run: 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, 2017-03-03 02:07:59]

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

[commit-bot: I haz the power, 2017-03-03 02:08:00]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-03-03 16:41:19]

LGTM with comment about setting the correct tooltip on the button at
instantiation time addressed.

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

https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:545:
scanning ? IDS_ASH_STATUS_TRAY_WIFI_SCANNING_MESSAGE
On 2017/03/02 22:16:39, Evan Stade wrote:
> On 2017/03/02 21:10:28, tdanderson wrote:
> > Is this really the right thing to do?
> 
> this is what the pre-md code used to do before we deleted it (without
replacing
> it with the md version till now).
> 
> > I think the loader is indication enough
> > that scanning is in progress, and for a ChromeVox user, hearing "Scanning is
> in
> > progress - Button" could be confusing;
> 
> for the non chromevox user, I don't think it's obvious what is being scanned.
> The string is "Searching for Wi-Fi networks...", not "Scanning is in
progress".
> 
> For chromevox users, depending on their vision, they probably can't even see
the
> progress bar so they'd have no idea what's going on without this change.

I mis-typed. I meant to type "Searching for Wi-Fi networks... - Button" instead
of "Scanning is in progress - Button".

The point I was trying to make is that if a ChromeVox user hears "Searching for
Wi-Fi networks... - Button" when focusing on the network info button, it doesn't
seem particularly clear that this is in fact the network info button, and that
clicking on this button will show me IP address, etc.

However I did not realize this was the pre-MD behavior for the button, so I
would be OK with this.

> 
> > I think "Network Info - Button" is better
> > in both cases since it indicates the purpose of the button.
> > 
> > If you end up making this change to the tooltip then I think you should also
> > make it at the time when |info_button_md_| is instantiated.
> 
> I don't understand this suggestion. Don't we already set TRAY_NETWORK_INFO at
> instantiation time?

Yes we do set TRAY_NETWORK_INFO at instantiation time. If however scanning is in
process at instantiation of the button, it should instead have the tooltip
IDS_..._MESSAGE instead, as you are doing here.

[Evan Stade, 2017-03-03 22:24:02]

On 2017/03/03 16:41:19, tdanderson (slow until Mar 6) wrote:
> LGTM with comment about setting the correct tooltip on the button at
> instantiation time addressed.
> 
>
https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
> File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
> (right):
> 
>
https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
> ash/common/system/chromeos/network/network_state_list_detailed_view.cc:545:
> scanning ? IDS_ASH_STATUS_TRAY_WIFI_SCANNING_MESSAGE
> On 2017/03/02 22:16:39, Evan Stade wrote:
> > On 2017/03/02 21:10:28, tdanderson wrote:
> > > Is this really the right thing to do?
> > 
> > this is what the pre-md code used to do before we deleted it (without
> replacing
> > it with the md version till now).
> > 
> > > I think the loader is indication enough
> > > that scanning is in progress, and for a ChromeVox user, hearing "Scanning
is
> > in
> > > progress - Button" could be confusing;
> > 
> > for the non chromevox user, I don't think it's obvious what is being
scanned.
> > The string is "Searching for Wi-Fi networks...", not "Scanning is in
> progress".
> > 
> > For chromevox users, depending on their vision, they probably can't even see
> the
> > progress bar so they'd have no idea what's going on without this change.
> 
> I mis-typed. I meant to type "Searching for Wi-Fi networks... - Button"
instead
> of "Scanning is in progress - Button".
> 
> The point I was trying to make is that if a ChromeVox user hears "Searching
for
> Wi-Fi networks... - Button" when focusing on the network info button, it
doesn't
> seem particularly clear that this is in fact the network info button, and that
> clicking on this button will show me IP address, etc.
> 
> However I did not realize this was the pre-MD behavior for the button, so I
> would be OK with this.
> 
> > 
> > > I think "Network Info - Button" is better
> > > in both cases since it indicates the purpose of the button.
> > > 
> > > If you end up making this change to the tooltip then I think you should
also
> > > make it at the time when |info_button_md_| is instantiated.
> > 
> > I don't understand this suggestion. Don't we already set TRAY_NETWORK_INFO
at
> > instantiation time?
> 
> Yes we do set TRAY_NETWORK_INFO at instantiation time. If however scanning is
in
> process at instantiation of the button, it should instead have the tooltip
> IDS_..._MESSAGE instead, as you are doing here.

but this function is called right after the button is created (Init first
creates stuff then calls Update).

[tdanderson, 2017-03-03 23:30:33]

On 2017/03/03 22:24:02, Evan Stade wrote:
> On 2017/03/03 16:41:19, tdanderson (slow until Mar 6) wrote:
> > LGTM with comment about setting the correct tooltip on the button at
> > instantiation time addressed.
> > 
> >
>
https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
> > File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2722373002/diff/1/ash/common/system/chromeos/...
> > ash/common/system/chromeos/network/network_state_list_detailed_view.cc:545:
> > scanning ? IDS_ASH_STATUS_TRAY_WIFI_SCANNING_MESSAGE
> > On 2017/03/02 22:16:39, Evan Stade wrote:
> > > On 2017/03/02 21:10:28, tdanderson wrote:
> > > > Is this really the right thing to do?
> > > 
> > > this is what the pre-md code used to do before we deleted it (without
> > replacing
> > > it with the md version till now).
> > > 
> > > > I think the loader is indication enough
> > > > that scanning is in progress, and for a ChromeVox user, hearing
"Scanning
> is
> > > in
> > > > progress - Button" could be confusing;
> > > 
> > > for the non chromevox user, I don't think it's obvious what is being
> scanned.
> > > The string is "Searching for Wi-Fi networks...", not "Scanning is in
> > progress".
> > > 
> > > For chromevox users, depending on their vision, they probably can't even
see
> > the
> > > progress bar so they'd have no idea what's going on without this change.
> > 
> > I mis-typed. I meant to type "Searching for Wi-Fi networks... - Button"
> instead
> > of "Scanning is in progress - Button".
> > 
> > The point I was trying to make is that if a ChromeVox user hears "Searching
> for
> > Wi-Fi networks... - Button" when focusing on the network info button, it
> doesn't
> > seem particularly clear that this is in fact the network info button, and
that
> > clicking on this button will show me IP address, etc.
> > 
> > However I did not realize this was the pre-MD behavior for the button, so I
> > would be OK with this.
> > 
> > > 
> > > > I think "Network Info - Button" is better
> > > > in both cases since it indicates the purpose of the button.
> > > > 
> > > > If you end up making this change to the tooltip then I think you should
> also
> > > > make it at the time when |info_button_md_| is instantiated.
> > > 
> > > I don't understand this suggestion. Don't we already set TRAY_NETWORK_INFO
> at
> > > instantiation time?
> > 
> > Yes we do set TRAY_NETWORK_INFO at instantiation time. If however scanning
is
> in
> > process at instantiation of the button, it should instead have the tooltip
> > IDS_..._MESSAGE instead, as you are doing here.
> 
> but this function is called right after the button is created (Init first
> creates stuff then calls Update).

Got it, thanks - missed that fact. LGTM to land.

[Evan Stade, 2017-03-06 17:51:27]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-06 17:51:59]

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, 2017-03-06 18:57:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488822687120060, "parent_rev":
"22a4567d56bc34730ebce60aa7157a8ab35e1415", "commit_rev":
"462cc9a6edabe3c44c90ca52a9d3452aed70ee78"}

[commit-bot: I haz the power, 2017-03-06 18:58:16]

Description was changed from

==========
cros - Show progress bar in network detailed view when scanning for
wifi networks

BUG=649799
==========

to

==========
cros - Show progress bar in network detailed view when scanning for
wifi networks

BUG=649799

Review-Url: https://codereview.chromium.org/2722373002
Cr-Commit-Position: refs/heads/master@{#454905}
Committed:
https://chromium.googlesource.com/chromium/src/+/462cc9a6edabe3c44c90ca52a9d3...
==========

[commit-bot: I haz the power, 2017-03-06 18:58:17]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/462cc9a6edabe3c44c90ca52a9d3...