Added download error descriptions to tooltips for Mac & Linux.

Added download error descriptions to tooltips for Mac & Linux.

This will allow users to find out more about the error that caused a download interruption.

BUG=121698
TEST=None

[ahendrickson, 2012-04-03 19:18:46]

Asanka, could you take a quick look at this code?

[ahendrickson, 2012-04-04 16:07:20]

PTAL.

[sky, 2012-04-04 16:55:34]

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:794:
MessageLoop::current()->PostDelayedTask(
Don't post tasks of arbitrary length. It makes for flaky and long running tests.
Instead listen for whatever it is you need to then exit when you receive the
event.

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_item_model.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_item_model.cc:43: int64 total= 0;
nit: 'total ='

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:90: bool GetStatusData(int64*
size_return,
Add a description of this, including what the return value means.

[asanka, 2012-04-04 17:27:03]

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:35: // Get the progress text to
display.
Consider adding some example text here.

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
chrome/browser/ui/gtk/download/download_item_gtk.cc:484:
get_download()->GetFileNameToReportUser(), gfx::Font(), kTooltipMaxWidth);
Can we move the tooltip generation logic into the model?

[Robert Sesek, 2012-04-04 17:27:40]

+thakis, who is more familiar with this piece of code

[ahendrickson, 2012-04-04 17:49:31]

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:794:
MessageLoop::current()->PostDelayedTask(
On 2012/04/04 16:55:34, sky wrote:
> Don't post tasks of arbitrary length. It makes for flaky and long running
tests.
> Instead listen for whatever it is you need to then exit when you receive the
> event.

Oh, sorry, I intended to remove this.  It was only to see if I could see the
tooltips during a browser test (I couldn't).

Removed.

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_item_model.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_item_model.cc:43: int64 total= 0;
On 2012/04/04 16:55:34, sky wrote:
> nit: 'total ='

Done.

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
File chrome/browser/download/download_item_model.h (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:35: // Get the progress text to
display.
On 2012/04/04 17:27:04, asanka wrote:
> Consider adding some example text here.

Done.

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/download/down...
chrome/browser/download/download_item_model.h:90: bool GetStatusData(int64*
size_return,
On 2012/04/04 16:55:34, sky wrote:
> Add a description of this, including what the return value means.

Done.

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
chrome/browser/ui/gtk/download/download_item_gtk.cc:484:
get_download()->GetFileNameToReportUser(), gfx::Font(), kTooltipMaxWidth);
On 2012/04/04 17:27:04, asanka wrote:
> Can we move the tooltip generation logic into the model?

The logic is different on Windows vs Posix.

[Nico, 2012-04-04 17:50:06]

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
chrome/browser/ui/gtk/download/download_item_gtk.cc:498: &elidedMessages);
Hm, maybe that should be on the downloads page instead?

From
http://developer.apple.com/library/mac/#documentation/UserExperience/Conceptu...
:

"""
The text of a help tag should briefly describe what an interface element does.
If you find that you need more than a few words to describe the function of a
control, you might want to reconsider the design of your application’s user
interface.

You can define help tags in Interface Builder, where they are called tooltips.
Here are some guidelines to help you create effective help tag messages.

In general, don’t name the interface element in the tag. Because a help tag is
specific to a UI element, it should not be necessary to refer to by name, unless
the name helps the user and isn’t available onscreen. If you do need to refer to
an element by name, make sure you use the same name throughout all of your
documentation.

Describe only the element that the pointer is resting on. Users expect a help
tag to describe what they can do with the control; they do not expect to read
about other controls or about how to perform a larger task.

Describe controls that are unique to your application. Don’t provide help tags
that describe window resize controls, scrollers, or other standard Aqua
controls.

Focus on the action users can perform using the control. A good way to stay
focused on the action is to begin the tag with a verb, for example, “Restores
default settings” or “Add or remove a language from the list”.

Use the fewest words possible. Try to keep your tags to a maximum of 60 to 75
characters. Because help tags are always on, it is important to keep your tag
text unobtrusive—that is, short—and useful. A tag should present only one
concept and that concept should be directly related to the interface element.
You can also omit articles to limit the length of the tag. Note that
localization can lengthen the text by 20 to 30 percent, which is another good
reason to keep the tag short.
"""

[sky, 2012-04-04 20:46:59]

LGTM

[asanka, 2012-04-05 19:08:34]

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
chrome/browser/ui/gtk/download/download_item_gtk.cc:498: &elidedMessages);
On 2012/04/04 17:50:06, Nico wrote:
> Hm, maybe that should be on the downloads page instead?

This has changed in the latest patch set. Part of the problem was that the
status message was too large to be contained within the confines of the download
item. So an interrupted download might show up on the download shelf as:

  | [ icon ] possibly long f...exe
  |          Insufficient disk sp...

And with this CL, the tooltip looks like:

  | possibly long filename.exe
  | Insufficient disk space

While not consistent with the guidelines for a help tag on Mac, wouldn't this
use still be consistent with the use of tooltips to disclose otherwise truncated
text?

> From
>
http://developer.apple.com/library/mac/#documentation/UserExperience/Conceptu...
> :
> 
> """
> The text of a help tag should briefly describe what an interface element does.
> If you find that you need more than a few words to describe the function of a
> control, you might want to reconsider the design of your application’s user
> interface.
> ....
> """

[ahendrickson, 2012-04-06 15:46:01]

Nico: Ping.

On 2012/04/05 19:08:34, asanka wrote:
>
http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
> File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):
> 
>
http://codereview.chromium.org/9968090/diff/9010/chrome/browser/ui/gtk/downlo...
> chrome/browser/ui/gtk/download/download_item_gtk.cc:498: &elidedMessages);
> On 2012/04/04 17:50:06, Nico wrote:
> > Hm, maybe that should be on the downloads page instead?
> 
> This has changed in the latest patch set. Part of the problem was that the
> status message was too large to be contained within the confines of the
download
> item. So an interrupted download might show up on the download shelf as:
> 
>   | [ icon ] possibly long f...exe
>   |          Insufficient disk sp...
> 
> And with this CL, the tooltip looks like:
> 
>   | possibly long filename.exe
>   | Insufficient disk space
> 
> While not consistent with the guidelines for a help tag on Mac, wouldn't this
> use still be consistent with the use of tooltips to disclose otherwise
truncated
> text?
> 
> > From
> >
>
http://developer.apple.com/library/mac/#documentation/UserExperience/Conceptu...
> > :
> > 
> > """
> > The text of a help tag should briefly describe what an interface element
does.
> > If you find that you need more than a few words to describe the function of
a
> > control, you might want to reconsider the design of your application’s user
> > interface.
> > ....
> > """

[Nico, 2012-04-06 16:14:05]

Patch set 11 LGTM, with 1 minor comment (that applies to 3 files). Thanks!

http://codereview.chromium.org/9968090/diff/24001/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (right):

http://codereview.chromium.org/9968090/diff/24001/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_item_controller.mm:253: DCHECK(reason
!= 0);
DCHECK_NE(reason, DOWNLOAD_INTERRUPT_REASON_NONE);

Also in the other files.

[asanka, 2012-04-23 18:29:29]

This is now http://codereview.chromium.org/10169025/

[ahendrickson, 2012-04-23 23:19:33]

On 2012/04/23 18:29:29, asanka wrote:
> This is now http://codereview.chromium.org/10169025/

Thanks for taking this!

[sky, 2012-04-23 23:43:15]

LGTM