Issue 2957043002: Add a row in the network tray to inform users to turn Bluetooth on to enable Tether.

lesliewatkins

Description was changed from ========== Add a row in the network tray to inform users ...

3 years, 5 months ago (2017-06-27 15:08:40 UTC) #1

lesliewatkins

lesliewatkins@chromium.org changed reviewers: + khorimoto@chromium.org, tdanderson@chromium.org

3 years, 5 months ago (2017-06-27 15:08:41 UTC) #2

tdanderson

Hi Leslie, please see my comments below. https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#newcode7 ash/ash_strings.grd:7: <grit base_dir="." ...

3 years, 5 months ago (2017-06-27 19:33:21 UTC) #4

Hi Leslie, please see my comments below.

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#new...
ash/ash_strings.grd:7: <grit base_dir="." latest_public_release="0"
current_release="1"
I see you are using bug 672257 to track many different changes. 
As a suggestion,consider splitting each surface into its own bug, 
and mark these as blocking the 672257 tracking bug. In my opinion 
there are many advantages to doing this:

* It makes it easier to track what has already been done and what is
left to do.
* If the feature work will span across different milestones, it's easier to tell
which parts were landed in which milestones since each bug will have its own M-
label.
* It is easier to track regressions, in case any individual CL needs to be
reverted.
* When a bug is used to track only a single piece of feature work, it makes it
simpler for the test team to perform manual verification.

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.cc:271: if (message_id ==
IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH)
TrayDetailsView::InfoLabel is a class used (or can potentially be used) by many
different UI surfaces, so I think a more scalable design would be to not have
specific mention of bluetooth/networking/etc within this class.

While considering my other comment regarding the consequences of having
InfoLabel extend ActionableView, consider how you could also address this
concern.

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.cc:300: case Action::OPEN_BLUETOOTH_SETTINGS:
Side note: please check with your PM as to whether they want to collect metrics
on how often this row is clicked. (These metrics can always be landed in a
follow-on CL.)

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:61: // message that can appear at the top of
the Bluetooth detailed view).
By making InfoLabel an ActionableView, you would be making all InfoLabel
instances inherit the 'clickable row' behaviors below, which are not desirable
to have for the non-clickable InfoRow instances, e.g., the info row that
displays the IDS_ASH_STATUS_TRAY_INITIALIZING_CELLULAR string:

* When clicked or tapped, the row will show an 'ink drop ripple'.

* When highlighting using ChromeVox spoken feedback, the row will be reported as
a "Button" when it isn't (see ActionableView::GetAccessibleNodeData()). As a
side note, it's a good practice to test your code with ChromeVox to make sure
a11y users are getting the correct experience - you can activate this from a11y
settings.

In other words, please look into an alternate design where all existing
InfoLabels do not change in behavior. (It's worth double-checking that what I
said above is correct too.)

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:70: // ActionableView
nit: ':' at end of 'ActionableView'

Kyle Horimoto

Your BUG= is the wrong one. Please use the cover bug, not the launch bug ...

3 years, 5 months ago (2017-06-27 19:36:57 UTC) #5

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#newcode7 ash/ash_strings.grd:7: <grit base_dir="." latest_public_release="0" current_release="1" On 2017/06/27 19:33:21, tdanderson wrote: ...

3 years, 5 months ago (2017-06-27 20:37:10 UTC) #6

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#new...
ash/ash_strings.grd:7: <grit base_dir="." latest_public_release="0"
current_release="1"
On 2017/06/27 19:33:21, tdanderson wrote:
> I see you are using bug 672257 to track many different changes. 
> As a suggestion,consider splitting each surface into its own bug, 
> and mark these as blocking the 672257 tracking bug. In my opinion 
> there are many advantages to doing this:
> 
> * It makes it easier to track what has already been done and what is
> left to do.
> * If the feature work will span across different milestones, it's easier to
tell
> which parts were landed in which milestones since each bug will have its own
M-
> label.
> * It is easier to track regressions, in case any individual CL needs to be
> reverted.
> * When a bug is used to track only a single piece of feature work, it makes it
> simpler for the test team to perform manual verification.

+1

Throughout this tether project, we've been using BUG=672263 for general-purpose
CLs that don't fix a particular bug (e.g., this CL).

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.cc:300: case Action::OPEN_BLUETOOTH_SETTINGS:
On 2017/06/27 19:33:21, tdanderson wrote:
> Side note: please check with your PM as to whether they want to collect
metrics
> on how often this row is clicked. (These metrics can always be landed in a
> follow-on CL.)

Good point. Leslie, please talk to hansberry@ regarding this - thanks! :)

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:61: // message that can appear at the top of
the Bluetooth detailed view).
On 2017/06/27 19:33:21, tdanderson wrote:
> By making InfoLabel an ActionableView, you would be making all InfoLabel
> instances inherit the 'clickable row' behaviors below, which are not desirable
> to have for the non-clickable InfoRow instances, e.g., the info row that
> displays the IDS_ASH_STATUS_TRAY_INITIALIZING_CELLULAR string:
> 
> * When clicked or tapped, the row will show an 'ink drop ripple'.
> 
> * When highlighting using ChromeVox spoken feedback, the row will be reported
as
> a "Button" when it isn't (see ActionableView::GetAccessibleNodeData()). As a
> side note, it's a good practice to test your code with ChromeVox to make sure
> a11y users are getting the correct experience - you can activate this from
a11y
> settings.
> 
> In other words, please look into an alternate design where all existing
> InfoLabels do not change in behavior. (It's worth double-checking that what I
> said above is correct too.)

Good point. Would you suggest creating a specific view type deriving from
ActionableView and using it only for this specific status message?

tdanderson

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#newcode7 ash/ash_strings.grd:7: <grit base_dir="." latest_public_release="0" current_release="1" On 2017/06/27 20:37:10, Kyle Horimoto ...

3 years, 5 months ago (2017-06-27 21:27:40 UTC) #7

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#new...
ash/ash_strings.grd:7: <grit base_dir="." latest_public_release="0"
current_release="1"
On 2017/06/27 20:37:10, Kyle Horimoto wrote:
> On 2017/06/27 19:33:21, tdanderson wrote:
> > I see you are using bug 672257 to track many different changes. 
> > As a suggestion,consider splitting each surface into its own bug, 
> > and mark these as blocking the 672257 tracking bug. In my opinion 
> > there are many advantages to doing this:
> > 
> > * It makes it easier to track what has already been done and what is
> > left to do.
> > * If the feature work will span across different milestones, it's easier to
> tell
> > which parts were landed in which milestones since each bug will have its own
> M-
> > label.
> > * It is easier to track regressions, in case any individual CL needs to be
> > reverted.
> > * When a bug is used to track only a single piece of feature work, it makes
it
> > simpler for the test team to perform manual verification.
> 
> +1
> 
> Throughout this tether project, we've been using BUG=672263 for
general-purpose
> CLs that don't fix a particular bug (e.g., this CL).

I would argue that the above points in favor of breaking out pieces of eng work
into separate bugs still apply even for CLs that are adding parts of a new
feature, such as this one. However ultimately this is a decision made by the
project TL.

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:61: // message that can appear at the top of
the Bluetooth detailed view).
On 2017/06/27 20:37:10, Kyle Horimoto wrote:
> On 2017/06/27 19:33:21, tdanderson wrote:
> > By making InfoLabel an ActionableView, you would be making all InfoLabel
> > instances inherit the 'clickable row' behaviors below, which are not
desirable
> > to have for the non-clickable InfoRow instances, e.g., the info row that
> > displays the IDS_ASH_STATUS_TRAY_INITIALIZING_CELLULAR string:
> > 
> > * When clicked or tapped, the row will show an 'ink drop ripple'.
> > 
> > * When highlighting using ChromeVox spoken feedback, the row will be
reported
> as
> > a "Button" when it isn't (see ActionableView::GetAccessibleNodeData()). As a
> > side note, it's a good practice to test your code with ChromeVox to make
sure
> > a11y users are getting the correct experience - you can activate this from
> a11y
> > settings.
> > 
> > In other words, please look into an alternate design where all existing
> > InfoLabels do not change in behavior. (It's worth double-checking that what
I
> > said above is correct too.)
> 
> Good point. Would you suggest creating a specific view type deriving from
> ActionableView and using it only for this specific status message?

I'll leave it to Leslie to investigate possible approaches here, but an
off-the-top-of-my-head idea would be to maybe have two concrete classes (one
extending View and the other extending ActionableView), perhaps with a common
interface. There would need to be some massaging of the logic in NetworkListView
with respect to how it instantiates/points to these objects.

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/40001/ash/system/network/network_icon.cc File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2957043002/diff/40001/ash/system/network/network_icon.cc#newcode885 ash/system/network/network_icon.cc:885: NetworkStateHandler::TECHNOLOGY_UNINITIALIZED) { On 2017/06/27 19:36:56, Kyle Horimoto wrote: > ...

