Switch default filename retrieval to use GetFielNameToReportUser, which is more authoritative.

Switch default filename retrieval to use GetFielNameToReportUser, which is more authoritative.

R=asanka@chromium.org
BUG=None
TEST=None


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

[Greg Billock, 2012-07-20 20:23:06]

(Note: see 10574002)

Asanka, will this end up giving us names like "filename (1).doc" instead of just
"filename.doc"? If so, I'd like to scrap this and keep it the way it is.

[asanka, 2012-07-20 20:52:52]

On 2012/07/20 20:23:06, Greg Billock wrote:
> (Note: see 10574002)
> 
> Asanka, will this end up giving us names like "filename (1).doc" instead of
just
> "filename.doc"? If so, I'd like to scrap this and keep it the way it is.

DI::GetFileNameToReportUser() will return filenames like "filename (1).doc".
It's basically reporting the filename of the download (unless an explicit
display name has been set).

DI::GetSuggestedFilename() will only be non-empty for downloads initiated by
extensions and downloads initiated by following an anchor tag with a 'download'
attribute.

It sounds like neither is going to give you exactly what you want. If you would
like an ununiquified filename for display purposes, one alternative is to do
something like the following:

  FilePath file_name;
  if (!item->GetForcedFilePath().empty())
    file_name = item->GetForcedFilePath().BaseName();
  else
    download_util::GenerateFileNameFromRequest(*item, &file_name);

This mirrors the filename generation logic in
ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone();

[Randy Smith (Not in Mondays), 2012-07-23 16:00:55]

Greg: How are you using the returned filename?  If there's a use case that we're
not planning for for filename accessors, we should understand it to incorporate
in our interface design.  Display names (use case: Give user a handle to what's
going on) I would think would in general want to have the disambiguator, since
users will want to be able to distinguish between the different files that they
downloaded of the same name.  Can you give me an idea as to why that's not true
in your case?

[Greg Billock, 2012-07-23 18:19:17]

On 2012/07/23 16:00:55, rdsmith wrote:
> Greg: How are you using the returned filename?  If there's a use case that
we're
> not planning for for filename accessors, we should understand it to
incorporate
> in our interface design.  Display names (use case: Give user a handle to
what's
> going on) I would think would in general want to have the disambiguator, since
> users will want to be able to distinguish between the different files that
they
> downloaded of the same name.  Can you give me an idea as to why that's not
true
> in your case?

The use case is when web intents handles the document, we want to forward a
suggested filename which is the actual one used by the file, prior to any
munging needed to unique-ify it for writing to disk.

[Randy Smith (Not in Mondays), 2012-07-23 18:54:35]

On 2012/07/23 18:19:17, Greg Billock wrote:
> On 2012/07/23 16:00:55, rdsmith wrote:
> > Greg: How are you using the returned filename?  If there's a use case that
> we're
> > not planning for for filename accessors, we should understand it to
> incorporate
> > in our interface design.  Display names (use case: Give user a handle to
> what's
> > going on) I would think would in general want to have the disambiguator,
since
> > users will want to be able to distinguish between the different files that
> they
> > downloaded of the same name.  Can you give me an idea as to why that's not
> true
> > in your case?
> 
> The use case is when web intents handles the document, we want to forward a
> suggested filename which is the actual one used by the file, prior to any
> munging needed to unique-ify it for writing to disk.

Help me understand the full user interaction context.  Are you going to leave
the file on disk, or delete it afterwards?  Does the user see the filename in a
list of downloads handled by web intents, or in some other context?

[Greg Billock, 2012-07-23 20:02:42]

On 2012/07/23 18:54:35, rdsmith wrote:
> On 2012/07/23 18:19:17, Greg Billock wrote:
> > On 2012/07/23 16:00:55, rdsmith wrote:
> > > Greg: How are you using the returned filename?  If there's a use case that
> > we're
> > > not planning for for filename accessors, we should understand it to
> > incorporate
> > > in our interface design.  Display names (use case: Give user a handle to
> > what's
> > > going on) I would think would in general want to have the disambiguator,
> since
> > > users will want to be able to distinguish between the different files that
> > they
> > > downloaded of the same name.  Can you give me an idea as to why that's not
> > true
> > > in your case?
> > 
> > The use case is when web intents handles the document, we want to forward a
> > suggested filename which is the actual one used by the file, prior to any
> > munging needed to unique-ify it for writing to disk.
> 
> Help me understand the full user interaction context.  Are you going to leave
> the file on disk, or delete it afterwards?  Does the user see the filename in
a
> list of downloads handled by web intents, or in some other context?

Yes, we leave it on disk.

The use for the filename is that we attach that name to the triggered web intent
to give the service a hint as to the filename the source intended. So it's not
directly given to the user, but the intention is that it's used by services in a
way that will be consumed by the user.

[Randy Smith (Not in Mondays), 2012-07-23 20:38:43]

On 2012/07/23 20:02:42, Greg Billock wrote:
> On 2012/07/23 18:54:35, rdsmith wrote:
> > On 2012/07/23 18:19:17, Greg Billock wrote:
> > > On 2012/07/23 16:00:55, rdsmith wrote:
> > > > Greg: How are you using the returned filename?  If there's a use case
that
> > > we're
> > > > not planning for for filename accessors, we should understand it to
> > > incorporate
> > > > in our interface design.  Display names (use case: Give user a handle to
> > > what's
> > > > going on) I would think would in general want to have the disambiguator,
> > since
> > > > users will want to be able to distinguish between the different files
that
> > > they
> > > > downloaded of the same name.  Can you give me an idea as to why that's
not
> > > true
> > > > in your case?
> > > 
> > > The use case is when web intents handles the document, we want to forward
a
> > > suggested filename which is the actual one used by the file, prior to any
> > > munging needed to unique-ify it for writing to disk.
> > 
> > Help me understand the full user interaction context.  Are you going to
leave
> > the file on disk, or delete it afterwards?  Does the user see the filename
in
> a
> > list of downloads handled by web intents, or in some other context?
> 
> Yes, we leave it on disk.
> 
> The use for the filename is that we attach that name to the triggered web
intent
> to give the service a hint as to the filename the source intended. So it's not
> directly given to the user, but the intention is that it's used by services in
a
> way that will be consumed by the user.

