Added tooltips to stats bar items.

Added tooltips to stats bar items.
BUG=chromium-os:5601
(http://code.google.com/p/chromium-os/issues/detail?id=5601)
TEST=Check that hover text over status bar icons seems reasonable. Test different states of network (disconnected, ethernet, connecting to a wifi network, connected to a wifi network) and battery (plugged in, unplugged).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57234

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58526

[stevenjb, 2010-08-16 21:55:08]

I did a first pass of what seemed like reasonable tooltips for the stats bar
items. Feedback welcome (in addition to code review comments of course).

I decided to put the logic for network_name() in network_library.cc instead of
network_menu_button.cc because it seemed like it might be useful elsewhere, and
that the precedence logic belonged there instead of in the UI, but I could
easily be persuaded to move it to network_menu_button.cc.

[Charlie Lee, 2010-08-17 16:56:33]

I think you should move the networks_name logic to network_menu_button. The
tooltip should match the icon we are showing. And that logic is in
network_menu_button. So in network_menu_button, wherever you see a SetIcon()
call, you should have corresponding SetToolTip() call.

http://codereview.chromium.org/3158014/diff/1/3
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/3158014/diff/1/3#newcode265
chrome/browser/chromeos/cros/network_library.cc:265:
IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
This message is "No networks are available"
Is that what we want?

http://codereview.chromium.org/3158014/diff/1/3#newcode964
chrome/browser/chromeos/cros/network_library.cc:964: virtual std::string
network_name() const { return std::string(); }
Do you need to add this to mock_network_library?

http://codereview.chromium.org/3158014/diff/1/7
File chrome/browser/chromeos/status/language_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/7#newcode141
chrome/browser/chromeos/status/language_menu_button.cc:141:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_LANGUAGE_TOOLTIP));
Should the tooltip show what language/input_method we are currently using?

http://codereview.chromium.org/3158014/diff/1/8
File chrome/browser/chromeos/status/network_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/8#newcode240
chrome/browser/chromeos/status/network_menu_button.cc:240: cros->wifi_name() :
cros->cellular_name();
not using this variable.

http://codereview.chromium.org/3158014/diff/1/8#newcode275
chrome/browser/chromeos/status/network_menu_button.cc:275:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
same comment about "no available networks."

[stevenjb, 2010-08-17 17:11:52]

OK, I'll combine the tooltip logic with the icon logic.

http://codereview.chromium.org/3158014/diff/1/3
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/3158014/diff/1/3#newcode265
chrome/browser/chromeos/cros/network_library.cc:265:
IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
On 2010/08/17 16:56:34, Charlie Lee wrote:
> This message is "No networks are available"
> Is that what we want?
Fair point, this would probably be better with a new message, "No Network"?

http://codereview.chromium.org/3158014/diff/1/7
File chrome/browser/chromeos/status/language_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/7#newcode141
chrome/browser/chromeos/status/language_menu_button.cc:141:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_LANGUAGE_TOOLTIP));
On 2010/08/17 16:56:34, Charlie Lee wrote:
> Should the tooltip show what language/input_method we are currently using?
Do we have more descriptive information for the language than what is already
shown in the menu? I'll ask someone on the i18n team when I'm in MTV on Thurs.

http://codereview.chromium.org/3158014/diff/1/8
File chrome/browser/chromeos/status/network_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/8#newcode240
chrome/browser/chromeos/status/network_menu_button.cc:240: cros->wifi_name() :
cros->cellular_name();
On 2010/08/17 16:56:34, Charlie Lee wrote:
> not using this variable.
oops. thanks.

http://codereview.chromium.org/3158014/diff/1/8#newcode275
chrome/browser/chromeos/status/network_menu_button.cc:275:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
On 2010/08/17 16:56:34, Charlie Lee wrote:
> same comment about "no available networks."
Will make sure it is consistent.

[oshima, 2010-08-17 22:00:23]