3 years, 5 months ago (2017-06-29 19:24:32 UTC) #8

lesliewatkins

Description was changed from ========== Add a row in the network tray to inform users ...

3 years, 5 months ago (2017-07-07 16:12:22 UTC) #9

lesliewatkins

This is almost certainly going to take some more back-and-forth, but I totally (re)changed the ...

3 years, 5 months ago (2017-07-07 16:19:19 UTC) #10

This is almost certainly going to take some more back-and-forth, but I totally
(re)changed the InfoLabel class. Sorry for any comments I didn't respond to
directly-- they are probably no longer relevant due to the larger changes to
InfoLabel. Thanks as always for the feedback!

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/ash_strings.grd#new...
ash/ash_strings.grd:889: Turn on Bluetooth to discover nearby devices.
On 2017/06/27 19:36:56, Kyle Horimoto wrote:
> Can you please double-check with the UX folks to make sure we're supposed to
> have a period at the end of the sentence? I know it appears in the mocks, but
I
> just wanted to make sure, since some of the other status tray strings (like
the
> two above this one) don't have one. Thanks!

Email sent, will follow up with response.

https://codereview.chromium.org/2957043002/diff/40001/ash/system/network/netw...
File ash/system/network/network_icon.cc (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/network/netw...
ash/system/network/network_icon.cc:885:
NetworkStateHandler::TECHNOLOGY_UNINITIALIZED) {
On 2017/06/29 19:24:32, Kyle Horimoto wrote:
> On 2017/06/27 19:36:56, Kyle Horimoto wrote:
> > This condition also occurs if IsCellularAvailableButNotEnabled(). You also
> need
> > to check if that condition is true before displaying this message.
> 
> I added a quick CL to help you on this. Use
> https://codereview.chromium.org/2969493002 once it lands to determine this.

Will incorporate as soon as this lands. Thanks for doing that!

https://codereview.chromium.org/2957043002/diff/40001/ash/system/network/netw...
ash/system/network/network_icon.cc:889: } else if
(handler->GetTechnologyState(NetworkTypePattern::Mobile()) ==
On 2017/06/27 19:36:56, Kyle Horimoto wrote:
> Does this change fix
> https://bugs.chromium.org/p/chromium/issues/detail?id=735642? If so, please
set
> the BUG= number at the top.

Done.

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2957043002/diff/40001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.cc:300: case Action::OPEN_BLUETOOTH_SETTINGS:
On 2017/06/27 20:37:10, Kyle Horimoto wrote:
> On 2017/06/27 19:33:21, tdanderson wrote:
> > Side note: please check with your PM as to whether they want to collect
> metrics
> > on how often this row is clicked. (These metrics can always be landed in a
> > follow-on CL.)
> 
> Good point. Leslie, please talk to hansberry@ regarding this - thanks! :)

Sent him a message, will follow up when I get a response.

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/network_icon.cc File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/network_icon.cc#newcode886 ash/system/network/network_icon.cc:886: s_uninitialized_msg = IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH; nit: Add a TODO here to ...

3 years, 5 months ago (2017-07-07 17:34:00 UTC) #11

tdanderson

Please see my suggestions below. Note I am OOO until Thursday of next week, so ...

3 years, 5 months ago (2017-07-07 21:25:11 UTC) #12

Please see my suggestions below. Note I am OOO until Thursday of next week, so
if you need an OWNERS review before then, please add in another owner (I suggest
jamescook@). Otherwise I can take another look at this on Thursday.