Hmmm.  What would you want to provide if the user was prompted for the filename
(either because something strange about the file system forced the prompt or
because the user's set the "always prompt" flag)?  If you just want what comes
in from the web, we could do that, but it might be violently different from the
filename on disk.  And if you want something like what's used on disk, I'm still
not completely clear as to why you want to avoid the disambiguation characters;
the name will still be meaningful to the user (and arguably more so if they ever
want to find the file on the disk).

[Greg Billock, 2012-07-23 20:52:25]

On 2012/07/23 20:38:43, rdsmith wrote:
> On 2012/07/23 20:02:42, Greg Billock wrote:
> > On 2012/07/23 18:54:35, rdsmith wrote:
> > > On 2012/07/23 18:19:17, Greg Billock wrote:
> > > > On 2012/07/23 16:00:55, rdsmith wrote:
> > > > > Greg: How are you using the returned filename?  If there's a use case
> that
> > > > we're
> > > > > not planning for for filename accessors, we should understand it to
> > > > incorporate
> > > > > in our interface design.  Display names (use case: Give user a handle
to
> > > > what's
> > > > > going on) I would think would in general want to have the
disambiguator,
> > > since
> > > > > users will want to be able to distinguish between the different files
> that
> > > > they
> > > > > downloaded of the same name.  Can you give me an idea as to why that's
> not
> > > > true
> > > > > in your case?
> > > > 
> > > > The use case is when web intents handles the document, we want to
forward
> a
> > > > suggested filename which is the actual one used by the file, prior to
any
> > > > munging needed to unique-ify it for writing to disk.
> > > 
> > > Help me understand the full user interaction context.  Are you going to
> leave
> > > the file on disk, or delete it afterwards?  Does the user see the filename
> in
> > a
> > > list of downloads handled by web intents, or in some other context?
> > 
> > Yes, we leave it on disk.
> > 
> > The use for the filename is that we attach that name to the triggered web
> intent
> > to give the service a hint as to the filename the source intended. So it's
not
> > directly given to the user, but the intention is that it's used by services
in
> a
> > way that will be consumed by the user.
> 
> Hmmm.  What would you want to provide if the user was prompted for the
filename
> (either because something strange about the file system forced the prompt or
> because the user's set the "always prompt" flag)?  If you just want what comes
> in from the web, we could do that, but it might be violently different from
the
> filename on disk.  And if you want something like what's used on disk, I'm
still
> not completely clear as to why you want to avoid the disambiguation
characters;
> the name will still be meaningful to the user (and arguably more so if they
ever
> want to find the file on the disk).

The local filename is a bit useful, but has no relationship to the app they are
using to view the file with (and possibly save or edit it). That app may have
its own munging system.

I'm not opposed to including the munged filename, if it is more reliable and we
don't have the pre-munged version. If I'm picking which one to grab first, and
then fallback to a different way of calculating it, I'd prefer to try to get the
unmunged version and, if it doesn't exist, get the fallback munged/local
version.

[Greg Billock, 2012-07-23 21:42:22]

http://codereview.chromium.org/10802056/diff/1/chrome/browser/download/chrome...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10802056/diff/1/chrome/browser/download/chrome...
chrome/browser/download/chrome_download_manager_delegate.cc:389: string16
filename = item->GetFileNameToReportUser().LossyDisplayName();
So this looks right, then, and is what we decided to use in the discussion?
Shall I just remove the GetSuggestedFilename call as superfluous?

[asanka, 2012-07-23 21:45:43]

http://codereview.chromium.org/10802056/diff/1/chrome/browser/download/chrome...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10802056/diff/1/chrome/browser/download/chrome...
chrome/browser/download/chrome_download_manager_delegate.cc:389: string16
filename = item->GetFileNameToReportUser().LossyDisplayName();
On 2012/07/23 21:42:22, Greg Billock wrote:
> So this looks right, then, and is what we decided to use in the discussion?
> Shall I just remove the GetSuggestedFilename call as superfluous?

Yes. GetFileNameToReportUser() is what we decided to use.

And you can get rid of the GetSuggestedFilename() call.

[Greg Billock, 2012-07-24 20:09:47]

http://codereview.chromium.org/10802056/diff/1/chrome/browser/download/chrome...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10802056/diff/1/chrome/browser/download/chrome...
chrome/browser/download/chrome_download_manager_delegate.cc:389: string16
filename = item->GetFileNameToReportUser().LossyDisplayName();
On 2012/07/23 21:45:43, asanka wrote:
> On 2012/07/23 21:42:22, Greg Billock wrote:
> > So this looks right, then, and is what we decided to use in the discussion?
> > Shall I just remove the GetSuggestedFilename call as superfluous?
> 
> Yes. GetFileNameToReportUser() is what we decided to use.
> 
> And you can get rid of the GetSuggestedFilename() call.

Done.

[asanka, 2012-07-24 20:35:00]

LGTM

[commit-bot: I haz the power, 2012-07-24 22:36:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10802056/10001

[commit-bot: I haz the power, 2012-07-25 00:11:09]

Change committed as 148256