+satorux for language menu tooltips

[DaveMoore, 2010-08-18 14:51:05]

http://codereview.chromium.org/3158014/diff/1/3
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/3158014/diff/1/3#newcode254
chrome/browser/chromeos/cros/network_library.cc:254: || (ethernet_.connecting()
Most chrome code puts the operator at the end of the first line instead of the
beginning of the second when splitting.

http://codereview.chromium.org/3158014/diff/1/5
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/5#newcode84
chrome/browser/chromeos/status/clock_menu_button.cc:84:
SetTooltipText(base::TimeFormatShortDate(base::Time::Now()));
You should get the time once and reuse it in both the SetText() and
SetTooltipText() calls to ensure it matches.

http://codereview.chromium.org/3158014/diff/1/9
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/9#newcode176
chrome/browser/chromeos/status/power_menu_button.cc:176:
SetTooltipText(UTF16ToWide(GetLabelAt(0)));
We shouldn't have both tooltip text and a menu that simply match. Can we do away
with the menu?

[stevenjb, 2010-08-18 16:33:53]

Some changes based on feedback. Still need to get i18n / product team feedback.

http://codereview.chromium.org/3158014/diff/1/3
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/3158014/diff/1/3#newcode254
chrome/browser/chromeos/cros/network_library.cc:254: || (ethernet_.connecting()
On 2010/08/18 14:51:05, davemoore wrote:
> Most chrome code puts the operator at the end of the first line instead of the
> beginning of the second when splitting.
Noted, but re-factored this anyway, per Charlie's comments.

http://codereview.chromium.org/3158014/diff/1/3#newcode265
chrome/browser/chromeos/cros/network_library.cc:265:
IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
On 2010/08/17 17:11:52, Steven Bennetts wrote:
> On 2010/08/17 16:56:34, Charlie Lee wrote:
> > This message is "No networks are available"
> > Is that what we want?
> Fair point, this would probably be better with a new message, "No Network"?
> 

Done.

http://codereview.chromium.org/3158014/diff/1/3#newcode964
chrome/browser/chromeos/cros/network_library.cc:964: virtual std::string
network_name() const { return std::string(); }
On 2010/08/17 16:56:34, Charlie Lee wrote:
> Do you need to add this to mock_network_library?
Removed.

http://codereview.chromium.org/3158014/diff/1/5
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/5#newcode84
chrome/browser/chromeos/status/clock_menu_button.cc:84:
SetTooltipText(base::TimeFormatShortDate(base::Time::Now()));
On 2010/08/18 14:51:05, davemoore wrote:
> You should get the time once and reuse it in both the SetText() and
> SetTooltipText() calls to ensure it matches.

Done.

http://codereview.chromium.org/3158014/diff/1/8
File chrome/browser/chromeos/status/network_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/8#newcode240
chrome/browser/chromeos/status/network_menu_button.cc:240: cros->wifi_name() :
cros->cellular_name();
On 2010/08/17 17:11:52, Steven Bennetts wrote:
> On 2010/08/17 16:56:34, Charlie Lee wrote:
> > not using this variable.
> oops. thanks.
Removed network_name(), so using it now.

http://codereview.chromium.org/3158014/diff/1/8#newcode275
chrome/browser/chromeos/status/network_menu_button.cc:275:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
On 2010/08/17 17:11:52, Steven Bennetts wrote:
> On 2010/08/17 16:56:34, Charlie Lee wrote:
> > same comment about "no available networks."
> Will make sure it is consistent.
> 

Done.

http://codereview.chromium.org/3158014/diff/1/9
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/1/9#newcode176
chrome/browser/chromeos/status/power_menu_button.cc:176:
SetTooltipText(UTF16ToWide(GetLabelAt(0)));
On 2010/08/18 14:51:05, davemoore wrote:
> We shouldn't have both tooltip text and a menu that simply match. Can we do
away
> with the menu?

I hate to rely on waiting for a tooltip for information, but don't have any
better suggestions for the tooltip. I will run it by Kan to get his feedback.

[satorux1, 2010-08-19 02:17:33]

http://codereview.chromium.org/3158014/diff/12001/13001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3158014/diff/12001/13001#newcode8826
chrome/app/generated_resources.grd:8826: Language
Please remove this.

http://codereview.chromium.org/3158014/diff/12001/13004
File chrome/browser/chromeos/status/language_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/12001/13004#newcode143
chrome/browser/chromeos/status/language_menu_button.cc:143:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_LANGUAGE_TOOLTIP));
Please remove this. We set the tool tip text based on the current input method
in LanguageMenuButton::UpdateIndicator().

