Added a link to the downloads tab that opens the downloads folder.

Added a link to the downloads tab that opens the downloads folder.

Contributed by fhd@ubercode.de

BUG=8915
TEST=chrome://downloads has a link labeled "Open downloads folder" that opens the configured downloads folder.


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

[Randy Smith (Not in Mondays), 2011-07-18 15:13:15]

Adding Ben to the review for the downloads dom handler portion, and Asanka for
general UI evaluation.  Asanka, if you decide this is a good idea, could you
also look into whether we need UI leads approval for this?  (I don't know if we
do or not.)

[asanka, 2011-07-18 17:18:48]

My inclination is to ping the UI leads about this.

Also, as an aside, since we now have other options in the vicinity of "clear
all", we should consider making it ask for confirmation before irreversibly
clearing the downloads list.

http://codereview.chromium.org/7398026/diff/1/chrome/browser/resources/downlo...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/7398026/diff/1/chrome/browser/resources/downlo...
chrome/browser/resources/downloads.js:579: chrome.send('openDownloadsFolder');
return false; or better yet, use event.preventDefault() here to cancel the link
navigation.

http://codereview.chromium.org/7398026/diff/1/chrome/browser/ui/webui/downloa...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/7398026/diff/1/chrome/browser/ui/webui/downloa...
chrome/browser/ui/webui/downloads_dom_handler.cc:237: platform_util::OpenItem(
This runs in the UI thread.  While OpenItem() is safe to call (and must be
called from) the UI thread on Mac, on other platforms it should be called from
the FILE thread.

[asanka, 2011-07-18 18:39:51]

A couple more comments:

I think "Open downloads folder" link might be better if it was to the left of
the "Clear all" link.  This would mirror the relative placement of "show in
folder" and "remove from list" links for the download items.

The layout should also have a consistent tab order.  With this patch, tabbing
through the downloads page results in the focus moving right to left in the
downloads-summary area.

I'll drop a note to the UI leads asking for their input.

[benjhayden, 2011-07-18 19:07:53]

http://codereview.chromium.org/7398026/diff/1/chrome/browser/resources/downlo...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/7398026/diff/1/chrome/browser/resources/downlo...
chrome/browser/resources/downloads.js:636: clearAllLink.oncontextmenu =
openDownloadsFolderLink.oncontextmenu =
Nit: Would you mind separating these assignments?
function returnFalse() { return false; }
clearAllLink.oncontextmenu = returnFalse
openDownloadsFolderLink.oncontextmenu = returnFalse

http://codereview.chromium.org/7398026/diff/1/chrome/browser/ui/webui/downloa...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/7398026/diff/1/chrome/browser/ui/webui/downloa...
chrome/browser/ui/webui/downloads_dom_handler.cc:236: PrefService* pref_service
= web_ui_->GetProfile()->GetPrefs();
Would you mind using download_manager_->download_prefs()->download_path()
instead?

[asanka, 2011-07-19 13:47:10]

We have UI leads' approval for adding the link.

[fhd, 2011-07-19 14:36:56]

On 2011/07/19 13:47:10, asanka wrote:
> We have UI leads' approval for adding the link.

Nice! I will implement all of your and bsh's recommendations regarding the code
quality and upload an update.

What about the other issues? Fixing the focus problem and moving the link to the
moving the link to the right of "Clear all" sounds good to me. Not sure if I
would consider a confirmation for "clear all" necessary, but I'll add one if
you'd prefer that.

[asanka, 2011-07-19 14:51:15]

On 2011/07/19 14:36:56, fhd wrote:
> On 2011/07/19 13:47:10, asanka wrote:
> > We have UI leads' approval for adding the link.
> 
> Nice! I will implement all of your and bsh's recommendations regarding the
code
> quality and upload an update.

Thanks!

> What about the other issues? Fixing the focus problem and moving the link to
the
> moving the link to the right of "Clear all" sounds good to me. Not sure if I
> would consider a confirmation for "clear all" necessary, but I'll add one if
> you'd prefer that.

Confirmation for "Clear all" is a separate issue, so it doesn't need to be
addressed for this CL.

[fhd, 2011-07-19 17:51:16]

On 2011/07/18 17:18:48, asanka wrote:
http://codereview.chromium.org/7398026/diff/1/chrome/browser/ui/webui/downloa...
> chrome/browser/ui/webui/downloads_dom_handler.cc:237: platform_util::OpenItem(
> This runs in the UI thread.  While OpenItem() is safe to call (and must be
> called from) the UI thread on Mac, on other platforms it should be called from
> the FILE thread.

I've fixed this with the new patch set. However, I have found the same logic in
chrome/browser/download/download_item.cc. It would IMHO be nice to have this in
a central place, but I wasn't sure where to put it.

[asanka, 2011-07-19 18:58:00]

Regarding the platform_util::OpenItem() call, I agree that it would be nice to
just call one function and have it dispatch to the FILE thread if necessary. 
But that's not for this CL :)

Can you clean-up the description?  The description for the CL becomes the commit
message when you commit this.

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/resources/dow...
chrome/browser/resources/downloads.html:215: <a id="clear-all" href=""
i18n-content="clear_all">Clear All</a>
Indent nested tags 2 spaces.

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/resources/dow...
chrome/browser/resources/downloads.html:217:
i18n-content="open_downloads_folder">Open downloads folder</a>
I thought we were moving the "Clear All" link to the right.

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/resources/dow...
chrome/browser/resources/downloads.js:631: function returnFalse() { return
false; };
Nit: I'll defer to Ben here, but I think having 'function () { return false; }'
in the two places below is clearer.  We aren't really saving much by defining
this function.

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/ui/webui/down...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/ui/webui/down...
chrome/browser/ui/webui/downloads_dom_handler.cc:21: #include
"chrome/browser/download/download_prefs.h"
download_prefs.h should go before download_util.h

http://codereview.chromium.org/7398026/diff/8001/chrome/browser/ui/webui/down...
chrome/browser/ui/webui/downloads_dom_handler.cc:28: #include
"chrome/common/pref_names.h"
Do you need this anymore?

[fhd, 2011-07-19 19:30:47]

I've fixed the remaining issues (sorry for missing those) and updated the
description. I thought it would be clearer if I removed the remark that we still
need translations for this text, is it sufficient if I mention it in this
comment?

[asanka, 2011-07-19 20:22:49]

On 2011/07/19 19:30:47, fhd wrote:
> I've fixed the remaining issues (sorry for missing those) and updated the
> description. I thought it would be clearer if I removed the remark that we
still
> need translations for this text, is it sufficient if I mention it in this
> comment?

http://dev.chromium.org/developers/design-documents/ui-localization

I believe you don't need to do anything extra for the translations.

[asanka, 2011-07-20 18:53:29]

http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.h (right):

http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.h:75: void
HandleOpenDownloadsFolder(const ListValue* args);
Has to be base::ListValue.

[fhd, 2011-07-20 21:07:12]

Weird, this was the case in patch set 3. Maybe this was introduced without me
noticing during the rebase. In any case, I've compared patch set 3 and 4 and the
rest of the changes are the same. I'll provide another patch set to fix the
issue.

On 2011/07/20 18:53:29, asanka wrote:
>
http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
> File chrome/browser/ui/webui/downloads_dom_handler.h (right):
> 
>
http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
> chrome/browser/ui/webui/downloads_dom_handler.h:75: void
> HandleOpenDownloadsFolder(const ListValue* args);
> Has to be base::ListValue.

[fhd, 2011-07-21 08:39:46]

I have uploaded a new patch set, in case you didn't get a notification from
codereview.chromium.org.

On Jul 20, 2011, at 8:53 PM, asanka@chromium.org wrote:

> 
>
http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
> File chrome/browser/ui/webui/downloads_dom_handler.h (right):
> 
>
http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
> chrome/browser/ui/webui/downloads_dom_handler.h:75: void
> HandleOpenDownloadsFolder(const ListValue* args);
> Has to be base::ListValue.
> 
> http://codereview.chromium.org/7398026/

[Randy Smith (Not in Mondays), 2011-07-21 16:22:29]

On 2011/07/21 08:39:46, fhd wrote:
> I have uploaded a new patch set, in case you didn't get a notification from
> http://codereview.chromium.org.

Just FYI, notifications aren't in general sent out on upload.  That let's the
coder upload several stages of patchset (personal reason for this: separating
merge to TOT from functional changes) and do self-reviews and only engage with
reviwers when they're ready.

> 
> On Jul 20, 2011, at 8:53 PM, mailto:asanka@chromium.org wrote:
> 
> > 
> >
>
http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
> > File chrome/browser/ui/webui/downloads_dom_handler.h (right):
> > 
> >
>
http://codereview.chromium.org/7398026/diff/15001/chrome/browser/ui/webui/dow...
> > chrome/browser/ui/webui/downloads_dom_handler.h:75: void
> > HandleOpenDownloadsFolder(const ListValue* args);
> > Has to be base::ListValue.
> > 
> > http://codereview.chromium.org/7398026/
>

[asanka, 2011-07-21 21:07:58]

LGTM.

arv: Can you take a look?

We'll need OWNERS approval.

[benjhayden, 2011-07-21 21:24:51]

LGTM

[fhd, 2011-07-22 03:42:07]

If you're happy with this, can one of you commit for me? I've added a
contributed by notice to the description.

[asanka, 2011-07-22 21:37:32]

[+estade]

Evan: Can you take a look?

[Evan Stade, 2011-07-25 20:58:09]

lgtm

[Evan Stade, 2011-07-25 20:58:23]

one nit

http://codereview.chromium.org/7398026/diff/16010/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.h (right):

http://codereview.chromium.org/7398026/diff/16010/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.h:74: // Callback for the
"openDownloadsFolder" message - opens the downloads folder
final .

[fhd, 2011-07-26 04:00:26]

Done.

[commit-bot: I haz the power, 2011-07-28 17:39:28]

Change committed as 94493

[achuithb, 2011-11-01 21:53:58]

FYI, this feature is broken on ChromeOS: http://crosbug.com/21585

[fhd, 2011-11-02 04:31:00]

On 2011/11/01 21:53:58, achuith.bhandarkar wrote:
> FYI, this feature is broken on ChromeOS: http://crosbug.com/21585

I get a 403 on that link, can you give me more details? I'm not very familiar
with Chromium OS - is there even a file manager?

[alidudek10, 2011-12-24 21:16:10]

http://codereview.chromium.org/7398026/diff/27001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (left):

http://codereview.chromium.org/7398026/diff/27001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:198: <a id="clear-all" href=""
i18n-content="clear_all">Clear All</a>
file:///C:/Documents%20and%20Settings/pc1/Plocha/adstahovac.exe