https://codereview.chromium.org/2957043002/diff/60001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/ash_strings.grd#new...
ash/ash_strings.grd:2: 
nit: Please wrap the CL description at 80 chars (it makes it easier to read when
examining git logs).

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
File ash/system/network/network_list.cc (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
ash/system/network/network_list.cc:644: info_label =
TrayDetailsView::CreateInfoLabel(
On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> This will only create an info label of a given type once, meaning that if
> |message_id| ever changes and requires a different label type, it will never
get
> updated.
> 
> You'll need to check to see if a different label type should be created
instead
> of just updating the existing one.

+1, and IMO this is the most awkward part to deal with. Here is another idea I
just had, which may or may not work:

* Don't introduce any new subclasses of InfoLabel.

* Make InfoLabel extend ActionableView (like in your previous patch set).

* Introduce a helper function in network_list.cc (you can put it in an anonymous
namespace of the .cc, doesn't need to be part of the class) called something
like bool IsMessageClickable(int message_id). Based on the message id you pass
in it would return true if that would correspond to a clickable or non-clickable
message.

* Inside UpdateInfoLabel(), you can call
info_label->SetEnabled(IsMessageClickable(message_id)). SetEnabled() is
a method on views::View - if a view is set as disabled that means
it cannot be the target of any events, and thus will not show
the "ripple" effect when clicked even though it is an ActionableView.

* You will also probably want to have an info_label->Update() method
or something that will toggle the font styling based on if it is
clickable or not (clickable should be blue, non-clickable should be
whatever color InfoLabel uses right now). Perhaps also change the
accessibility role to "button" when clickable and "label" when not
clickable.

* You should still not make mention of networking/bluetooth/etc within
the InfoLabel class itself. InfoLabel::PerformAction() could delegate
back to NetworkListView in the following way:

    - Introduce a pure virtual interface InfoLabelDelegate.
      It would have one method, OnLabelClicked(int message_id).
    - Make NetworkListView implement InfoLabelDelegate.
    - Give the InfoLabel class an InfoLabelDelegate member.
    - InfoLabel::PerformAction() would just call
      delegate_->OnLabelClicked(message_id).
    - Then NetworkListView::OnLabelClicked() would perform the
      corresponding action (e.g., launching Bluetooth settings).
      It would probably be a good idea to have
      DCHECK(IsMessageClickable(message_id)) here too.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:71: class InfoLabelNoAction : public
InfoLabel {
On 2017/07/07 17:34:00, Kyle Horimoto wrote:
> nit: tanderson@ probably has a better idea about UI component naming, but I
> wouldn't add "NoAction" to the name. Instead, I'd just call this InfoLabel and
> have the extended view's name include something about how this is actionable
> (i.e., a button).

I made a suggestion above that could avoid introducing any new
subclasses. If that doesn't work though, consider if you could have
just two classes:

* InfoLabel (extends View, is non-clickable)
* ClickableInfoLabel (extends InfoLabel, is clickable)

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:85: public views::ButtonListener {
By extending ButtonListener instead of ActionableView, you won't get the
clickable row properties for free: material design "ripple" effect upon clicking
the row, a11y role, etc.

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/network_list.cc File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/network_list.cc#newcode644 ash/system/network/network_list.cc:644: info_label = TrayDetailsView::CreateInfoLabel( On 2017/07/07 21:25:11, tdanderson (OOO til ...

3 years, 5 months ago (2017-07-07 21:39:25 UTC) #13

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
File ash/system/network/network_list.cc (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
ash/system/network/network_list.cc:644: info_label =
TrayDetailsView::CreateInfoLabel(
On 2017/07/07 21:25:11, tdanderson (OOO til 7-13) wrote:
> On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> > This will only create an info label of a given type once, meaning that if
> > |message_id| ever changes and requires a different label type, it will never
> get
> > updated.
> > 
> > You'll need to check to see if a different label type should be created
> instead
> > of just updating the existing one.
> 
> +1, and IMO this is the most awkward part to deal with. Here is another idea I
> just had, which may or may not work:
> 
> * Don't introduce any new subclasses of InfoLabel.
> 
> * Make InfoLabel extend ActionableView (like in your previous patch set).
> 
> * Introduce a helper function in network_list.cc (you can put it in an
anonymous
> namespace of the .cc, doesn't need to be part of the class) called something
> like bool IsMessageClickable(int message_id). Based on the message id you pass
> in it would return true if that would correspond to a clickable or
non-clickable
> message.
> 
> * Inside UpdateInfoLabel(), you can call
> info_label->SetEnabled(IsMessageClickable(message_id)). SetEnabled() is
> a method on views::View - if a view is set as disabled that means
> it cannot be the target of any events, and thus will not show
> the "ripple" effect when clicked even though it is an ActionableView.
> 
> * You will also probably want to have an info_label->Update() method
> or something that will toggle the font styling based on if it is
> clickable or not (clickable should be blue, non-clickable should be
> whatever color InfoLabel uses right now). Perhaps also change the
> accessibility role to "button" when clickable and "label" when not
> clickable.
> 
> * You should still not make mention of networking/bluetooth/etc within
> the InfoLabel class itself. InfoLabel::PerformAction() could delegate
> back to NetworkListView in the following way:
> 
>     - Introduce a pure virtual interface InfoLabelDelegate.
>       It would have one method, OnLabelClicked(int message_id).
>     - Make NetworkListView implement InfoLabelDelegate.
>     - Give the InfoLabel class an InfoLabelDelegate member.
>     - InfoLabel::PerformAction() would just call
>       delegate_->OnLabelClicked(message_id).
>     - Then NetworkListView::OnLabelClicked() would perform the
>       corresponding action (e.g., launching Bluetooth settings).
>       It would probably be a good idea to have
>       DCHECK(IsMessageClickable(message_id)) here too.

This plan sounds great to me! Leslie, let me know if you need any help
implementing it.

lesliewatkins

lesliewatkins@chromium.org changed reviewers: + jamescook@chromium.org

3 years, 5 months ago (2017-07-09 00:57:46 UTC) #14

lesliewatkins

jamescook@, tdanderson@ suggested that you review this CL in his absence. https://codereview.chromium.org/2957043002/diff/60001/ash/ash_strings.grd File ash/ash_strings.grd (right): ...

3 years, 5 months ago (2017-07-09 00:57:48 UTC) #15

jamescook@, tdanderson@ suggested that you review this CL in his absence.

https://codereview.chromium.org/2957043002/diff/60001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/ash_strings.grd#new...
ash/ash_strings.grd:2: 
On 2017/07/07 21:25:11, tdanderson (OOO til 7-13) wrote:
> nit: Please wrap the CL description at 80 chars (it makes it easier to read
when
> examining git logs).

Acknowledged.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
File ash/system/network/network_icon.cc (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
ash/system/network/network_icon.cc:886: s_uninitialized_msg =
IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH;
On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> nit: Add a TODO here to only do this when Tether is uninitialized due to no
> Bluetooth.

Done.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
File ash/system/network/network_list.cc (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
ash/system/network/network_list.cc:644: info_label =
TrayDetailsView::CreateInfoLabel(
On 2017/07/07 21:25:11, tdanderson (OOO til 7-13) wrote:
> On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> > This will only create an info label of a given type once, meaning that if
> > |message_id| ever changes and requires a different label type, it will never
> get
> > updated.
> > 
> > You'll need to check to see if a different label type should be created
> instead
> > of just updating the existing one.
> 
> +1, and IMO this is the most awkward part to deal with. Here is another idea I
> just had, which may or may not work:
> 
> * Don't introduce any new subclasses of InfoLabel.
> 
> * Make InfoLabel extend ActionableView (like in your previous patch set).
> 
> * Introduce a helper function in network_list.cc (you can put it in an
anonymous
> namespace of the .cc, doesn't need to be part of the class) called something
> like bool IsMessageClickable(int message_id). Based on the message id you pass
> in it would return true if that would correspond to a clickable or
non-clickable
> message.
> 
> * Inside UpdateInfoLabel(), you can call
> info_label->SetEnabled(IsMessageClickable(message_id)). SetEnabled() is
> a method on views::View - if a view is set as disabled that means
> it cannot be the target of any events, and thus will not show
> the "ripple" effect when clicked even though it is an ActionableView.
> 
> * You will also probably want to have an info_label->Update() method
> or something that will toggle the font styling based on if it is
> clickable or not (clickable should be blue, non-clickable should be
> whatever color InfoLabel uses right now). Perhaps also change the
> accessibility role to "button" when clickable and "label" when not
> clickable.
> 
> * You should still not make mention of networking/bluetooth/etc within
> the InfoLabel class itself. InfoLabel::PerformAction() could delegate
> back to NetworkListView in the following way:
> 
>     - Introduce a pure virtual interface InfoLabelDelegate.
>       It would have one method, OnLabelClicked(int message_id).
>     - Make NetworkListView implement InfoLabelDelegate.
>     - Give the InfoLabel class an InfoLabelDelegate member.
>     - InfoLabel::PerformAction() would just call
>       delegate_->OnLabelClicked(message_id).
>     - Then NetworkListView::OnLabelClicked() would perform the
>       corresponding action (e.g., launching Bluetooth settings).
>       It would probably be a good idea to have
>       DCHECK(IsMessageClickable(message_id)) here too.

I hopefully did some variation of this, but again, might require fine-tuning.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/network/netw...
ash/system/network/network_list.cc:645: message_id);  // new
InfoLabel(message_id);
On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> Remove comment, make into a single line of code.
> 
> Also, remove the extra line you added below.

Done.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.cc:245:
TrayDetailsView::InfoLabel::InfoLabel(int message_id) {
On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> Nothing is actually done with |message_id|.

Acknowledged.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:68: DISALLOW_COPY_AND_ASSIGN(InfoLabel);
On 2017/07/07 17:33:59, Kyle Horimoto wrote:
> This should be private.

Done.

https://codereview.chromium.org/2957043002/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:71: class InfoLabelNoAction : public
InfoLabel {
On 2017/07/07 21:25:11, tdanderson (OOO til 7-13) wrote:
> On 2017/07/07 17:34:00, Kyle Horimoto wrote:
> > nit: tanderson@ probably has a better idea about UI component naming, but I
> > wouldn't add "NoAction" to the name. Instead, I'd just call this InfoLabel
and
> > have the extended view's name include something about how this is actionable
> > (i.e., a button).
> 
> I made a suggestion above that could avoid introducing any new
> subclasses. If that doesn't work though, consider if you could have
> just two classes:
> 
> * InfoLabel (extends View, is non-clickable)
> * ClickableInfoLabel (extends InfoLabel, is clickable)

Acknowledged.

Kyle Horimoto

Please update your CL description since it is no longer accurate. Thanks! https://codereview.chromium.org/2957043002/diff/80001/ash/system/bluetooth/tray_bluetooth.cc File ash/system/bluetooth/tray_bluetooth.cc ...

3 years, 5 months ago (2017-07-10 18:37:08 UTC) #16

James Cook

https://codereview.chromium.org/2957043002/diff/80001/ash/system/network/network_icon.cc File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2957043002/diff/80001/ash/system/network/network_icon.cc#newcode880 ash/system/network/network_icon.cc:880: static base::Time s_uninitialized_state_time; Aside: You didn't add it in ...

3 years, 5 months ago (2017-07-10 18:43:03 UTC) #17

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/80001/ash/system/network/network_icon.cc File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2957043002/diff/80001/ash/system/network/network_icon.cc#newcode885 ash/system/network/network_icon.cc:885: // due to no Bluetooth (dependent on 2969493002). On ...

3 years, 5 months ago (2017-07-10 19:15:37 UTC) #18

James Cook

Looks like I collided with Kyle. I'll let you sort things out with him first, ...

3 years, 5 months ago (2017-07-10 19:31:34 UTC) #19

James Cook

https://codereview.chromium.org/2957043002/diff/80001/ash/system/tray/tray_details_view.h File ash/system/tray/tray_details_view.h (right): https://codereview.chromium.org/2957043002/diff/80001/ash/system/tray/tray_details_view.h#newcode59 ash/system/tray/tray_details_view.h:59: class InfoLabelDelegate; On 2017/07/10 18:37:08, Kyle Horimoto wrote: > ...

3 years, 5 months ago (2017-07-11 23:14:15 UTC) #20

lesliewatkins

Description was changed from ========== Add a row in the network tray to inform users ...

3 years, 5 months ago (2017-07-12 21:48:21 UTC) #21

lesliewatkins

https://codereview.chromium.org/2957043002/diff/80001/ash/system/bluetooth/tray_bluetooth.cc File ash/system/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2957043002/diff/80001/ash/system/bluetooth/tray_bluetooth.cc#newcode319 ash/system/bluetooth/tray_bluetooth.cc:319: nullptr /* InfoLabelDelegate */)); On 2017/07/10 18:37:07, Kyle Horimoto ...

3 years, 5 months ago (2017-07-12 21:49:51 UTC) #22

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/100001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2957043002/diff/100001/ash/ash_strings.grd#newcode897 ash/ash_strings.grd:897: <message name="IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH" desc="The message to display in the network ...

3 years, 5 months ago (2017-07-12 22:45:59 UTC) #24

lesliewatkins

lesliewatkins@chromium.org changed reviewers: - tdanderson@chromium.org

3 years, 5 months ago (2017-07-13 22:12:46 UTC) #25

lesliewatkins

https://codereview.chromium.org/2957043002/diff/100001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2957043002/diff/100001/ash/ash_strings.grd#newcode897 ash/ash_strings.grd:897: <message name="IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH" desc="The message to display in the network ...

3 years, 5 months ago (2017-07-13 22:12:49 UTC) #26

https://codereview.chromium.org/2957043002/diff/100001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/ash_strings.grd#ne...
ash/ash_strings.grd:897: <message name="IDS_ASH_STATUS_TRAY_ENABLE_BLUETOOTH"
desc="The message to display in the network list when Tether is enabled but
Bluetooth is disabled.">
On 2017/07/12 22:45:58, Kyle Horimoto wrote:
> nit: Just checking - you said you had a presubmit warning that this file had a
> \t character. It isn't in this new message you added, right?

It was in mine, but it's fixed now!

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_d...
File ash/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.cc:20: #include
"ui/accessibility/ax_node_data.h"
On 2017/07/12 22:45:58, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_d...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:11: #include
"ash/system/tray/actionable_view.h"
On 2017/07/12 22:45:58, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:17: #include
"ui/views/controls/button/label_button.h"
On 2017/07/12 22:45:58, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:59: class TrayDetailClickHandler {
On 2017/07/12 22:45:58, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.cc:16: : ActionableView(nullptr /*owner*/,
TrayPopupInkDropStyle::FILL_BOUNDS),
On 2017/07/12 22:45:58, Kyle Horimoto wrote:
> nit: Spaces at beginning/end of comment: /* owner */

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label.h (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:17: // A view containing only a label, which
is to be inserted as a non-targetable
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> What does non-targetable mean? I Googled it and didn't see any View-related
> hits. Can you use a more specific word?

I didn't write that, but based on context, it may no longer be relevant. I'll
just remove the word.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:20: // InfoLabel can be clickable if it is
passed a non-null TrayDetailClickHandler
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> How about "InfoLabel can be clickable; this property is configured by its
> delegate."

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:20: // InfoLabel can be clickable if it is
passed a non-null TrayDetailClickHandler
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> InfoLabel --> TrayInfoLabel, TrayDetailClickHandler -> delegate

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:24: // A delegate for handling actions when an
InfoLabel is clicked.
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> nit: It does more than just that :)

Changed the wording to reflect that it also determines whether or not a label is
clickable.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:29: virtual bool LabelIsClickable(int
message_id) = 0;
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> nit: IsLabelClickable() reads better.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:32: // handler can be nullptr, in which case,
the InfoLabel will not be
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> handler --> delegate

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:32: // handler can be nullptr, in which case,
the InfoLabel will not be
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> How about "|delegate| may be null, which results in an InfoLabel which cannot
be
> clicked."

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label.h:38: // Updates the message id, the label text,
and the style if the
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> How about: "Updates the InfoLabel to display the message associated with
> |message_id|. This may update text styling if the delegate indicates that the
> InfoLabel should be clickable."

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:13: class TestEvent : public
ui::Event {
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> TestEvent --> TestClickEvent

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:18: class TestNonClickableDelegate :
public TrayInfoLabel::Delegate {
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> Just make one TestDelegate class which can be set to clickable or
non-clickable:
> 
> class TestDelegate : public TrayInfoLabel::Delegate {
>  public:
>   TestDelegate() {}
>   ~TestDelegate() override {}
> 
>   void clicked_message_ids() { return clicked_message_ids_; }
> 
>   void AddClickableMessageId(int message_id) {
>     clickable_message_ids_.insert(message_id);
>   }
> 
>   // TrayInfoLabel::Delegate:
>   void OnLabelClicked(int message_id) override {
>     clicked_message_ids_.push_back(message_id);
>   }
> 
>   void IsLabelClickable(int message_id) override {
>     return clickable_message_ids_.find(message_id) !=
> clickable_message_ids_.end();
>   }
> 
>  private:
>   std::vector<int> clicked_message_ids_;
>   unordered_set<int> clickable_message_ids_;
> };
> 
> Then configure the TestDelegate how you need to for each test.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:37: }  // namespace
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> nit: Newline after this.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:38: namespace test {
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> Is there a reason why you're using this extra namespace? I haven't seen it
> before in the Chromium code base. Did you copy this from another example?
> 
> If you aren't following a convention of other classes in this namespace,
please
> remove the extra namespace.

I moved the test namespace up to the top. The anonymous namespace is following
conventions as far as I can tell.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:40: class TrayInfoLabelTest : public
AshTestBase {
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> Add instance variables (std::unique_ptr's) for both the label and the
delegate.
> 
> Then, add a CreateLabel(int message_id, bool use_delegate) function. In that
> function, initialize your delegate if |use_delegate| is true, then initialize
> your label.
> 
> Then, change your Confirm{Non}ClickableLabel() to:
> 
> void VerifyClickability(bool expected_clickable) {
>   EXPECT_EQ(expected_clickable, label_->IsClickable());
> }
> 
> Then, add a function to click:
> 
> void ClickOnLabel() {
>   label_->PerformAction(TestClickEvent());
> }
> 
> Then, add functions which verify clicks:
> 
> void VerifyClicks(const std::vector<int> expected_clicked_message_ids) {
>   if (!delegate_) {
>     EXPECT_TRUE(expected_clicked_message_ids.empty());
>     return;
>   }
> 
>   EXPECT_EQ(expected_clicked_message_ids, delegate_->clicked_message_ids());
> }
> 
> void VerifyNoClicks() {
>   VerifyClicks(std::vector<int>());
> }
> 
> Your tests then turn into something like:
> 
> CreateLabel(1 /* message_id */, false /* use_delegate */);
> VerifyClickability(false);
> ClickOnLabel();
> VerifyNoClicks();
> 
> or
> 
> CreateLabel(1 /* message_id */, true /* use_delegate */);
> delegate_->AddClickableMessageId(1);
> VerifyClickability(true);
> ClickOnLabel();
> VerifyClicks(std::vector<int> { 1 });

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> For the tests with the delegate, call Update() with different message IDs and
> assert that the clickability changes and that clicks are handled properly.
Test
> multiple clicks, clicks on different message IDs, etc.

That's what the Update test does.
> 
> You also need to add tests which verify that the style was updated properly
and
> that the a11y stuff is correct after Update() causes them to change.

Can you give me an example of another system tray element that tests style and
a11y? It seems like pretty non-standard practice, and it's going to be difficult
and time consuming to implement, to the point that it may not be worth it. I'm
happy to be shown otherwise though.

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc File ash/system/tray/tray_info_label_unittest.cc (right): https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc#newcode38 ash/system/tray/tray_info_label_unittest.cc:38: namespace test { On 2017/07/13 22:12:48, lesliewatkins wrote: > ...

3 years, 5 months ago (2017-07-13 23:15:37 UTC) #27

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:38: namespace test {
On 2017/07/13 22:12:48, lesliewatkins wrote:
> On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> > Is there a reason why you're using this extra namespace? I haven't seen it
> > before in the Chromium code base. Did you copy this from another example?
> > 
> > If you aren't following a convention of other classes in this namespace,
> please
> > remove the extra namespace.
> 
> I moved the test namespace up to the top. The anonymous namespace is following
> conventions as far as I can tell.

Sorry I wasn't clear - I'm referring to the test namespace (this test is
ash::test::TrayInfoLabelTest), not the anonymous namespace :)

Yesterday, Codesearch was down, so I couldn't search very well, but now that
it's up again, it looks like there are only a few unit tests using the test::
namespace with hundreds not using it in //ash/system. Let's go ahead and get rid
of that namespace since it seems non-standard.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
On 2017/07/13 22:12:48, lesliewatkins wrote:
> On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> > For the tests with the delegate, call Update() with different message IDs
and
> > assert that the clickability changes and that clicks are handled properly.
> Test
> > multiple clicks, clicks on different message IDs, etc.
> 
> That's what the Update test does.
> > 
> > You also need to add tests which verify that the style was updated properly
> and
> > that the a11y stuff is correct after Update() causes them to change.
> 
> Can you give me an example of another system tray element that tests style and
> a11y? It seems like pretty non-standard practice, and it's going to be
difficult
> and time consuming to implement, to the point that it may not be worth it. I'm
> happy to be shown otherwise though.
> 

You just need to show that the correct values are returned by the label. To do
so, modify VerifyClickability():

void VerifyClickability(bool expected_clickable) {
  EXPECT_EQ(expected_clickable, label_->IsClickable());

  ui::AXNodeData node_data;
  label_->GetAccessibleNodeData(&node_data);

  if (expected_clickable) {
    EXPECT_EQ(ui::AX_ROLE_BUTTON, node_data.role);
    EXPECT_EQ(<clickableFontList>, label_->font_list());
    EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
    EXPECT_EQ(<clickableSelectionTextColor> label_->selection_text_color());
  } else {
    EXPECT_EQ(ui::AX_ROLE_LABEL_TEXT, node_data.role);
    EXPECT_EQ(<clickableFontList>, label_->font_list());
    EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
    EXPECT_EQ(<clickableSelectionTextColor> label_->selection_text_color());
  }
}

(You'll have to figure out the proper values for the things I put in <>'s. You
can find the values in your changes to tray_popup_item_style.cc.)

As an alternative to the if/else above, you could calculate all the variables
ahead of time and only call the EXPECT_EQ() once (instead of once in the "if"
and once in the "else"). Example:

AXRole expected_role = expected_clickable ?
    ui::AX_ROLE_BUTTON : ui::AX_ROLE_LABEL_TEXT;
/* Create other expected variables... */

EXPECT_EQ(expected_role, node_data.role);
/* Verify other expected values against label_... */

https://codereview.chromium.org/2957043002/diff/120001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/120001/ash/ash_strings.grd#ne...
ash/ash_strings.grd:898: Turn on Bluetooth to discover nearby devices.
Just double-checking: I might be remembering wrong, but I thought UX said we
should not have the period at the end here. Can you confirm? Thanks!

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:14: const int clickableMessageId =
1;
nit: Constants are prefixed with a k: (e.g., kClickableMessageId). Same below.

(But I think it will be better to use the suggestions I had below instead.)

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:44: std::unique_ptr<TrayInfoLabel>
label_;
nit: Move these to the bottom of the test class - functions should appear first.
Also, make them protected.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:49: delegate_ =
base::MakeUnique<TestDelegate>() else delegate_ = nullptr;
Instead of setting delegate_ to nullptr in the else case, please add a
TearDown() function which resets the pointer. That way, we can just assume that
the delegate starts off as null.

Two nits:
(1) Instead of setting a std::unique_ptr to nullptr, just call reset().
(2) It's difficult to read when you put the "else" condition on the same line as
the "if". You should always put the "else" on a new line.

How TearDown() should look:

void TearDown() override {
  label_.reset();
  delegate_.reset();
}

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:54: void UpdateLabel(int message_id)
{ label_->Update(message_id); }
No need for this function - Update() is already a public function on
TrayInfoLabel, so you can just call the function directly.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:56: void ClickOnLabel() {
label_->PerformAction(TestClickEvent()); }
Let's also verify that the correct bool is returned by PerformAction().

To do so, take a click_expected_to_be_handled bool as a parameter to
ClickOnLabel(), then verify that the value is what we expect:

void ClickOnLabel(bool click_expected_to_be_handled) {
  bool was_handled = label_->PerformAction(TestClickEvent());
  EXPECT_EQ(click_expected_to_be_handled, was_handled);
}

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:58: VerifyMessageId(int
expected_message_id) {
No need for this function either. In general, you should only test the public
API of the class under test as well as whether the class under test interacted
with other objects which have been injected into the class under test. From the
perspective of a test, we shouldn't care about what the internal details of the
class look like as long as it works.

Imagine, for example, that someone edits TrayInfoLabel later and decides that
the message ID should be stored internally as a uint64_t instead of an int. This
person would only be changing internal implementation details of the class, and
none of the public-facing API should change how it functions. In this case, we
would want the unit tests for this class to continue to work as expected,
without any modification. If you add tests which use implementation details
(i.e., private/protected fields) of the class, the test becomes more brittle and
must be updated when the implementation changes, even if the functionality of
the class does not change.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:79: CreateLabel(false,
nonClickableMessageId);
nit: /* use_delegate */

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:81: VerifyClickability(false);
After this, update the label to another message ID and verify that it is still
not clickable.

In my comment on the last patchset, I suggested adding a clickable_message_ids
set which you could set on a per-test basis. This makes it easy to determine
which is clickable and which is not, instead of using clickable/non-clickable
constants. If you went that route, this test could read like:

TEST_F(TrayInfoLabelTest, NoDelegate) {
  CreateLabel(false /* use_delegate */, 1 /* message_id */);
  VerifyClickability(false);

  label_->Update(2 /* message_id *);
  VerifyClickability(false);

  label_->Update(3 /* message_id *);
  VerifyClickability(false);
}

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:84: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
Instead of having a delegate which is entirely clickable vs. entirely
non-clickable and no updates vs. one update, let's write one test which combines
these:

TEST_F(TrayInfoLabelTest, TestUpdate) {
  // Message IDs 1 and 2 are clickable; all others are not.
  delegate_->AddClickableMessageId(1);
  delegate_->AddClickableMessageId(2);

  CreateLabel(true /* use_delegate */, 1 /* message_id */);
  VerifyClickability(true);

  label_->Update(2 /* message_id *);
  VerifyClickability(true);

  label_->Update(3 /* message_id *);
  VerifyClickability(false);

  label_->Update(4 /* message_id *);
  VerifyClickability(false);
}

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:96: TEST_F(TrayInfoLabelTest,
PerformAction) {
To test performing actions, verify that you can click multiple times on
different message IDs, and verify that clicking when the label is not clickable
does not result in a registered click:

TEST_F(TrayInfoLabelTest, TestClicks) {
  // Message IDs 1 and 2 are clickable; all others are not.
  delegate_->AddClickableMessageId(1);
  delegate_->AddClickableMessageId(2);

  CreateLabel(true /* use_delegate */, 1 /* message_id */);
  VerifyClickability(true);

  // Click once.
  ClickOnLabel(true /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1});

  // Click again.
  ClickOnLabel(true /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1, 1});

  // Update to another clickable message ID and click again.
  label_->Update(2 /* message_id *);
  VerifyClickability(true);
  ClickOnLabel(true /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1, 1, 2});

  // Update to a non-clickable message ID.
  label_->Update(3 /* message_id *);
  VerifyClickability(false);

  // Click on the label; the delegate should not have received a
  // click, so the verified clicks should remain the same.
  ClickOnLabel(false /* click_expected_to_be_handled */);
  VerifyClicks(std::vector<int> {1, 1, 2});
}

lesliewatkins

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc File ash/system/tray/tray_info_label_unittest.cc (right): https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc#newcode38 ash/system/tray/tray_info_label_unittest.cc:38: namespace test { On 2017/07/13 23:15:36, Kyle Horimoto wrote: ...

3 years, 5 months ago (2017-07-14 06:41:54 UTC) #28

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:38: namespace test {
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> On 2017/07/13 22:12:48, lesliewatkins wrote:
> > On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> > > Is there a reason why you're using this extra namespace? I haven't seen it
> > > before in the Chromium code base. Did you copy this from another example?
> > > 
> > > If you aren't following a convention of other classes in this namespace,
> > please
> > > remove the extra namespace.
> > 
> > I moved the test namespace up to the top. The anonymous namespace is
following
> > conventions as far as I can tell.
> 
> Sorry I wasn't clear - I'm referring to the test namespace (this test is
> ash::test::TrayInfoLabelTest), not the anonymous namespace :)
> 
> Yesterday, Codesearch was down, so I couldn't search very well, but now that
> it's up again, it looks like there are only a few unit tests using the test::
> namespace with hundreds not using it in //ash/system. Let's go ahead and get
rid
> of that namespace since it seems non-standard.

Done.

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> On 2017/07/13 22:12:48, lesliewatkins wrote:
> > On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> > > For the tests with the delegate, call Update() with different message IDs
> and
> > > assert that the clickability changes and that clicks are handled properly.
> > Test
> > > multiple clicks, clicks on different message IDs, etc.
> > 
> > That's what the Update test does.
> > > 
> > > You also need to add tests which verify that the style was updated
properly
> > and
> > > that the a11y stuff is correct after Update() causes them to change.
> > 
> > Can you give me an example of another system tray element that tests style
and
> > a11y? It seems like pretty non-standard practice, and it's going to be
> difficult
> > and time consuming to implement, to the point that it may not be worth it.
I'm
> > happy to be shown otherwise though.
> > 
> 
> You just need to show that the correct values are returned by the label. To do
> so, modify VerifyClickability():
> 
> void VerifyClickability(bool expected_clickable) {
>   EXPECT_EQ(expected_clickable, label_->IsClickable());
> 
>   ui::AXNodeData node_data;
>   label_->GetAccessibleNodeData(&node_data);
> 
>   if (expected_clickable) {
>     EXPECT_EQ(ui::AX_ROLE_BUTTON, node_data.role);
>     EXPECT_EQ(<clickableFontList>, label_->font_list());
>     EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
>     EXPECT_EQ(<clickableSelectionTextColor> label_->selection_text_color());
>   } else {
>     EXPECT_EQ(ui::AX_ROLE_LABEL_TEXT, node_data.role);
>     EXPECT_EQ(<clickableFontList>, label_->font_list());
>     EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
>     EXPECT_EQ(<clickableSelectionTextColor> label_->selection_text_color());
>   }
> }
> 
> (You'll have to figure out the proper values for the things I put in <>'s. You
> can find the values in your changes to tray_popup_item_style.cc.)
> 
> As an alternative to the if/else above, you could calculate all the variables
> ahead of time and only call the EXPECT_EQ() once (instead of once in the "if"
> and once in the "else"). Example:
> 
> AXRole expected_role = expected_clickable ?
>     ui::AX_ROLE_BUTTON : ui::AX_ROLE_LABEL_TEXT;
> /* Create other expected variables... */
> 
> EXPECT_EQ(expected_role, node_data.role);
> /* Verify other expected values against label_... */

I added the AX node stuff, but still not convinced that checking the font and
style is the purpose of this test. At any rate, FontList doesn't seem to even
have a comparison operator.

https://codereview.chromium.org/2957043002/diff/120001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/2957043002/diff/120001/ash/ash_strings.grd#ne...
ash/ash_strings.grd:898: Turn on Bluetooth to discover nearby devices.
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> Just double-checking: I might be remembering wrong, but I thought UX said we
> should not have the period at the end here. Can you confirm? Thanks!

No period. Fixed!

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:14: const int clickableMessageId =
1;
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> nit: Constants are prefixed with a k: (e.g., kClickableMessageId). Same below.
> 
> (But I think it will be better to use the suggestions I had below instead.)

Acknowledged.

I really would prefer to keep the constants-- I think they're easier to read and
more accurately test the scenario where this class is being used. I also think
that they fully test everything in a simpler way.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:44: std::unique_ptr<TrayInfoLabel>
label_;
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> nit: Move these to the bottom of the test class - functions should appear
first.
> Also, make them protected.

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:49: delegate_ =
base::MakeUnique<TestDelegate>() else delegate_ = nullptr;
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> Instead of setting delegate_ to nullptr in the else case, please add a
> TearDown() function which resets the pointer. That way, we can just assume
that
> the delegate starts off as null.
> 
> Two nits:
> (1) Instead of setting a std::unique_ptr to nullptr, just call reset().
> (2) It's difficult to read when you put the "else" condition on the same line
as
> the "if". You should always put the "else" on a new line.
> 
> How TearDown() should look:
> 
> void TearDown() override {
>   label_.reset();
>   delegate_.reset();
> }

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:54: void UpdateLabel(int message_id)
{ label_->Update(message_id); }
On 2017/07/13 23:15:37, Kyle Horimoto wrote:
> No need for this function - Update() is already a public function on
> TrayInfoLabel, so you can just call the function directly.

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:56: void ClickOnLabel() {
label_->PerformAction(TestClickEvent()); }
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> Let's also verify that the correct bool is returned by PerformAction().
> 
> To do so, take a click_expected_to_be_handled bool as a parameter to
> ClickOnLabel(), then verify that the value is what we expect:
> 
> void ClickOnLabel(bool click_expected_to_be_handled) {
>   bool was_handled = label_->PerformAction(TestClickEvent());
>   EXPECT_EQ(click_expected_to_be_handled, was_handled);
> }

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:58: VerifyMessageId(int
expected_message_id) {
On 2017/07/13 23:15:37, Kyle Horimoto wrote:
> No need for this function either. In general, you should only test the public
> API of the class under test as well as whether the class under test interacted
> with other objects which have been injected into the class under test. From
the
> perspective of a test, we shouldn't care about what the internal details of
the
> class look like as long as it works.
> 
> Imagine, for example, that someone edits TrayInfoLabel later and decides that
> the message ID should be stored internally as a uint64_t instead of an int.
This
> person would only be changing internal implementation details of the class,
and
> none of the public-facing API should change how it functions. In this case, we
> would want the unit tests for this class to continue to work as expected,
> without any modification. If you add tests which use implementation details
> (i.e., private/protected fields) of the class, the test becomes more brittle
and
> must be updated when the implementation changes, even if the functionality of
> the class does not change.

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:79: CreateLabel(false,
nonClickableMessageId);
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> nit: /* use_delegate */

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:81: VerifyClickability(false);
On 2017/07/13 23:15:37, Kyle Horimoto wrote:
> After this, update the label to another message ID and verify that it is still
> not clickable.
> 
> In my comment on the last patchset, I suggested adding a clickable_message_ids
> set which you could set on a per-test basis. This makes it easy to determine
> which is clickable and which is not, instead of using clickable/non-clickable
> constants. If you went that route, this test could read like:
> 
> TEST_F(TrayInfoLabelTest, NoDelegate) {
>   CreateLabel(false /* use_delegate */, 1 /* message_id */);
>   VerifyClickability(false);
> 
>   label_->Update(2 /* message_id *);
>   VerifyClickability(false);
> 
>   label_->Update(3 /* message_id *);
>   VerifyClickability(false);
> }

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:84: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> Instead of having a delegate which is entirely clickable vs. entirely
> non-clickable and no updates vs. one update, let's write one test which
combines
> these:
> 
> TEST_F(TrayInfoLabelTest, TestUpdate) {
>   // Message IDs 1 and 2 are clickable; all others are not.
>   delegate_->AddClickableMessageId(1);
>   delegate_->AddClickableMessageId(2);
> 
>   CreateLabel(true /* use_delegate */, 1 /* message_id */);
>   VerifyClickability(true);
> 
>   label_->Update(2 /* message_id *);
>   VerifyClickability(true);
> 
>   label_->Update(3 /* message_id *);
>   VerifyClickability(false);
> 
>   label_->Update(4 /* message_id *);
>   VerifyClickability(false);
> }

Done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:96: TEST_F(TrayInfoLabelTest,
PerformAction) {
On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> To test performing actions, verify that you can click multiple times on
> different message IDs, and verify that clicking when the label is not
clickable
> does not result in a registered click:
> 
> TEST_F(TrayInfoLabelTest, TestClicks) {
>   // Message IDs 1 and 2 are clickable; all others are not.
>   delegate_->AddClickableMessageId(1);
>   delegate_->AddClickableMessageId(2);
> 
>   CreateLabel(true /* use_delegate */, 1 /* message_id */);
>   VerifyClickability(true);
> 
>   // Click once.
>   ClickOnLabel(true /* click_expected_to_be_handled */);
>   VerifyClicks(std::vector<int> {1});
> 
>   // Click again.
>   ClickOnLabel(true /* click_expected_to_be_handled */);
>   VerifyClicks(std::vector<int> {1, 1});
> 
>   // Update to another clickable message ID and click again.
>   label_->Update(2 /* message_id *);
>   VerifyClickability(true);
>   ClickOnLabel(true /* click_expected_to_be_handled */);
>   VerifyClicks(std::vector<int> {1, 1, 2});
> 
>   // Update to a non-clickable message ID.
>   label_->Update(3 /* message_id *);
>   VerifyClickability(false);
> 
>   // Click on the label; the delegate should not have received a
>   // click, so the verified clicks should remain the same.
>   ClickOnLabel(false /* click_expected_to_be_handled */);
>   VerifyClicks(std::vector<int> {1, 1, 2});
> }

Done.

Kyle Horimoto

Almost there!!! https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc File ash/system/tray/tray_info_label_unittest.cc (right): https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc#newcode59 ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest, NonClickableDelegate) { On 2017/07/14 06:41:51, lesliewatkins ...

3 years, 5 months ago (2017-07-14 17:54:22 UTC) #29

Almost there!!!

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
On 2017/07/14 06:41:51, lesliewatkins wrote:
> On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> > On 2017/07/13 22:12:48, lesliewatkins wrote:
> > > On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> > > > For the tests with the delegate, call Update() with different message
IDs
> > and
> > > > assert that the clickability changes and that clicks are handled
properly.
> > > Test
> > > > multiple clicks, clicks on different message IDs, etc.
> > > 
> > > That's what the Update test does.
> > > > 
> > > > You also need to add tests which verify that the style was updated
> properly
> > > and
> > > > that the a11y stuff is correct after Update() causes them to change.
> > > 
> > > Can you give me an example of another system tray element that tests style
> and
> > > a11y? It seems like pretty non-standard practice, and it's going to be
> > difficult
> > > and time consuming to implement, to the point that it may not be worth it.
> I'm
> > > happy to be shown otherwise though.
> > > 
> > 
> > You just need to show that the correct values are returned by the label. To
do
> > so, modify VerifyClickability():
> > 
> > void VerifyClickability(bool expected_clickable) {
> >   EXPECT_EQ(expected_clickable, label_->IsClickable());
> > 
> >   ui::AXNodeData node_data;
> >   label_->GetAccessibleNodeData(&node_data);
> > 
> >   if (expected_clickable) {
> >     EXPECT_EQ(ui::AX_ROLE_BUTTON, node_data.role);
> >     EXPECT_EQ(<clickableFontList>, label_->font_list());
> >     EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
> >     EXPECT_EQ(<clickableSelectionTextColor> label_->selection_text_color());
> >   } else {
> >     EXPECT_EQ(ui::AX_ROLE_LABEL_TEXT, node_data.role);
> >     EXPECT_EQ(<clickableFontList>, label_->font_list());
> >     EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
> >     EXPECT_EQ(<clickableSelectionTextColor> label_->selection_text_color());
> >   }
> > }
> > 
> > (You'll have to figure out the proper values for the things I put in <>'s.
You
> > can find the values in your changes to tray_popup_item_style.cc.)
> > 
> > As an alternative to the if/else above, you could calculate all the
variables
> > ahead of time and only call the EXPECT_EQ() once (instead of once in the
"if"
> > and once in the "else"). Example:
> > 
> > AXRole expected_role = expected_clickable ?
> >     ui::AX_ROLE_BUTTON : ui::AX_ROLE_LABEL_TEXT;
> > /* Create other expected variables... */
> > 
> > EXPECT_EQ(expected_role, node_data.role);
> > /* Verify other expected values against label_... */
> 
> I added the AX node stuff, but still not convinced that checking the font and
> style is the purpose of this test. At any rate, FontList doesn't seem to even
> have a comparison operator.

Okay, that's fine! The a11y stuff was the bigger concern of mine.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:14: const int clickableMessageId =
1;
On 2017/07/14 06:41:52, lesliewatkins wrote:
> On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> > nit: Constants are prefixed with a k: (e.g., kClickableMessageId). Same
below.
> > 
> > (But I think it will be better to use the suggestions I had below instead.)
> 
> Acknowledged.
> 
> I really would prefer to keep the constants-- I think they're easier to read
and
> more accurately test the scenario where this class is being used. I also think
> that they fully test everything in a simpler way.

The reason that I do not like these is because they are misleading when we don't
have a delegate. For example, your test currently has these lines:

label_->Update(kClickableMessageId);
VerifyClickability(false);

This makes the test hard to understand since, presumably, if you set a clickable
message ID, your label should be clickable.

Since the clickability of a message ID depends on the delegate's implementation,
it does not makes sense to make constants which are agnostic to the delegate.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:96: TEST_F(TrayInfoLabelTest,
PerformAction) {
On 2017/07/14 06:41:52, lesliewatkins wrote:
> On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> > To test performing actions, verify that you can click multiple times on
> > different message IDs, and verify that clicking when the label is not
> clickable
> > does not result in a registered click:
> > 
> > TEST_F(TrayInfoLabelTest, TestClicks) {
> >   // Message IDs 1 and 2 are clickable; all others are not.
> >   delegate_->AddClickableMessageId(1);
> >   delegate_->AddClickableMessageId(2);
> > 
> >   CreateLabel(true /* use_delegate */, 1 /* message_id */);
> >   VerifyClickability(true);
> > 
> >   // Click once.
> >   ClickOnLabel(true /* click_expected_to_be_handled */);
> >   VerifyClicks(std::vector<int> {1});
> > 
> >   // Click again.
> >   ClickOnLabel(true /* click_expected_to_be_handled */);
> >   VerifyClicks(std::vector<int> {1, 1});
> > 
> >   // Update to another clickable message ID and click again.
> >   label_->Update(2 /* message_id *);
> >   VerifyClickability(true);
> >   ClickOnLabel(true /* click_expected_to_be_handled */);
> >   VerifyClicks(std::vector<int> {1, 1, 2});
> > 
> >   // Update to a non-clickable message ID.
> >   label_->Update(3 /* message_id *);
> >   VerifyClickability(false);
> > 
> >   // Click on the label; the delegate should not have received a
> >   // click, so the verified clicks should remain the same.
> >   ClickOnLabel(false /* click_expected_to_be_handled */);
> >   VerifyClicks(std::vector<int> {1, 1, 2});
> > }
> 
> Done.

Hey Leslie, doesn't look like this was done. Your test only simulates one click
on one clickable ID and one click on one non-clickable ID. Please include
multiple clicks on multiple clickable and multiple non-clickable IDs.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:25: TestDelegate() :
clicked_message_ids_(std::vector<int>()) {}
std::vector has a default constructor - no need to include this in the
initializer list.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:29: void OnLabelClicked(int
message_id) override {
Before this line:

  // TrayInfoLabel::Delegate:

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:69: gfx::FontList font_list =
views::Label::GetDefaultFontList();
Unused - remove.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:76: void VerifyClicks(const
std::vector<int> expected_clicked_message_ids) {
const std::vector<int>&

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:94: 
nit: Generally, tests are easier to read when the verification comes right after
the action so that a reader can logically group things together.

CreateLabel();
VerifyClickability(false);

Update();
VerifyClickability(false);

Same below for other tests.

lesliewatkins

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc File ash/system/tray/tray_info_label_unittest.cc (right): https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_info_label_unittest.cc#newcode59 ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest, NonClickableDelegate) { On 2017/07/14 17:54:21, Kyle Horimoto wrote: ...

3 years, 5 months ago (2017-07-14 20:43:17 UTC) #30

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/100001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:59: TEST_F(TrayInfoLabelTest,
NonClickableDelegate) {
On 2017/07/14 17:54:21, Kyle Horimoto wrote:
> On 2017/07/14 06:41:51, lesliewatkins wrote:
> > On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> > > On 2017/07/13 22:12:48, lesliewatkins wrote:
> > > > On 2017/07/12 22:45:59, Kyle Horimoto wrote:
> > > > > For the tests with the delegate, call Update() with different message
> IDs
> > > and
> > > > > assert that the clickability changes and that clicks are handled
> properly.
> > > > Test
> > > > > multiple clicks, clicks on different message IDs, etc.
> > > > 
> > > > That's what the Update test does.
> > > > > 
> > > > > You also need to add tests which verify that the style was updated
> > properly
> > > > and
> > > > > that the a11y stuff is correct after Update() causes them to change.
> > > > 
> > > > Can you give me an example of another system tray element that tests
style
> > and
> > > > a11y? It seems like pretty non-standard practice, and it's going to be
> > > difficult
> > > > and time consuming to implement, to the point that it may not be worth
it.
> > I'm
> > > > happy to be shown otherwise though.
> > > > 
> > > 
> > > You just need to show that the correct values are returned by the label.
To
> do
> > > so, modify VerifyClickability():
> > > 
> > > void VerifyClickability(bool expected_clickable) {
> > >   EXPECT_EQ(expected_clickable, label_->IsClickable());
> > > 
> > >   ui::AXNodeData node_data;
> > >   label_->GetAccessibleNodeData(&node_data);
> > > 
> > >   if (expected_clickable) {
> > >     EXPECT_EQ(ui::AX_ROLE_BUTTON, node_data.role);
> > >     EXPECT_EQ(<clickableFontList>, label_->font_list());
> > >     EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
> > >     EXPECT_EQ(<clickableSelectionTextColor>
label_->selection_text_color());
> > >   } else {
> > >     EXPECT_EQ(ui::AX_ROLE_LABEL_TEXT, node_data.role);
> > >     EXPECT_EQ(<clickableFontList>, label_->font_list());
> > >     EXPECT_EQ(<clickableEnabledColor> label_->enabled_color());
> > >     EXPECT_EQ(<clickableSelectionTextColor>
label_->selection_text_color());
> > >   }
> > > }
> > > 
> > > (You'll have to figure out the proper values for the things I put in <>'s.
> You
> > > can find the values in your changes to tray_popup_item_style.cc.)
> > > 
> > > As an alternative to the if/else above, you could calculate all the
> variables
> > > ahead of time and only call the EXPECT_EQ() once (instead of once in the
> "if"
> > > and once in the "else"). Example:
> > > 
> > > AXRole expected_role = expected_clickable ?
> > >     ui::AX_ROLE_BUTTON : ui::AX_ROLE_LABEL_TEXT;
> > > /* Create other expected variables... */
> > > 
> > > EXPECT_EQ(expected_role, node_data.role);
> > > /* Verify other expected values against label_... */
> > 
> > I added the AX node stuff, but still not convinced that checking the font
and
> > style is the purpose of this test. At any rate, FontList doesn't seem to
even
> > have a comparison operator.
> 
> Okay, that's fine! The a11y stuff was the bigger concern of mine.

Acknowledged.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:14: const int clickableMessageId =
1;
On 2017/07/14 17:54:22, Kyle Horimoto wrote:
> On 2017/07/14 06:41:52, lesliewatkins wrote:
> > On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> > > nit: Constants are prefixed with a k: (e.g., kClickableMessageId). Same
> below.
> > > 
> > > (But I think it will be better to use the suggestions I had below
instead.)
> > 
> > Acknowledged.
> > 
> > I really would prefer to keep the constants-- I think they're easier to read
> and
> > more accurately test the scenario where this class is being used. I also
think
> > that they fully test everything in a simpler way.
> 
> The reason that I do not like these is because they are misleading when we
don't
> have a delegate. For example, your test currently has these lines:
> 
> label_->Update(kClickableMessageId);
> VerifyClickability(false);
> 
> This makes the test hard to understand since, presumably, if you set a
clickable
> message ID, your label should be clickable.
> 
> Since the clickability of a message ID depends on the delegate's
implementation,
> it does not makes sense to make constants which are agnostic to the delegate.

Alrighty, done.

https://codereview.chromium.org/2957043002/diff/120001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:96: TEST_F(TrayInfoLabelTest,
PerformAction) {
On 2017/07/14 17:54:21, Kyle Horimoto wrote:
> On 2017/07/14 06:41:52, lesliewatkins wrote:
> > On 2017/07/13 23:15:36, Kyle Horimoto wrote:
> > > To test performing actions, verify that you can click multiple times on
> > > different message IDs, and verify that clicking when the label is not
> > clickable
> > > does not result in a registered click:
> > > 
> > > TEST_F(TrayInfoLabelTest, TestClicks) {
> > >   // Message IDs 1 and 2 are clickable; all others are not.
> > >   delegate_->AddClickableMessageId(1);
> > >   delegate_->AddClickableMessageId(2);
> > > 
> > >   CreateLabel(true /* use_delegate */, 1 /* message_id */);
> > >   VerifyClickability(true);
> > > 
> > >   // Click once.
> > >   ClickOnLabel(true /* click_expected_to_be_handled */);
> > >   VerifyClicks(std::vector<int> {1});
> > > 
> > >   // Click again.
> > >   ClickOnLabel(true /* click_expected_to_be_handled */);
> > >   VerifyClicks(std::vector<int> {1, 1});
> > > 
> > >   // Update to another clickable message ID and click again.
> > >   label_->Update(2 /* message_id *);
> > >   VerifyClickability(true);
> > >   ClickOnLabel(true /* click_expected_to_be_handled */);
> > >   VerifyClicks(std::vector<int> {1, 1, 2});
> > > 
> > >   // Update to a non-clickable message ID.
> > >   label_->Update(3 /* message_id *);
> > >   VerifyClickability(false);
> > > 
> > >   // Click on the label; the delegate should not have received a
> > >   // click, so the verified clicks should remain the same.
> > >   ClickOnLabel(false /* click_expected_to_be_handled */);
> > >   VerifyClicks(std::vector<int> {1, 1, 2});
> > > }
> > 
> > Done.
> 
> Hey Leslie, doesn't look like this was done. Your test only simulates one
click
> on one clickable ID and one click on one non-clickable ID. Please include
> multiple clicks on multiple clickable and multiple non-clickable IDs.

Okay, I added some more clicks.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
File ash/system/tray/tray_info_label_unittest.cc (right):

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:25: TestDelegate() :
clicked_message_ids_(std::vector<int>()) {}
On 2017/07/14 17:54:22, Kyle Horimoto wrote:
> std::vector has a default constructor - no need to include this in the
> initializer list.

Done.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:29: void OnLabelClicked(int
message_id) override {
On 2017/07/14 17:54:22, Kyle Horimoto wrote:
> Before this line:
> 
>   // TrayInfoLabel::Delegate:

Done.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:69: gfx::FontList font_list =
views::Label::GetDefaultFontList();
On 2017/07/14 17:54:22, Kyle Horimoto wrote:
> Unused - remove.

Done.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:76: void VerifyClicks(const
std::vector<int> expected_clicked_message_ids) {
On 2017/07/14 17:54:22, Kyle Horimoto wrote:
> const std::vector<int>&

Done.

https://codereview.chromium.org/2957043002/diff/140001/ash/system/tray/tray_i...
ash/system/tray/tray_info_label_unittest.cc:94: 
On 2017/07/14 17:54:22, Kyle Horimoto wrote:
> nit: Generally, tests are easier to read when the verification comes right
after
> the action so that a reader can logically group things together.
> 
> CreateLabel();
> VerifyClickability(false);
> 
> Update();
> VerifyClickability(false);
> 
> Same below for other tests.

Done.

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/160001/ash/system/tray/tray_info_label.h File ash/system/tray/tray_info_label.h (right): https://codereview.chromium.org/2957043002/diff/160001/ash/system/tray/tray_info_label.h#newcode27 ash/system/tray/tray_info_label.h:27: virtual bool IsLabelClickable(int message_id) = 0; nit: Make this ...

3 years, 5 months ago (2017-07-14 20:58:50 UTC) #31

lesliewatkins

https://codereview.chromium.org/2957043002/diff/160001/ash/system/tray/tray_info_label.h File ash/system/tray/tray_info_label.h (right): https://codereview.chromium.org/2957043002/diff/160001/ash/system/tray/tray_info_label.h#newcode27 ash/system/tray/tray_info_label.h:27: virtual bool IsLabelClickable(int message_id) = 0; On 2017/07/14 20:58:49, ...

3 years, 5 months ago (2017-07-18 17:00:06 UTC) #32

Kyle Horimoto

LGTM w/ nits. Nice work - thanks for sticking through the review process! https://codereview.chromium.org/2957043002/diff/180001/ash/system/tray/tray_info_label.h File ...

3 years, 5 months ago (2017-07-18 17:10:05 UTC) #33

James Cook

LGTM with nits and an optional suggestion. Nice CL. https://codereview.chromium.org/2957043002/diff/180001/ash/system/tray/tray_info_label.cc File ash/system/tray/tray_info_label.cc (right): https://codereview.chromium.org/2957043002/diff/180001/ash/system/tray/tray_info_label.cc#newcode1 ash/system/tray/tray_info_label.cc:1: ...

3 years, 5 months ago (2017-07-18 17:26:10 UTC) #34

Kyle Horimoto

https://codereview.chromium.org/2957043002/diff/180001/ash/system/tray/tray_info_label_unittest.cc File ash/system/tray/tray_info_label_unittest.cc (right): https://codereview.chromium.org/2957043002/diff/180001/ash/system/tray/tray_info_label_unittest.cc#newcode117 ash/system/tray/tray_info_label_unittest.cc:117: ClickOnLabel(true /* expect_click_was_handled */); On 2017/07/18 17:26:10, James Cook ...

3 years, 5 months ago (2017-07-18 17:38:08 UTC) #35

lesliewatkins

The CQ bit was checked by lesliewatkins@chromium.org

3 years, 5 months ago (2017-07-18 21:24:49 UTC) #36

lesliewatkins

The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, jamescook@chromium.org Link to ...

3 years, 5 months ago (2017-07-18 21:24:50 UTC) #37

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-18 21:25:07 UTC) #38

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-18 23:14:24 UTC) #39

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-18 23:14:26 UTC) #40

lesliewatkins

The CQ bit was checked by lesliewatkins@chromium.org

3 years, 5 months ago (2017-07-18 23:56:49 UTC) #41

lesliewatkins

The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, jamescook@chromium.org Link to ...

3 years, 5 months ago (2017-07-18 23:56:49 UTC) #42

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-18 23:56:58 UTC) #43

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-19 01:51:26 UTC) #44

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-19 01:51:28 UTC) #45

lesliewatkins

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

3 years, 5 months ago (2017-07-19 19:10:06 UTC) #46

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-19 19:10:24 UTC) #47

lesliewatkins

The CQ bit was unchecked by lesliewatkins@chromium.org

3 years, 5 months ago (2017-07-19 19:15:39 UTC) #48

lesliewatkins

The CQ bit was checked by lesliewatkins@chromium.org

3 years, 5 months ago (2017-07-19 19:15:40 UTC) #49

lesliewatkins

The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, jamescook@chromium.org Link to ...

3 years, 5 months ago (2017-07-19 19:15:41 UTC) #50

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-19 19:15:54 UTC) #51

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1500491740778380, "parent_rev": "1556c052c3473114aec642489e6c72a00def3bf3", "commit_rev": "2257878a0dfb81bbbc37613752865983568580af"}

3 years, 5 months ago (2017-07-19 21:03:59 UTC) #52

commit-bot: I haz the power

Description was changed from ========== Add a row in the network tray to inform users ...

3 years, 5 months ago (2017-07-19 21:04:20 UTC) #53

commit-bot: I haz the power

Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/2257878a0dfb81bbbc37613752865983568580af

3 years, 5 months ago (2017-07-19 21:04:22 UTC) #54

sclittle

A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2978363002/ by sclittle@chromium.org. ...

3 years, 5 months ago (2017-07-19 22:08:08 UTC) #55

findit-for-me

3 years, 5 months ago (2017-07-19 22:19:01 UTC) #56

Message was sent while issue was closed.

Findit (https://goo.gl/kROfz5) confirmed this CL at revision 487975 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

Issue 2957043002: Add a row in the network tray to inform users to turn Bluetooth on to enable Tether. (Closed)

Description

Patch Set 1 #

Patch Set 2 : changed clickable boolean to enum #

Patch Set 3 : added actions and slightly refactored the infolabel class #

Patch Set 4 : Overhaul of previous implementation #

Patch Set 5 : tdanderson@ and khorimoto@ comments #

Patch Set 6 : khorimoto@ and jamescook@ comments #

Patch Set 7 : khorimoto@ comments #

Patch Set 8 : khorimoto@ comments #

Patch Set 9 : khorimoto@ comments #

Patch Set 10 : khorimoto@ comments #

Patch Set 11 : khorimoto@ and jamescook@ comments #

Patch Set 12 : changed message ids in unittests #

Patch Set 13 : changed message ids to actual string names #

Messages