[stevenjb, 2010-08-20 22:53:56]

Fixes based on feedback completed.

Only remaining issue is what to do for the power tooltip / menu. Leave it as is,
or change the tooltip and/or remove the menu?

http://codereview.chromium.org/3158014/diff/12001/13004
File chrome/browser/chromeos/status/language_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/12001/13004#newcode143
chrome/browser/chromeos/status/language_menu_button.cc:143:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_LANGUAGE_TOOLTIP));
On 2010/08/19 02:17:33, satorux1 wrote:
> Please remove this. We set the tool tip text based on the current input method
> in LanguageMenuButton::UpdateIndicator().

Ugh, can't believe I missed that, sorry. Reverted my changes.

[satorux1, 2010-08-20 22:58:53]

http://codereview.chromium.org/3158014/diff/12001/13004
File chrome/browser/chromeos/status/language_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/12001/13004#newcode143
chrome/browser/chromeos/status/language_menu_button.cc:143:
SetTooltipText(l10n_util::GetString(IDS_STATUSBAR_LANGUAGE_TOOLTIP));
On 2010/08/20 22:53:56, Steven Bennetts wrote:
> On 2010/08/19 02:17:33, satorux1 wrote:
> > Please remove this. We set the tool tip text based on the current input
method
> > in LanguageMenuButton::UpdateIndicator().
> 
> Ugh, can't believe I missed that, sorry. Reverted my changes.
> 
> 

Thanks. Looks good to me. :)

[DaveMoore, 2010-08-24 15:34:07]

LGTM

[stevenjb, 2010-08-25 20:04:21]

I failed to notice that the changes to power_menu_button.cc caused many tests to
fail. 

In the process of trying to fix the tests (and learning about gmock in the
process), I re-factored the way that PowerMenuButton updates the icon and builds
the menu to make it more predictable for setting up gmock tests. If these
changes are undesirable I believe I now understand gmock well enough to make a
smaller set of incremental changes that would fix the tests, but I believe the
changes here will make the code easier to maintain.

[DaveMoore, 2010-08-31 16:07:00]

http://codereview.chromium.org/3158014/diff/29001/30007
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/29001/30007#newcode201
chrome/browser/chromeos/status/power_menu_button.cc:201:
labels_.push_back(l10n_util::GetStringFUTF16(msg, hour_str, min_str));
There is no need to store an array of the strings. GetStringFUTF16() is plenty
fast. And then you don't have to worry about language changes.

[stevenjb, 2010-09-01 01:27:33]

http://codereview.chromium.org/3158014/diff/29001/30007
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/3158014/diff/29001/30007#newcode201
chrome/browser/chromeos/status/power_menu_button.cc:201:
labels_.push_back(l10n_util::GetStringFUTF16(msg, hour_str, min_str));
On 2010/08/31 16:07:01, davemoore wrote:
> There is no need to store an array of the strings. GetStringFUTF16() is plenty
> fast. And then you don't have to worry about language changes.
Good point about the language change edge case. Re-factored to put the logic
back into GetLabelAt(); storing the libcros results instead.

[DaveMoore, 2010-09-02 14:56:52]

LGTM