Detect removed files and reflect the state in chrome://downloads and the download shelf

Detect removed files and reflect the state in chrome://downloads and the download shelf
- Invalidate the links of removed files in chrome://downloads page
- Label "Removed" for the removed files in the download shelf

BUG=29093
TEST=Observe that in chrome://downloads, the downloaded and then removed files are labeled "Removed" and have no links, Observe that in the download shelf, the downloaded and then removed files are labeled "Removed", DownloadManagerTest.*


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

[haraken1, 2011-04-27 01:53:52]

I am trying to fix the bug 29093. I fixed so that the downloaded but removed
files are labeled "Removed" and have no file links in chrome://downloads page. 

I found your name near the code I modified. Would you take a look at it?

[Paweł Hajdan Jr., 2011-04-27 08:53:30]

Drive-by with download comments.

http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_util.cc:667: bool path_exists = true;
It seems like you're checking whether a path exists. Why don't you use one of
the functions from base/file_util?

http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_util.cc:676: #else
A "dangling else" like that is generally dangerous for portable code. The #if
will probably disappear, but a general tip is please put a NOTIMPLEMENTED(), or
#error in such an else to prevent a _silent_ failure on (hypothetical) platforms
not covered by OS_WIN and OS_POSIX.

http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_util.cc:678: if (path_exists) {
nit: It's probably cleaner to use a ternary operator like SetString("state",
path_exists ? "COMPLETE" : "REMOVED");

[ahendrickson, 2011-04-27 15:37:47]

Added mmenke as a reviewer for chrome/app/generated_resources.grd

[mmenke, 2011-04-27 16:02:10]

http://codereview.chromium.org/6905049/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6905049/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:2214: desc="Remove link text">
I know you're just following the style of the entry above, but you should be a
bit clearer in your description, since (I believe) it's all translators are
given to work with.

I don't think "Remove link text" is at all clear, since it's not actually a
link, and it doesn't remove anything.  Instead, it's text that appears next
downloaded files that have been removed.  Say something along those lines.

It's be great if you could update "Cancel link text" in a similar manner while
you're at it.

[ahendrickson, 2011-04-27 17:30:56]

You also need to update the download item on the download shelf.

That's going to be more complicated, and will involve adding a new state to
DownloadItem::DownloadState.

[haraken1, 2011-04-27 17:44:35]

>
http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
> File chrome/browser/download/download_util.cc (right):
> 
>
http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_util.cc:667: bool path_exists = true;
> It seems like you're checking whether a path exists. Why don't you use one of
> the functions from base/file_util?

I first thought it but didn't because the functions in base/file_util can be
called only from the threads with base::ThreadRestrictions::SetIOAllowed(true).
Thus, here we cannot directly use the functions in base/file_util. 

The first possible way is not using the functions in base/file_util like the
code that I wrote. The second possible way is setting
base::ThreadRestrictions::SetIOAllowed(true) temporarily and then using the
functions in base/file_util. I think the first way is clearer, but what do you
think?

>
http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_util.cc:676: #else
> A "dangling else" like that is generally dangerous for portable code. The #if
> will probably disappear, but a general tip is please put a NOTIMPLEMENTED(),
or
> #error in such an else to prevent a _silent_ failure on (hypothetical)
platforms
> not covered by OS_WIN and OS_POSIX.

I fixed it. 

>
http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_util.cc:678: if (path_exists) {
> nit: It's probably cleaner to use a ternary operator like SetString("state",
> path_exists ? "COMPLETE" : "REMOVED");

I fixed it.

[haraken1, 2011-04-27 17:45:26]

>
http://codereview.chromium.org/6905049/diff/1/chrome/app/generated_resources.grd
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/6905049/diff/1/chrome/app/generated_resources....
> chrome/app/generated_resources.grd:2214: desc="Remove link text">
> I know you're just following the style of the entry above, but you should be a
> bit clearer in your description, since (I believe) it's all translators are
> given to work with.
> 
> I don't think "Remove link text" is at all clear, since it's not actually a
> link, and it doesn't remove anything.  Instead, it's text that appears next
> downloaded files that have been removed.  Say something along those lines.

I fixed it. 

> It's be great if you could update "Cancel link text" in a similar manner while
> you're at it.

I also fixed it.

[haraken1, 2011-04-27 17:46:18]

> You also need to update the download item on the download shelf.
> That's going to be more complicated, and will involve adding a new state to
> DownloadItem::DownloadState.

I see. I'll try. Thanks!

[Randy Smith (Not in Mondays), 2011-04-27 18:21:21]

I'm afraid I have some intrusive drive-by comments.  First of all, keep in mind
that no matter what we do, we have to deal with the possibility of a race where
we think that the file is there when it isn't (because the user races with us on
deleting the file as we attempt to traverse the link).  So a change like this,
while it may be useful for reflecting state from the file system, isn't going to
deal with the whole problem.  As a result, I've been generally opposed to trying
to track file system changes that aren't caused by actions in Chrome; I'd rather
try and make sure that Chrome handles operations correctly when it figures out
that the internal state it's been tracking is incorrect.

Second of all, as mentioned below, we aren't allowed to access the file system
on the UI thread, and I'm pretty sure that's what this patch does.  So if we do
want to do this, we need to find a different approach (specifically, we need to
probe for FS state at a different point than when the HTML probes the DOM for
the download state, so that that state is already present in the DownloadItem
when CreateDownloadItemValue is called).

Third of all, I'm resistant to the idea of adding a new state to
DownloadItem::DownloadState, just because it'll make the code somewhat more
messy; adding states to state variables is dangerous because it increases the
number of different cases to be considered at many different locations in the
program.  I won't say "You can't do that", but I'd like to understand the
motivation for the change and believe there's real value for the users before I
let it in.

Looking at the bug, my inclination would be to change the thrust of the bug to
"Chrome should detect the deletion of a download file when an action on that
file is requested from the browser".  Would you object to that?  More generally,
what's your motivation for this change?

http://codereview.chromium.org/6905049/diff/1006/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/6905049/diff/1006/chrome/browser/download/down...
chrome/browser/download/download_util.cc:679: file_value->SetString("state",
path_exists ? "COMPLETE" : "REMOVED");
All file system accesses need to happen on the FILE thread. 
CreateDownloadItemValue isn't restricted to the file thread, and I believe (from
a cursory scan of the call sites) is usually called on the UI thread.  So this
won't work.

[mmenke, 2011-04-27 18:25:08]

On 2011/04/27 17:44:35, haraken wrote:
> >
>
http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
> > File chrome/browser/download/download_util.cc (right):
> > 
> >
>
http://codereview.chromium.org/6905049/diff/1/chrome/browser/download/downloa...
> > chrome/browser/download/download_util.cc:667: bool path_exists = true;
> > It seems like you're checking whether a path exists. Why don't you use one
of
> > the functions from base/file_util?
> 
> I first thought it but didn't because the functions in base/file_util can be
> called only from the threads with
base::ThreadRestrictions::SetIOAllowed(true).
> Thus, here we cannot directly use the functions in base/file_util. 
> 
> The first possible way is not using the functions in base/file_util like the
> code that I wrote. The second possible way is setting
> base::ThreadRestrictions::SetIOAllowed(true) temporarily and then using the
> functions in base/file_util. I think the first way is clearer, but what do you
> think?

Paweł may have a different opinion, but I'd say the second way is far
preferable.  It both avoids an extra dependency on OS version, and helps anyone
looking to get additional file I/O off of the UI thread find it.

[mmenke, 2011-04-27 18:29:31]

I defer to Randy on this issue.

[Avi (use Gerrit), 2011-04-27 18:35:42]

On 2011/04/27 17:44:35, haraken wrote:
> I first thought it but didn't because the functions in base/file_util can be
> called only from the threads with
base::ThreadRestrictions::SetIOAllowed(true).
> Thus, here we cannot directly use the functions in base/file_util. 

You're not allowed to pave a new road from A to B to avoid the speedbumps that
are on the existing road from A to B. The speedbumps are there for a reason.

> The first possible way is not using the functions in base/file_util like the
> code that I wrote. The second possible way is setting
> base::ThreadRestrictions::SetIOAllowed(true) temporarily and then using the
> functions in base/file_util. I think the first way is clearer, but what do you
> think?

Neither is allowed.

Randy is right; listen to him.

[mmenke, 2011-04-27 19:13:23]

On 2011/04/27 18:21:21, rdsmith wrote:
> More generally, what's your motivation for this change?

While I can't speak to haraken's motivation, or that of the original bug
reporter, it bugs me that months after you've downloaded and deleted a file,
it's still listed on the download shelf with two dead links (open it and show in
folder).

I think that the current behavior is fine for files deleted since the page was
opened (Or other relatively recently deleted files, for that matter), but if I
open up the download page, and a file was deleted some time back, it would be
great if that page somehow reflected that.

[Randy Smith (Not in Mondays), 2011-04-27 20:06:08]

On 2011/04/27 19:13:23, Matt Menke wrote:
> While I can't speak to haraken's motivation, or that of the original bug
> reporter, it bugs me that months after you've downloaded and deleted a file,
> it's still listed on the download shelf with two dead links (open it and show
in
> folder).
> 
> I think that the current behavior is fine for files deleted since the page was
> opened (Or other relatively recently deleted files, for that matter), but if I
> open up the download page, and a file was deleted some time back, it would be
> great if that page somehow reflected that.

That makes a great deal of sense to me.  The way I'd like to solve it is to have
"probe points" in the code where messages are sent to the file thread to figure
out the state of the download, and the response updates the information in the
DownloadItem, and the download shelf and download page just reflect the current
state in the download item without trying to validate it, *and* we make sure
that when we have the state wrong, and attempt to open the file or show it in
the folder, we detect the race loss at that point and let the user know "oops,
not there anymore" at that point.  I'd be inclined to make probe points "browser
start" (in download context, OnQueryDownloadEntriesComplete) and whenever we
show the download shelf, but I could certainly be talked into others/out of
those.

Depending on what haraken wants to do with this bug, I'll file any new bugs
needed to cpature this behavior.

[mmenke, 2011-04-27 20:20:24]

On 2011/04/27 20:06:08, rdsmith wrote:
> That makes a great deal of sense to me.  The way I'd like to solve it is to
have
> "probe points" in the code where messages are sent to the file thread to
figure
> out the state of the download, and the response updates the information in the
> DownloadItem, and the download shelf and download page just reflect the
current
> state in the download item without trying to validate it, *and* we make sure
> that when we have the state wrong, and attempt to open the file or show it in
> the folder, we detect the race loss at that point and let the user know "oops,
> not there anymore" at that point.  I'd be inclined to make probe points
"browser
> start" (in download context, OnQueryDownloadEntriesComplete) and whenever we
> show the download shelf, but I could certainly be talked into others/out of
> those.
> 
> Depending on what haraken wants to do with this bug, I'll file any new bugs
> needed to cpature this behavior.

That sounds quite reasonable, though I think a couple minutes after browser
start would probably be better - a lot of things happen on the file thread on
browser start, and I think it's best not to add to them when not necessary.

[haraken1, 2011-04-27 23:11:14]

Randy wrote:
>> Looking at the bug, my inclination would be to change the thrust of the bug
to
>> "Chrome should detect the deletion of a download file when an action on that
>> file is requested from the browser".  Would you object to that?  More
generally,
>> what's your motivation for this change?

My motivation is just the one that Menke mentioned below. 

Menke wrote:
> While I can't speak to haraken's motivation, or that of the original bug
> reporter, it bugs me that months after you've downloaded and deleted a file,
> it's still listed on the download shelf with two dead links (open it and show
in
> folder).
> 
> I think that the current behavior is fine for files deleted since the page was
> opened (Or other relatively recently deleted files, for that matter), but if I
> open up the download page, and a file was deleted some time back, it would be
> great if that page somehow reflected that.


Randy wrote:
> That makes a great deal of sense to me.  The way I'd like to solve it is to
have
> "probe points" in the code where messages are sent to the file thread to
figure
> out the state of the download, and the response updates the information in the
> DownloadItem, and the download shelf and download page just reflect the
current
> state in the download item without trying to validate it, *and* we make sure
> that when we have the state wrong, and attempt to open the file or show it in
> the folder, we detect the race loss at that point and let the user know "oops,
> not there anymore" at that point.  I'd be inclined to make probe points
"browser
> start" (in download context, OnQueryDownloadEntriesComplete) and whenever we
> show the download shelf, but I could certainly be talked into others/out of
> those.

It makes a great sense to me. I will try the above approach. Thanks.

[Paweł Hajdan Jr., 2011-04-29 08:43:41]

http://codereview.chromium.org/6905049/diff/1006/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/6905049/diff/1006/chrome/browser/download/down...
chrome/browser/download/download_util.cc:679: file_value->SetString("state",
path_exists ? "COMPLETE" : "REMOVED");
On 2011/04/27 18:21:21, rdsmith wrote:
> All file system accesses need to happen on the FILE thread. 
> CreateDownloadItemValue isn't restricted to the file thread, and I believe
(from
> a cursory scan of the call sites) is usually called on the UI thread.  So this
> won't work.

Exactly. And using SetAllow... or re-implementing the code is really just
bypassing checks intended to catch blocking IO on wrong threads.

Please note we also cannot jump through the FILE thread in a way that would make
the UI thread block or wait on the FILE thread.

I think the check for file existence should be done asynchronously.

[haraken1, 2011-05-10 07:41:53]

Thank you very much for the dedicated advice.

Randy wrote:
> That makes a great deal of sense to me.  The way I'd like to solve it is to
> have
> "probe points" in the code where messages are sent to the file thread to
> figure
> out the state of the download, and the response updates the information in
> the
> DownloadItem, and the download shelf and download page just reflect the
> current
> state in the download item without trying to validate it, *and* we make sure
> that when we have the state wrong, and attempt to open the file or show it
> in
> the folder, we detect the race loss at that point and let the user know
> "oops,
> not there anymore" at that point.  I'd be inclined to make probe points
> "browser
> start" (in download context, OnQueryDownloadEntriesComplete) and whenever we
> show the download shelf, but I could certainly be talked into others/out of
> those.

For the time being, I implemented the feature for chrome://downloads
page (I did not yet implemented the feature for the download shelf).

- I added a new member |is_path_exists| to DownloadItem, which
indicates whether the download item still exists or not in a download
folder.
- Whenever the chrome://downloads page is loaded (in
DownloadsDOMHandler::HandleGetDownloads()), whether the path of each
download item still exists is checked on the FILE thread (In other
words, here is the "probe point").
- If some download item is found to be already non-existent by the
FILE thread, the fact is soon observed by the UIthread and the label
of the download item in chrome://downloads is changed to "Removed".

Would you review the code?

[Paweł Hajdan Jr., 2011-05-10 07:59:46]

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:262: bool is_path_exists() const {
return is_path_exists_; }
nit: is_path_exists is a bad name. Why not just file_exists()? Here and in the
member variable.

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:263: void set_is_path_exists(bool
is_path_exists) {
We generally avoid raw setters like this. How about OnFileRemovalDetected that
sets the flag to false?

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:239: void CheckExistingPaths();
nit: Would CheckForFilesRemoval (or similar) be a better name?
CheckExistingPaths suggests we do some verification of _existing_ paths, but
here we don't know whether they exist, and we're checking for their existence.

Also, this method not just checks, it updates the state of the downloads item.
And also we do that to display an indication of removed files in the UI. Please
make sure to update the comment.

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:240: void
CheckExistingPathsOnFileThread(PathVector existing_paths);
nit: Why not const PathVector&, here and below?

Also, only CheckExistingPaths needs to be public, please make other two methods
private and add comments.

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/10001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:153: (*it)->AddObserver(this);
This AddObserver logic getting scattered across even more methods can quickly
get harder to follow. How about just observing all downloads? Have you tried
that?

[haraken1, 2011-05-10 09:53:36]

>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
> File chrome/browser/download/download_item.h (right):
>
>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
> chrome/browser/download/download_item.h:262: bool is_path_exists() const
> { return is_path_exists_; }
> nit: is_path_exists is a bad name. Why not just file_exists()? Here and
> in the member variable.

I fixed it.

>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
> chrome/browser/download/download_item.h:263: void
> set_is_path_exists(bool is_path_exists) {
> We generally avoid raw setters like this. How about
> OnFileRemovalDetected that sets the flag to false?

I wrote the setter just because currently all members in DownloadItem
class are private and getter and setter functions are defined to
access these variables. Do you mean that we should make |file_exists_|
a public variable? I did not yet fix this, but I will fix this if it
is our convention.

>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
> File chrome/browser/download/download_manager.h (right):
>
>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
> chrome/browser/download/download_manager.h:239: void
> CheckExistingPaths();
> nit: Would CheckForFilesRemoval (or similar) be a better name?
> CheckExistingPaths suggests we do some verification of _existing_ paths,
> but here we don't know whether they exist, and we're checking for their
> existence.
>
> Also, this method not just checks, it updates the state of the downloads
> item. And also we do that to display an indication of removed files in
> the UI. Please make sure to update the comment.

I fixed it. I added the comments in download_manager.h.

>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/download/dow...
> chrome/browser/download/download_manager.h:240: void
> CheckExistingPathsOnFileThread(PathVector existing_paths);
> nit: Why not const PathVector&, here and below?

I fixed it.

> Also, only CheckExistingPaths needs to be public, please make other two
> methods private and add comments.

I fixed it.

>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/ui/webui/dow...
> File chrome/browser/ui/webui/downloads_dom_handler.cc (right):
>
>
http://codereview.chromium.org/6905049/diff/10001/chrome/browser/ui/webui/dow...
> chrome/browser/ui/webui/downloads_dom_handler.cc:153:
> (*it)->AddObserver(this);
> This AddObserver logic getting scattered across even more methods can
> quickly get harder to follow. How about just observing all downloads?
> Have you tried that?

I fixed it so that observers of all download items are just once
registered only at DownloadsDOMHandler::SendCurrentDownloads().

[Paweł Hajdan Jr., 2011-05-10 11:37:51]

> I wrote the setter just because currently all members in DownloadItem
> class are private and getter and setter functions are defined to
> access these variables.

Yeah, most of those getters and setters are bad too. It's really not much better
than a public variable, and make analyzing the code equally hard. Let's not add
more, even if there are still some.

Please note that there are many other places where we have getters and setters
and they're not always bad. But here we have 20+ of them, and often they're used
in a way that should be converted to a method of DownloadItem instead of using
the accessors directly.

> Do you mean that we should make |file_exists_|
> a public variable? I did not yet fix this, but I will fix this if it
> is our convention.

No, no, public class variables are forbidden
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...
; this also shows that accessors are generally fine, just should not be
overused).

I meant that we flip file_exists_ from true to false, but never in the opposite
direction. It may be better to add a method OnDownloadedFileRemoved to reflect
that.

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:373: // A flag for indicating if the
downloaded path exists or not.
nit: Please remove "or not", and replace "path" with "file". It's not obvious
what would it mean to download a path.

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:268: void
DownloadManager::CheckForFilesRemoval() {
nit: Could you add a DCHECK(BrowserThread::CureentlyOn(...)) to each of the new
methods to make it immediately obvious which thread they run on and catch any
potential misuse?

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:308: // Since all download items are
registered to DownloadDOMHandler's
nit: I'd rather remove this comment. It might become confusing if this code has
more callers, and generally it's not necessary to comment each call to
UpdateObservers.

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:73: typedef
std::vector<std::pair<int64, FilePath> > PathVector;
nit: According to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...,
typedefs should be listed before ctors. Sorry I missed that earlier.

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:238: // Check the existence of
downloaded files.
The comment should be more precise than "Check". It's not necessarily obvious
what that means. How about "Checks whether downloaded files still exists.
Updates state of downloads that refer to removed files. The check runs in the
background and may finish asynchronously after this method returns."?

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:281: // We don't check the file
existence on the UI thread to prevent UI thread
nit: Remove the "We don't" part, we don't write that in every place using the
FILE thread.

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:283: void
CheckForFilesRemovalOnFileThread(const PathVector& existing_paths);
nit: How about renaming existing_paths to just paths? They are not necessarily
existing, we have to check for that.

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/14002/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:123: for
(OrderedDownloads::iterator it = download_items_.begin();
Oh, the loop is now empty and the comment is also misleading. I think the code
can stay here, just always call AddObserver. You shouldn't need HasObservers,
because this is the only place that adds something to download_items_.

Also, adding observers in SendCurrentDownloads is a surprising side effect. I
think the best place for observer adding code is here, in ModelChanged.

[Randy Smith (Not in Mondays), 2011-05-10 20:29:51]

This is definitely a step in the right direction; thank you.  Some high level
comments:
* I'm reluctant to have this going in without handling the original race
properly (i.e. if we delete a file outside of chrome just as we're doing
something else with it inside of chrome, the "right thing" should happen). 
Could you look at what it would take to handle that race for the various actions
one might take on a download correctly?  If possible, I'd like to have a base
fix for the problem as well as improving the user experience.  (I'm ok if the
result of losing the race is just setting the download item as corresponding to
a removed file.)  I'd be ok with this being in a different CL, but that CL
strikes me as more important than this one, so I wouldn't end up happy if just
this CL went in.

* The download shelf should also be updated to behave properly if the file
disappears out from under the user.  I think transitioning to the "cancelled"
state and modifying the status text would be the easiest way to do that; the
"Cancelled" state will result in the appropriate actions being disabled, and
modifying the status text would result in the proper user experience.  Status
text is controlled by DownloadItemModel::GetStatusText().

* It would be good to write tests for new functionality added.  For the download
item shelf, I'd be ok with confirming the behavior described above at the unit
test level (i.e. instantiate a download item and a DownloadItemModel, link them
together, call the DownloadItem notification function that indicates that the
file's been deleted, and probe at DownloadItem and DownloadItemModel to make
sure that all the appropriate state changes have occurred).  I'm less familiar
with testing for the downloads page--if you can't find a test to copy and tweak
looking near downloads.html, ping me and I'll try to find something.

* I have a preference for per-download messages from UI to file thread and back.
 It means a lot more messages, but they won't happen very often, and the
interfaces (on both UI and file thread) will be simpler (just a FilePath & id
UI->File, and just an id FILE->UI).  

I'll postpone a more detailed review until we've figured out the high level
approach.  

Thanks again for doing this--it will be an improvement for the user experience.

[haraken1, 2011-05-12 04:07:03]

Dear Phajdan

Thank you very much for your comments!

>> Do you mean that we should make |file_exists_|
>> a public variable? I did not yet fix this, but I will fix this if it
>> is our convention.
>
> I meant that we flip file_exists_ from true to false, but never in the
> opposite
> direction. It may be better to add a method OnDownloadedFileRemoved to
> reflect
> that.

I fixed it.

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> File chrome/browser/download/download_item.h (right):
>
>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_item.h:373: // A flag for indicating if
> the downloaded path exists or not.
> nit: Please remove "or not", and replace "path" with "file". It's not
> obvious what would it mean to download a path.

I fixed it.

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> File chrome/browser/download/download_manager.cc (right):
>
>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_manager.cc:268: void
> DownloadManager::CheckForFilesRemoval() {
> nit: Could you add a DCHECK(BrowserThread::CureentlyOn(...)) to each of
> the new methods to make it immediately obvious which thread they run on
> and catch any potential misuse?

I fixed it.

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_manager.cc:308: // Since all download
> items are registered to DownloadDOMHandler's
> nit: I'd rather remove this comment. It might become confusing if this
> code has more callers, and generally it's not necessary to comment each
> call to UpdateObservers.

I fixed it.

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> File chrome/browser/download/download_manager.h (right):
>
>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_manager.h:73: typedef
> std::vector<std::pair<int64, FilePath> > PathVector;
> nit: According to
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...,
> typedefs should be listed before ctors. Sorry I missed that earlier.

I deleted the line since I found that PathVector is no longer needed
(See the randy's comment).

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_manager.h:238: // Check the existence
> of downloaded files.
> The comment should be more precise than "Check". It's not necessarily
> obvious what that means. How about "Checks whether downloaded files
> still exists. Updates state of downloads that refer to removed files.
> The check runs in the background and may finish asynchronously after
> this method returns."?

I fixed it.

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_manager.h:281: // We don't check the
> file existence on the UI thread to prevent UI thread
> nit: Remove the "We don't" part, we don't write that in every place
> using the FILE thread.

I fixed it.

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/download/dow...
> chrome/browser/download/download_manager.h:283: void
> CheckForFilesRemovalOnFileThread(const PathVector& existing_paths);
> nit: How about renaming existing_paths to just paths? They are not
> necessarily existing, we have to check for that.

I removed |existing_paths| since I found that |existing_paths| is no
longer needed (See the randy's comment).

>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/ui/webui/dow...
> File chrome/browser/ui/webui/downloads_dom_handler.cc (right):
>
>
http://codereview.chromium.org/6905049/diff/14002/chrome/browser/ui/webui/dow...
> chrome/browser/ui/webui/downloads_dom_handler.cc:123: for
> (OrderedDownloads::iterator it = download_items_.begin();
> Oh, the loop is now empty and the comment is also misleading. I think
> the code can stay here, just always call AddObserver. You shouldn't need
> HasObservers, because this is the only place that adds something to
> download_items_.
>
> Also, adding observers in SendCurrentDownloads is a surprising side
> effect. I think the best place for observer adding code is here, in
> ModelChanged.

I agree that HasObservers() is not needed.
Consequently, I wrote AddObserver() for all download items only at
DownloadsDOMHandler::ModelChanged().

Also, I added the following code in chrome/browser/resources/downloads.html,
though I am afraid that this code is "tricky" and is not so good:

 504 if (this.state_ != Download.States.REMOVED) {
 505     this.state_ = download.state;
 506   }

The above code is necessary because a "subtle" contention problem can
occur at chrome://downloads page without the if statement at the line
504. Specifically, the following problem can occur.

(1) A user opens chrome://downloads page.
(2) The user downloads files, say A, B and C.
(3) A, B and C are shown with file links in the chrome://downloads page.
(4) The user deletes A, B and C manually.
(5) The user reloads the chrome://downloads page.
(6) The expected behavior here is that A, B and C are labeled
"Removed" (without file links). However, there is the possibility that
some or all of A, B and C still remain shown with file links without
labeled "Removed". In my environment, this happens in the 20~30% of
the time.

The cause of this problem is as follows.

(1) When the user reloads the chrome://downloads page,
web_ui_->CallJavascriptFunction("downloadsList", results_value) (at
the line 214 of downloads_dom_handler.cc) is executed, and thus the
DOM of all download items begins to be updated by
downloadUpdated(results) (at the line 748 of downloads.html). This
update tries to update the state of A, B and C to the state COMPLETE.
However, this update does not occur in one breath but occurs one by
one at intervals of 50 ms because of setTimeout (at the line 759 of
downloads.html).

(2) The UI thread calls download_manager_->CheckForFilesRemoval() (at
the line 142 of downloads_dom_handler.cc) without waiting the
completion of the update for A, B and C in the step (1). Then the FILE
thread checks the existence of A, B and C asynchronously and
eventually calls download_item->OnDownloadedFileRemoved() (at the line
301 of download_manager.cc). This triggers the observers of A, B and C
to call downloadUpdated(results) (at the line 748 of downloads.html)
one by one in order to update the state of A, B and C to the state
REMOVED.

(3) As you can see, here it is not guaranteed that the update in the
step (1) completes before the update in the step (2) starts. In case
that the update in the step (1) runs after the update in the step (2)
for some or all of A, B and C, the files are not labeled REMOVED,
unfortunately.

For the time being, I solved this problem by inserting the if
statement to the downloads.html, which forbids the update in the step
(1) to overwrite the update in the step (2). This works well based on
the assumption that a file once marked as REMOVED will be never marked
as COMPLETE.

Are there any good way to solve this "subtle" contention?

[haraken1, 2011-05-12 04:07:14]

I would really appreciate your dedicated review and clear explanation!

> * The download shelf should also be updated to behave properly if the file
> disappears out from under the user.  I think transitioning to the
> "cancelled"
> state and modifying the status text would be the easiest way to do that; the
> "Cancelled" state will result in the appropriate actions being disabled, and
> modifying the status text would result in the proper user experience.
>  Status
> text is controlled by DownloadItemModel::GetStatusText().

For the time being, I fixed it only for gtk (because I do not have Mac
and Windows environments for now). I set the "probe point" in
DownloadShelfGtk::Show(). The removed file is labeled as "Removed" in
the download shelf.

> * It would be good to write tests for new functionality added.  For the
> download
> item shelf, I'd be ok with confirming the behavior described above at the
> unit
> test level (i.e. instantiate a download item and a DownloadItemModel, link
> them
> together, call the DownloadItem notification function that indicates that
> the
> file's been deleted, and probe at DownloadItem and DownloadItemModel to make
> sure that all the appropriate state changes have occurred).  I'm less
> familiar
> with testing for the downloads page--if you can't find a test to copy and
> tweak
> looking near downloads.html, ping me and I'll try to find something.

I added the unit test of DownloadItem and DownloadItemModel to
DownloadManagerTest. In particular, I added the unit test for file
removal as DownloadManagerTest.DownloadRemoveTest.

However, I could not find any good tests to copy or tweak for
chrome://downloads page.

> * I have a preference for per-download messages from UI to file thread and
> back.
>  It means a lot more messages, but they won't happen very often, and the
> interfaces (on both UI and file thread) will be simpler (just a FilePath &
> id
> UI->File, and just an id FILE->UI).

I fixed it.

> * I'm reluctant to have this going in without handling the original race
> properly (i.e. if we delete a file outside of chrome just as we're doing
> something else with it inside of chrome, the "right thing" should happen).
> Could you look at what it would take to handle that race for the various
> actions
> one might take on a download correctly?  If possible, I'd like to have a
> base
> fix for the problem as well as improving the user experience.  (I'm ok if
> the
> result of losing the race is just setting the download item as corresponding
> to
> a removed file.)  I'd be ok with this being in a different CL, but that CL
> strikes me as more important than this one, so I wouldn't end up happy if
> just
> this CL went in.

(I am sorry if the below statements miss the point you want to argue.)

Potentially, there can be two races:

[1] A file is shown (in chrome://downloads or in the download shelf)
as it exists, but actually it does not exist.
[2] A file is shown as it does not exist, but actually it exists.

In my current implementation, ***whether the file is removed inside or
outside of Chrome***, the race [2] never happens but the race [1] can
happen. For example, if a file A is removed outside of Chrome manually
with rm command, the file remains shown as it exists until any new
file is downloaded and thus chrome://downloads and the download shelf
are updated.

Actually, completely avoiding the race [1] is impractical (since in
order to avoid the race [1] we have to "always" watch for the file
existence). So I think that the best (practical) UI may be the
following one:

- Chrome checks the existence of downloaded files only whenever
chrome://downloads or the download shelf is going to be refreshed.
(already implemented)
- When a user sees the race [1] unfortunately and clicks the file
links trying to open the (already removed) file, then Chrome just
tells the user "Sorry, that file is not found..." using an alert
dialog or something. (not yet implemented)

Would you please give me any comments?

[Randy Smith (Not in Mondays), 2011-05-12 18:00:54]

On 2011/05/12 04:07:03, haraken wrote:
> (1) When the user reloads the chrome://downloads page,
> web_ui_->CallJavascriptFunction("downloadsList", results_value) (at
> the line 214 of downloads_dom_handler.cc) is executed, and thus the
> DOM of all download items begins to be updated by
> downloadUpdated(results) (at the line 748 of downloads.html). This
> update tries to update the state of A, B and C to the state COMPLETE.
> However, this update does not occur in one breath but occurs one by
> one at intervals of 50 ms because of setTimeout (at the line 759 of
> downloads.html).

To me, this is the basic cause of this race--information from the past is being
sent into the future by the timeout and is stale by the time it gets there.  My
inclination would be to say that the list of downloads to update can be sent and
handled in this way, but that the information which is used for the update
should be fetched inline each time.  This would mean a) (optional) changing the
downloadUpdated() message to just have a list of ids, and b) changing the
Downloads.prototype.updated and Download.prototype.update functions to fetch the
information inline in those routines.  

I don't consider this is a reasonable expansion of this CL, so I'd be ok with
the change you've suggested above.  But I'd appreciate it if you'd file a new
bug describing the issue as you do above, and comment the code at the point of
the test against REMOVED to describe the problem and point to the bug (standard
form is via short URL: http://crbug.com/<number>).

[Randy Smith (Not in Mondays), 2011-05-12 18:04:44]

On 2011/05/12 04:07:14, haraken wrote:
> - When a user sees the race [1] unfortunately and clicks the file
> links trying to open the (already removed) file, then Chrome just
> tells the user "Sorry, that file is not found..." using an alert
> dialog or something. (not yet implemented)

Yes, this is the behavior I was hoping we could implement, for open or any other
behavior the user can do from the download shelf or downloads.html for this
file.

[Randy Smith (Not in Mondays), 2011-05-12 18:39:13]

On 2011/05/12 04:07:14, haraken wrote:
> > * The download shelf should also be updated to behave properly if the file
> > disappears out from under the user.  I think transitioning to the
> > "cancelled"
> > state and modifying the status text would be the easiest way to do that; the
> > "Cancelled" state will result in the appropriate actions being disabled, and
> > modifying the status text would result in the proper user experience.
> >  Status
> > text is controlled by DownloadItemModel::GetStatusText().
> 
> For the time being, I fixed it only for gtk (because I do not have Mac
> and Windows environments for now). I set the "probe point" in
> DownloadShelfGtk::Show(). The removed file is labeled as "Removed" in
> the download shelf.

I was hoping for a modification that didn't require per-UI changes, both because
it's a pain to test, because it brings up the possibility of skew between the
platforms, and because the more platform specific code we have, the more work
the next person modifying the code has to do :-}.  What made the GTK change
required--why wasn't just modifying the status text and enable/disable state of
the commands enough?

(As best I can tell, the only place DownloadShelf*::Show() is called on any of
the three platforms is when a download is added to the shelf, which occurs in
platform independent code in Browser::OnStartDownload.  So that would be a place
to put the probe point which is outside the UI implementations.)

[Randy Smith (Not in Mondays), 2011-05-12 20:21:17]

Next round of comments; hopefully useful.  Two notes:
* I couldn't find any tests for downloads.html either :-{.  We should fix that,
but that's not part of this CL.  I've filed a bug; hopefully we can get to it
sometime soon.
* I don't know if you're familiar with this aspect of Rietveld (==
codereview.chromium.org) but you can read the comments directly in the context
of the source diff in which they were made by viewing the diffs after folks have
submitted comments.  If you do that, you can mark those comments as done with a
single button click, or reply right there, which is useful for other folks in
that they can see the entire thread of discussions around a single piece of
source at one point in the file.  It's a minor point, but I thought I'd share
it, since I find it a fairly useful feature.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:373: bool file_exists_;
This flag is currently only valid after the state has transitioned to
completed/the file has been renamed to its final name.  I'd like to either
change the flag name so that it reflects that, or change its behavior so that
it's valid throughout the lifetime of the DownloadItem (commenting that we can
only rely on it after the state transitions to COMPLETE is another option, but
one I'd like to avoid it we can.)

The first option is the easiest; invert the meaning, and change the name to
"file_externally_removed_".  The second involves initializing this flag as
false, and then setting it to true when we rename to the final name.  That's
tricky, as for some downloads (everything but drag and drop) that transition
occurs in OnDownloadRenamedToFinalName, but for drag-and-drop, it has the final
name throughout the download process (and it's a bit tricky to figure that out).
 But if you want to go that route I can dig in and guide you in how to figure it
out.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:294: void
DownloadManager::OnFilesRemovalDetected(
Given that we're doing single file checks now, we should change the plural in
the name to a singular (Files->File).

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:297: if (!file_exists) {
Given that this function doesn't do anything if file_exists is false, how about
removing that argument from the function signature and not sending the response
message from the file thread if the file is still there?

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:430: new
DownloadItemModel(download));
It doesn't look like this is used.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:443: OnDownloadError(0,
error_size, -6);
Why put in this change?

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:461:
EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
I have no objection to you adding these tests, but I don't see the relationship
to your CL, and I wanted to make sure I wasn't missing anything.  Is there a
relationship between these changes and your CL?

[Paweł Hajdan Jr., 2011-05-13 08:41:10]

+erg for GTK changes

By the way, are you going to also update Windows/Mac UI code? I think it can be
postponed, but there should be a PlatformParity bug open.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:373: bool file_exists_;
On 2011/05/12 20:21:17, rdsmith wrote:
> This flag is currently only valid after the state has transitioned to
> completed/the file has been renamed to its final name.  I'd like to either
> change the flag name so that it reflects that, or change its behavior so that
> it's valid throughout the lifetime of the DownloadItem (commenting that we can
> only rely on it after the state transitions to COMPLETE is another option, but
> one I'd like to avoid it we can.)
> 
> The first option is the easiest; invert the meaning, and change the name to
> "file_externally_removed_". 

I prefer file_exists for simplicity. file_externally_removed_ may result in many
double-negations in caller code, and the name is just longer.

I think that because of asynchronous operations on the FILE thread there will
always be a period of time when the flag will be invalid, no matter how it's
named.

My general recommendation is then - just make it simple.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:700: // Remove the
downloaded file
nit: Dot at the end of the sentence.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:701:
file_util::Delete(new_path, false);
Please check return value of this.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:503: // This code is tricky and is not
so good. See issue 6905049 for more details.
No no, I think we have enough hacks in the download code and don't need another
one. We can have a separate discussion how to deal with it the best way (thanks
for explaining the issue in detail).

Can we just introduce a "don't know" or "updating" state?

Please note that the bug for this CL is Mstone-X, i.e. not a priority. I'd
rather postpone this patch than make the download code more messy.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/gtk/downl...
File chrome/browser/ui/gtk/download/download_item_gtk.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/gtk/downl...
chrome/browser/ui/gtk/download/download_item_gtk.h:163: // The widget that
contains the texts of name_label_ and status_label_
nit: Dot at the end, surround member variable names in || like |name_label_|.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:122: // Add oneself to all
download items as an observer
nit: Dot at the end of the sentence.

[Randy Smith (Not in Mondays), 2011-05-13 13:33:38]

On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> +erg for GTK changes

erg: I'd recommend you hold on this review until we've determined if we really
need per-platform UI changes.  I'm not yet convinced, and if we can avoid them
I'd like to, for the reasons I give in my last set of comments.

[haraken1, 2011-05-13 14:05:50]

Dear Pawel

> By the way, are you going to also update Windows/Mac UI code? I think it can
> be postponed, but there should be a PlatformParity bug open.

Sure, I will also do the update for Windows and Mac as another CL.
However, it may take a month or so, since I do not yet get Windows and
Mac environments.

[haraken1, 2011-05-13 14:05:52]

Dear Randy

>> - When a user sees the race [1] unfortunately and clicks the file
>> links trying to open the (already removed) file, then Chrome just
>> tells the user "Sorry, that file is not found..." using an alert
>> dialog or something. (not yet implemented)
>
> Yes, this is the behavior I was hoping we could implement, for open or any
> other
> behavior the user can do from the download shelf or downloads.html for this
> file.

I added this functionality. When a user clicks a file which is shown
as if it exists in the download shelf or in chrome://downloads page
but does not exist actually, then the file link is invalidated
immediately and the file is labeled as "Removed". I think that this
behavior would be enough for the user to know what is happening. I
think an alert dialog is not necessary here, but what do you think?

>> For the time being, I fixed it only for gtk (because I do not have Mac
>> and Windows environments for now). I set the "probe point" in
>> DownloadShelfGtk::Show(). The removed file is labeled as "Removed" in
>> the download shelf.
>
> I was hoping for a modification that didn't require per-UI changes, both
> because
> it's a pain to test, because it brings up the possibility of skew between
> the
> platforms, and because the more platform specific code we have, the more
> work
> the next person modifying the code has to do :-}.  What made the GTK change
> required--why wasn't just modifying the status text and enable/disable state
> of
> the commands enough?

I am afraid I am wrong, but I think the per-UI change would be required.

This is because "the change that occurs in the download shelf when the
state changes from IN_PROGRESS to CANCELED" and "the change that
occurs when the state changes from COMPLETE to REMOVED" are different
in the UI level. In the IN_PROGRESS to CANCELED case, the shelf's text
should be changed like this:

[Before]
foo.txt
0.0/4.0GB, 8 mins left

[After]
foo.txt
Canceled

As you can see, we can make this change just by changing the text of
the second line. However, in the COMPLETE to REMOVED case, the shelf's
text should be changed like this:

[Before]
foo.txt      <--- displayed at the vertical center of the shelf

[After]
foo.txt      <--- displayed at the upper half of the shelf
Removed      <--- displayed at the lower half of the shelf

As you can see, we have to increase the number of labels from one (a
file name at the vertical center) to two (a file name at the upper
half and the state at the lower half) and set their positions. I guess
that this change will require per-UI modification.


> (As best I can tell, the only place DownloadShelf*::Show() is called on any
> of
> the three platforms is when a download is added to the shelf, which occurs
> in
> platform independent code in Browser::OnStartDownload.  So that would be a
> place
> to put the probe point which is outside the UI implementations.)

Done.

[haraken1, 2011-05-13 14:06:11]

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:373: bool file_exists_;
I also prefer |file_externally_removed_| in this case. As Randy advised, I
inverted the meaning and changed the name to |file_externally_removed_|.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:294: void
DownloadManager::OnFilesRemovalDetected(
On 2011/05/12 20:21:17, rdsmith wrote:
> Given that we're doing single file checks now, we should change the plural in
> the name to a singular (Files->File).

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:297: if (!file_exists) {
On 2011/05/12 20:21:17, rdsmith wrote:
> Given that this function doesn't do anything if file_exists is false, how
about
> removing that argument from the function signature and not sending the
response
> message from the file thread if the file is still there?

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:430: new
DownloadItemModel(download));
On 2011/05/12 20:21:17, rdsmith wrote:
> It doesn't look like this is used.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:443: OnDownloadError(0,
error_size, -6);
Sorry, this change is not related to this CL. I changed the value of error_size
from 1024 to 3 just because practically the error_size should be less than
kTestDataLen.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:461:
EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
The purpose of this CL is

[1] to add TEST_F(DownloadManagerTest, DownloadRemoveTest)
[2] to add the test for DownloadItemModel::GetStatusText()

Here is the part where I would like to test DownloadItemModel::GetStatusText().
However, it is a bit complicated to directly check the returned string by
GetStatusText(), since GetStatusText() is internally doing value format
conversion. So, instead, I inserted the code to check each value (which will be
used in the returned string) individually.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:700: // Remove the
downloaded file
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> nit: Dot at the end of the sentence.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:701:
file_util::Delete(new_path, false);
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> Please check return value of this.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:503: // This code is tricky and is not
so good. See issue 6905049 for more details.
I would like to postpone to fix this hack. As Randy advised, I filed this issue
as a bug and referred the bug number in the comment.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/gtk/downl...
File chrome/browser/ui/gtk/download/download_item_gtk.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/gtk/downl...
chrome/browser/ui/gtk/download/download_item_gtk.h:163: // The widget that
contains the texts of name_label_ and status_label_
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> nit: Dot at the end, surround member variable names in || like |name_label_|.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:122: // Add oneself to all
download items as an observer
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> nit: Dot at the end of the sentence.

Done.

[haraken1, 2011-05-13 14:08:17]

Sorry for the duplicate comments below (because of my operation mistake).

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:373: bool file_exists_;
I also prefer |file_externally_removed_| in this case. As Randy advised, I
inverted the meaning and changed the name to |file_externally_removed_|.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:294: void
DownloadManager::OnFilesRemovalDetected(
On 2011/05/12 20:21:17, rdsmith wrote:
> Given that we're doing single file checks now, we should change the plural in
> the name to a singular (Files->File).

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:297: if (!file_exists) {
On 2011/05/12 20:21:17, rdsmith wrote:
> Given that this function doesn't do anything if file_exists is false, how
about
> removing that argument from the function signature and not sending the
response
> message from the file thread if the file is still there?

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:430: new
DownloadItemModel(download));
On 2011/05/12 20:21:17, rdsmith wrote:
> It doesn't look like this is used.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:443: OnDownloadError(0,
error_size, -6);
Sorry, this change is not related to this CL. I changed the value of error_size
from 1024 to 3 just because practically the error_size should be less than
kTestDataLen.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:461:
EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
The purpose of this CL is

[1] to add TEST_F(DownloadManagerTest, DownloadRemoveTest)
[2] to add the test for DownloadItemModel::GetStatusText()

Here is the part where I would like to test DownloadItemModel::GetStatusText().
However, it is a bit complicated to directly check the returned string by 
GetStatusText(), since GetStatusText() is internally doing value format
conversion. So, instead, I inserted the code to check each value (which will be
used in the returned string) individually.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:700: // Remove the
downloaded file
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> nit: Dot at the end of the sentence.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:701:
file_util::Delete(new_path, false);
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> Please check return value of this.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:503: // This code is tricky and is not
so good. See issue 6905049 for more details.
I would like to postpone to fix this hack. As Randy advised, I filed this issue
as a bug and referred the bug number in the comment.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/gtk/downl...
File chrome/browser/ui/gtk/download/download_item_gtk.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/gtk/downl...
chrome/browser/ui/gtk/download/download_item_gtk.h:163: // The widget that
contains the texts of name_label_ and status_label_
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> nit: Dot at the end, surround member variable names in || like |name_label_|.

Done.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:122: // Add oneself to all
download items as an observer
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> nit: Dot at the end of the sentence.

Done.

[Randy Smith (Not in Mondays), 2011-05-13 20:08:40]

On 2011/05/13 14:05:52, haraken wrote:
> Dear Randy
> 
> >> - When a user sees the race [1] unfortunately and clicks the file
> >> links trying to open the (already removed) file, then Chrome just
> >> tells the user "Sorry, that file is not found..." using an alert
> >> dialog or something. (not yet implemented)
> >
> > Yes, this is the behavior I was hoping we could implement, for open or any
> > other
> > behavior the user can do from the download shelf or downloads.html for this
> > file.
> 
> I added this functionality. When a user clicks a file which is shown
> as if it exists in the download shelf or in chrome://downloads page
> but does not exist actually, then the file link is invalidated
> immediately and the file is labeled as "Removed". I think that this
> behavior would be enough for the user to know what is happening. I
> think an alert dialog is not necessary here, but what do you think?

That sounds like good behavior; thank you.

> >> For the time being, I fixed it only for gtk (because I do not have Mac
> >> and Windows environments for now). I set the "probe point" in
> >> DownloadShelfGtk::Show(). The removed file is labeled as "Removed" in
> >> the download shelf.
> >
> > I was hoping for a modification that didn't require per-UI changes, both
> > because
> > it's a pain to test, because it brings up the possibility of skew between
> > the
> > platforms, and because the more platform specific code we have, the more
> > work
> > the next person modifying the code has to do :-}.  What made the GTK
change
> > required--why wasn't just modifying the status text and enable/disable state
> > of
> > the commands enough?
> 
> I am afraid I am wrong, but I think the per-UI change would be required.
> [...]

Nope, you're right; I read what you said and looked at the Views implementation,
and we're going to need per-platform changes for this.  Unfortunately, I have
the same reluctance to make the platforms behave differently as Pawel has to
adding the REMOVED check in downloads.html; this bug doesn't seem high enough
priority to me to skew the platforms.  What I think I'll suggest for that is:
* Try to keep the per-UI changes as simple as possible.  I'd recommend making
each UI display the label in the vertical middle if the status text is null, and
one above the other if the status text isn't null, and don't do anything fancier
than that.
* Do that change on all three platforms.  You can use the try bots for compile
and automatic testing; when that works, ping me and I'll do whatever manual
testing you specify.
* We'll need to pull in new reviewers for those files, as neither Pawel nor I
knows the code well.  I usually use dmazzoni (views), thakis (cocoa), and estade
(gtk), but erg is fine for GTK as well.

One other high level thing I noticed that I'll mention now is that I'd really
like to reduce the number of "probe points".  As far as I'm concerned, this is a
rare race, and one that we're never going to completely win--we'll always have
to handle the case where a file got removed at "just" the wrong time.  For that
reason, I'd like to probe where it's convenient to the browser, and not bother
if it isn't.  Specifically, I don't want to do any probes within the UI code,
and I don't want to do any probes that occur on a regular basis (e.g.
per-download, or per-user action (unless that action can't be completed because
the file's gone, and that's not probing, that's noticing for that single file
that it's gone.)

More detailed review coming.

[Randy Smith (Not in Mondays), 2011-05-13 20:45:32]

Next round.  One high level comment: I had been imagining that the detection of
the file having been removed upon Open would occur in OpenDownload (or similar)
by way of detecting when that produced an error.  As indicated below, I'm
uncomfortable with the frequency and location of probes that are produced by
just checking everything every time we go to open a download.  Unfortunately,
looking at at least the Linux code, I see no good way to be absolutely sure that
the open has succeeded, so your basic idea is probably the best choice. 
However, I'd like to do it only for the particular file that is being opened. 
To that end, I'd suggest that you modify the download manager routine on the
file thread so that it's publicly accessible, and post a task to it from
DownloadItem::OpenDownload() for only that one item.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:373: bool file_exists_;
On 2011/05/13 08:41:10, Paweł Hajdan Jr. wrote:
> On 2011/05/12 20:21:17, rdsmith wrote:
> > This flag is currently only valid after the state has transitioned to
> > completed/the file has been renamed to its final name.  I'd like to either
> > change the flag name so that it reflects that, or change its behavior so
that
> > it's valid throughout the lifetime of the DownloadItem (commenting that we
can
> > only rely on it after the state transitions to COMPLETE is another option,
but
> > one I'd like to avoid it we can.)
> > 
> > The first option is the easiest; invert the meaning, and change the name to
> > "file_externally_removed_". 
> 
> I prefer file_exists for simplicity. file_externally_removed_ may result in
many
> double-negations in caller code, and the name is just longer.
> 
> I think that because of asynchronous operations on the FILE thread there will
> always be a period of time when the flag will be invalid, no matter how it's
> named.

Sure, but having a race condition to watch for between threads is different that
the value lying for the entire length of the download processing (once we
complete the download we don't really run much code on it, so we care less about
the value).

> My general recommendation is then - just make it simple.

haraken: If you want to follow both Pawel's and my preferences, the way to do it
would be to leave the name as file_exists_, initialize it to (target_name_ ==
full_path_.BaseName()), set it to that value again in SetFileCheckResults, and
set it unilaterally to true in DownloadItem::OnDownloadRenamedToFinalName(). 
But I really don't want to leave it as file_exists_ and not have it be valid for
the entire life of the DownloadItem.

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/19001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:503: // This code is tricky and is not
so good. See issue 6905049 for more details.
On 2011/05/13 14:08:17, haraken wrote:
> I would like to postpone to fix this hack. As Randy advised, I filed this
issue
> as a bug and referred the bug number in the comment.

haraken: I'm afraid I want to support Pawel on this.  I tend to bias towards
letting people contribute, but his comment's reminded me that that decision
needs to be tempered by the priority of the fix and the nature of the code.  
And he's quite right that we have a lot of hacks and need to be moving towards
cleaning the code up.

Would it be possible to keep the updates in the downloads.html file FIFO?  I.e.
if new updates come in (e.g. because the file has been REMOVED) to put it at the
end of the list that's being processed in 50ms chunks?

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:456: // and
download->total_bytes(), individually.
I think these are all fine things to test, but testing them doesn't really seem
relevant to testing status text.  If you're testing the code that produces
status text from these values you need to examine the return from
GetStatusText().  Without checking that, you're not actually testing the
modification you made to GetStatusText().  Am I confused?

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:504: // See http://crbug.com/82531
To add a little more meat to my earlier suggestion: This is occurring because of
a race between multiple calls to the Javascript downloadUpdated() function.  If
you modify that function to suffix the incoming data to a global array, and then
process that array in 50ms chunks as current, the race goes away, you'll then be
processing the incoming info in FIFO order.  I think that's a fairly simple
change, and it doesn't require the high overhead of a round trip to the browser
process that my original "here's the real fix" suggestion had.  Given that Pawel
objects to putting this hack in (and he's right), could you fix the problem that
way?

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3230: CheckForFilesRemoval();
I apologize--I know I recommended this location, but now that I actually see it
here, my brain kicks into gear, and I don't think this function call belongs
here.  Where we put the probe points is completely up to us--I agree that it's
good to "clean up" old history that no longer has the downloads around, but I
don't think it's worth a lot of effort to try and keep current downloads up to
date.  So I think the right place for the probe point is in
DownloadManager::OnQueryDownloadEntriesComplete(), and basically no where else
(i.e. once per browser startup, after we've loaded everything from the history).
 If you have other locations you'd like to put it, let me know, but I'd like it
to be in the core downloads code, and not to happen too often.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/gtk/downl...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/gtk/downl...
chrome/browser/ui/gtk/download/download_item_gtk.cc:850:
parent_shelf_->CheckForFilesRemoval();
As mentioned elsewhere, I don't think this belongs here.  It's both not core
download code, and it's much more frequently than I think we want to be throwing
a whole lot of probe messages around for a rare occurence.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:142:
download_manager_->CheckForFilesRemoval();
And I'd also nuke these, as well.  I think doing the check for file removal
occasionally (even as infrequently as when you startup the browser) is fine; the
"real" fix is handling the case when we try to do an operation on a file that's
been removed.

[haraken1, 2011-05-16 11:42:27]

> Nope, you're right; I read what you said and looked at the Views
implementation,
> and we're going to need per-platform changes for this.  Unfortunately, I have
> the same reluctance to make the platforms behave differently as Pawel has to
> adding the REMOVED check in downloads.html; this bug doesn't seem high enough
> priority to me to skew the platforms.  What I think I'll suggest for that is:
> * Try to keep the per-UI changes as simple as possible.  I'd recommend
> making each UI display the label in the vertical middle if the status text is
null,
> and one above the other if the status text isn't null, and don't do anything
> fancier than that.
> * Do that change on all three platforms.  You can use the try bots for
> compile and automatic testing; when that works, ping me and I'll do whatever
manual
> testing you specify.
> * We'll need to pull in new reviewers for those files, as neither Pawel nor I
> knows the code well.  I usually use dmazzoni (views), thakis (cocoa), and
> estade (gtk), but erg is fine for GTK as well.

I am very sorry for the confusion, but consequently I reached the point that the
modification only for gtk will be enough. I looked at the download shelf code
for 
views and gtk, and found that the modification is required for gtk but no
modification will be required for views and cocoa in order to get the (fancy)
behavior that I described (i.e. the download shelf displays a file name at the
first line and  "Removed" label at the second line), although I have not yet
tested the behavior for views and cocoa since I do not have Windows and Mac
environments for now. 

Therefore, I would like to leave the change that I made for gtk as it is, but
what do you think? If it can cause any problem on views and cocoa, I would like
to follow your idea. 

> Sure, but having a race condition to watch for between threads is
> different that the value lying for the entire length of the download
> processing (once we complete the download we don't really run much code
> on it, so we care less about the value).
>
>> My general recommendation is then - just make it simple.
>
> haraken: If you want to follow both Pawel's and my preferences, the way
> to do it would be to leave the name as file_exists_, initialize it to
> (target_name_ == full_path_.BaseName()), set it to that value again in
> SetFileCheckResults, and set it unilaterally to true in
> DownloadItem::OnDownloadRenamedToFinalName().  But I really don't want
> to leave it as file_exists_ and not have it be valid for the entire life
> of the DownloadItem.

I understand Pawel and Randy's thoughts...and then my inclination is
|file_externally_removed_| (that Randy proposed previously) since it captures
the feature of the variable best in the three approaches, although I agree that
the length of the variable is a bit too long as Pawel indicated. For the time
being, the current patch set uses |file_externally_removed_|. 

> Next round.  One high level comment: I had been imagining that the detection
> of
> the file having been removed upon Open would occur in OpenDownload (or
> similar)
> by way of detecting when that produced an error.  As indicated below, I'm
> uncomfortable with the frequency and location of probes that are produced by
> just checking everything every time we go to open a download.
>  Unfortunately,
> looking at at least the Linux code, I see no good way to be absolutely sure
> that
> the open has succeeded, so your basic idea is probably the best choice.
> However, I'd like to do it only for the particular file that is being
> opened.
> To that end, I'd suggest that you modify the download manager routine on the
> file thread so that it's publicly accessible, and post a task to it from
> DownloadItem::OpenDownload() for only that one item.

I agree. I fixed it.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:456: // and
download->total_bytes(), individually.
It makes sense. I fixed it.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:504: // See http://crbug.com/82531
That's a great solution!!! I fixed it using a FIFO array.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3230: CheckForFilesRemoval();
I agree that we should not include too frequent probe points for rare cases.
However, I think that only at DownloadManager::OnQueryDownloadEntriesComplete()
(i.e. once per browser startup) is a bit too infrequent. I mean, I am afraid
that it is not intuitive for a user that removed files remain displayed as if
they exist even if the user updates the chrome://downloads page (with F5 key).
For example, consider the case where a user removes many files in his download
directory. Then the user opens chrome://downloads page and clicks one of the
files. Then it immediately changes its state to "Removed" and thus the user can
notice that the file is already removed. Next, the user clicks another file.
Then it also immediately changes its state to "Removed" and thus the user can
notice that the file is also already removed. Here the user would wonder that
the chrome://downloads page does not reflect the latest state of the download
directory. So the user presses F5 key to update the chrome://downloads page...
but the states of the page does not change...I think this behavior is not
intuitive. 

Thus, I am not sure but, for the time being, I inserted the probe point only at
DownloadsDOMHandler::ModelChanged(). (In this case, the probe point at
DownloadManager::OnQueryDownloadEntriesComplete() is not necessary because it
eventually calls DownloadsDOMHandler::ModelChanged()). I would like to hear your
opinion.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/gtk/downl...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/gtk/downl...
chrome/browser/ui/gtk/download/download_item_gtk.cc:850:
parent_shelf_->CheckForFilesRemoval();
I removed it.

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:142:
download_manager_->CheckForFilesRemoval();
I removed it.

[Randy Smith (Not in Mondays), 2011-05-16 20:57:12]

This is looking good; I think we're converging.  Thanks for the hard work.

With regard to the GTK-only change: looking at the views code (basically,
searching for show_status_text_ in download_item_view.cc) it looks as if that
variable is constructed true, and is at some point set to false and never back
to true again.  So I would think that once the download had been completed (and
that variable set to false) even if we changed the status text to "Removed",
that wouldn't show up in the views code.  Am I missing something?  (No, I
haven't tested this--it'll be a bit of a hassle to set that up, so I want to
have some expectation that it'll work first :-}).

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6905049/diff/23002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3230: CheckForFilesRemoval();
On 2011/05/16 11:42:27, haraken wrote:
> I agree that we should not include too frequent probe points for rare cases.
> However, I think that only at
DownloadManager::OnQueryDownloadEntriesComplete()
> (i.e. once per browser startup) is a bit too infrequent. I mean, I am afraid
> that it is not intuitive for a user that removed files remain displayed as if
> they exist even if the user updates the chrome://downloads page (with F5 key).
> For example, consider the case where a user removes many files in his download
> directory. Then the user opens chrome://downloads page and clicks one of the
> files. Then it immediately changes its state to "Removed" and thus the user
can
> notice that the file is already removed. Next, the user clicks another file.
> Then it also immediately changes its state to "Removed" and thus the user can
> notice that the file is also already removed. Here the user would wonder that
> the chrome://downloads page does not reflect the latest state of the download
> directory. So the user presses F5 key to update the chrome://downloads page...
> but the states of the page does not change...I think this behavior is not
> intuitive. 
> 
> Thus, I am not sure but, for the time being, I inserted the probe point only
at
> DownloadsDOMHandler::ModelChanged(). (In this case, the probe point at
> DownloadManager::OnQueryDownloadEntriesComplete() is not necessary because it
> eventually calls DownloadsDOMHandler::ModelChanged()). I would like to hear
your
> opinion. 

So I think the use case you describe (a "reload" on the downloads.html page
updating the "removed" state) is a good one, and we should find a way to handle
it.  But I'd rather have the probe point be in
DownloadsDOMHandler::HandleGetDownloads() rather than in ModelChanged(). 
ModelChanged() will be called any time a new download is added to or deleted
from the DownloadManager, but based on the use case, we only want to do a probe
when the downloads.html page is reloaded.  Does that make sense?

(I think I'd still like to keep the call in OnQueryDownloadEntriesComplete,
though I don't feel strongly about it.  Whichever way we go with the above, I
don't think the call in OnQueryDownloadEntriesComplete() will change user facing
behavior, but it'll keep the in-memory entries consistent on a browser-startup
granularity in case we ever put other code in that relies on historical download
entries being updated with regard to file system changes.  But it is an
unnecessary probe point, albeit one that only happens once on browser startup.)

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
I think I'd like a comment here explaining this design choice.  Maybe something
like "Ideally, we'd detecting errors in opening and report them, but we don't
generally have the proper interface for that to the external program that opens
the file.  So instead we spawn a check to update the UI if the file has been
deleted in parallel with the open."?

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:782: return;
Do we still need this comment and the null check for downloads?  Once downloads
has been created, won't the object stick around even if it's cleared (based on
my reading of the code, but I may be wrong; I'm not a JS expert).

[Paweł Hajdan Jr., 2011-05-17 20:03:39]

This is getting better, just a few comments.

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
On 2011/05/16 20:57:12, rdsmith wrote:
> I think I'd like a comment here explaining this design choice.  Maybe
something
> like "Ideally, we'd detecting errors in opening and report them, but we don't
> generally have the proper interface for that to the external program that
opens
> the file.  So instead we spawn a check to update the UI if the file has been
> deleted in parallel with the open."?

There is an ugliness in a way those objects communicate. DownloadItem calls
DownloadManager here, which is going to ask DownloadItem for its ID and path,
and then it's going to update DownloadItem's file removal flag.

How about doing the check in DownloadItem instead, i.e. making
DownloadManager::CheckForFileRemoval a DownloadItem's method, removing
DownloadItem::OnDownloadedFileRemoved and adding
DownloadManager::OnDownloadedFileRemoved instead to update |history_downloads_|?

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:273: void
DownloadManager::CheckForFileRemoval(DownloadItem* download_item) {
nit: Add a BrowserThread::CurrentlyOn DCHECK here too.

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_shelf.cc:73: return !download_->IsCancelled()
&& !download_->file_externally_removed();
I don't like how this file_externally_removed accessor's usage is proliferating.
Is it possible to have just one method in DownloadItem like CanOpenDownload,
IsInProgress and so on, that we can call to see whether SHOW_IN_FOLDER and
OPEN_WHEN_COMPLETE commands should be enabled?

[Randy Smith (Not in Mondays), 2011-05-18 19:28:18]

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
On 2011/05/17 20:03:39, Paweł Hajdan Jr. wrote:
> On 2011/05/16 20:57:12, rdsmith wrote:
> > I think I'd like a comment here explaining this design choice.  Maybe
> something
> > like "Ideally, we'd detecting errors in opening and report them, but we
don't
> > generally have the proper interface for that to the external program that
> opens
> > the file.  So instead we spawn a check to update the UI if the file has been
> > deleted in parallel with the open."?
> 
> There is an ugliness in a way those objects communicate. DownloadItem calls
> DownloadManager here, which is going to ask DownloadItem for its ID and path,
> and then it's going to update DownloadItem's file removal flag.
> 
> How about doing the check in DownloadItem instead, i.e. making
> DownloadManager::CheckForFileRemoval a DownloadItem's method, removing
> DownloadItem::OnDownloadedFileRemoved and adding
> DownloadManager::OnDownloadedFileRemoved instead to update
|history_downloads_|?

Pawel: I'm confused about what you mean.  Dispatching the check directly from
DownloadItem (i.e. making CheckForFileRemoval a DownloadItem method) makes sense
to me.  But I don't understand the change on the return path--if you get rid of
DownloadItem::OnDownloadFileRemoved(), how do you update the state internal to
the download item?  Or are you thinking of storing that state outside of
DownloadItem?  I presume not--that seems pretty ugly to me.

[Paweł Hajdan Jr., 2011-05-18 20:03:42]

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
On 2011/05/18 19:28:18, rdsmith wrote:
> On 2011/05/17 20:03:39, Paweł Hajdan Jr. wrote:
> > On 2011/05/16 20:57:12, rdsmith wrote:
> > > I think I'd like a comment here explaining this design choice.  Maybe
> > something
> > > like "Ideally, we'd detecting errors in opening and report them, but we
> don't
> > > generally have the proper interface for that to the external program that
> > opens
> > > the file.  So instead we spawn a check to update the UI if the file has
been
> > > deleted in parallel with the open."?
> > 
> > There is an ugliness in a way those objects communicate. DownloadItem calls
> > DownloadManager here, which is going to ask DownloadItem for its ID and
path,
> > and then it's going to update DownloadItem's file removal flag.
> > 
> > How about doing the check in DownloadItem instead, i.e. making
> > DownloadManager::CheckForFileRemoval a DownloadItem's method, removing
> > DownloadItem::OnDownloadedFileRemoved and adding
> > DownloadManager::OnDownloadedFileRemoved instead to update
> |history_downloads_|?
> 
> Pawel: I'm confused about what you mean.  Dispatching the check directly from
> DownloadItem (i.e. making CheckForFileRemoval a DownloadItem method) makes
sense
> to me.  But I don't understand the change on the return path--if you get rid
of
> DownloadItem::OnDownloadFileRemoved(), how do you update the state internal to
> the download item?  Or are you thinking of storing that state outside of
> DownloadItem?  I presume not--that seems pretty ugly to me.

Sorry, I was a bit afraid that comment could be confusing.

I meant to keep the state in DownloadItem, as it currently is in the latest
patch set. Now if we dispatch the check directly from DownloadItem, we should no
longer need a *public* method DownloadItem::OnDownloadFileRemoved() - it can
still exist, just please make it private.

[Randy Smith (Not in Mondays), 2011-05-18 20:13:13]

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
On 2011/05/18 20:03:43, Paweł Hajdan Jr. wrote:
> On 2011/05/18 19:28:18, rdsmith wrote:
> > On 2011/05/17 20:03:39, Paweł Hajdan Jr. wrote:
> > > On 2011/05/16 20:57:12, rdsmith wrote:
> > > > I think I'd like a comment here explaining this design choice.  Maybe
> > > something
> > > > like "Ideally, we'd detecting errors in opening and report them, but we
> > don't
> > > > generally have the proper interface for that to the external program
that
> > > opens
> > > > the file.  So instead we spawn a check to update the UI if the file has
> been
> > > > deleted in parallel with the open."?
> > > 
> > > There is an ugliness in a way those objects communicate. DownloadItem
calls
> > > DownloadManager here, which is going to ask DownloadItem for its ID and
> path,
> > > and then it's going to update DownloadItem's file removal flag.
> > > 
> > > How about doing the check in DownloadItem instead, i.e. making
> > > DownloadManager::CheckForFileRemoval a DownloadItem's method, removing
> > > DownloadItem::OnDownloadedFileRemoved and adding
> > > DownloadManager::OnDownloadedFileRemoved instead to update
> > |history_downloads_|?
> > 
> > Pawel: I'm confused about what you mean.  Dispatching the check directly
from
> > DownloadItem (i.e. making CheckForFileRemoval a DownloadItem method) makes
> sense
> > to me.  But I don't understand the change on the return path--if you get rid
> of
> > DownloadItem::OnDownloadFileRemoved(), how do you update the state internal
to
> > the download item?  Or are you thinking of storing that state outside of
> > DownloadItem?  I presume not--that seems pretty ugly to me.
> 
> Sorry, I was a bit afraid that comment could be confusing.
> 
> I meant to keep the state in DownloadItem, as it currently is in the latest
> patch set. Now if we dispatch the check directly from DownloadItem, we should
no
> longer need a *public* method DownloadItem::OnDownloadFileRemoved() - it can
> still exist, just please make it private.

Ah, thank you!  That makes sense, but it won't work.  In general, you really
don't want to target DownloadItem methods from off-thread, because DownloadItem
can be cancelled or removed at almost any time (think shutdown races).  So all
off-thread communication with the DownloadItem needs to go through the download
manager (which is refcounted for just such an occasion).

[Paweł Hajdan Jr., 2011-05-18 20:23:44]

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
On 2011/05/18 20:13:13, rdsmith wrote:
> Ah, thank you!  That makes sense, but it won't work.  In general, you really
> don't want to target DownloadItem methods from off-thread, because
DownloadItem
> can be cancelled or removed at almost any time (think shutdown races).  So all
> off-thread communication with the DownloadItem needs to go through the
download
> manager (which is refcounted for just such an occasion).

Oh, I see. Indeed, please ignore my comment about that then.

[haraken1, 2011-06-07 12:49:17]

I am very sorry for this late reply...

Finally, I confirmed that the download shelf works as we expect on all of Mac,
Linux and Windows. Specifically, the label of the download shelf changes as
follows when we click the shelf of a removed file (or reload chrome://downloads
page). 

[Before]
foo.txt      <--- displayed at the vertical center of the shelf

[After]
foo.txt      <--- displayed at the upper half of the shelf
Removed      <--- displayed at the lower half of the shelf

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:263:
download_manager_->CheckForFileRemoval(this);
In conclusion, here I only added the comment that Randy advised me.

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:273: void
DownloadManager::CheckForFileRemoval(DownloadItem* download_item) {
On 2011/05/17 20:03:39, Paweł Hajdan Jr. wrote:
> nit: Add a BrowserThread::CurrentlyOn DCHECK here too.

Done.

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_shelf.cc:73: return !download_->IsCancelled()
&& !download_->file_externally_removed();
I created DownloadItem::CanShowInFolder() and used it here. Also, I thought that
the name of DownloadItem::CanOpenDownload(), which judges whether we can
register the type of the file so that it can always open automatically, is a bit
confusing and thus I changed the name to DownloadItem::CanOpenAlways().

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/6905049/diff/28001/chrome/browser/resources/do...
chrome/browser/resources/downloads.html:782: return;
On 2011/05/16 20:57:12, rdsmith wrote:
> Do we still need this comment and the null check for downloads?  Once
downloads
> has been created, won't the object stick around even if it's cleared (based on
> my reading of the code, but I may be wrong; I'm not a JS expert).

Done.

[haraken1, 2011-06-07 12:53:58]

> Also, I thought that the name of DownloadItem::CanOpenDownload(), which judges
whether we can
> register the type of the file so that it can always open automatically, is a
bit
> confusing and thus I changed the name to DownloadItem::CanOpenAlways().

Sorry! Please ignore the above comment. I leave the name of
DownloadItem::CanOpenDownload() as it is (since DownloadItem::CanOpenDownload()
is the method that checks if we can call OpenDownload()).

[hendrickson_a, 2011-06-07 15:38:52]

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:641: DownloadCreateInfo*
info(new DownloadCreateInfo);
You need to get the latest code.

This section will need to be changed to match the other functions.

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:295: REMOVED : "REMOVED",
I don't like this mismatch between the states in DownloadItem and in JavaScript.

[Randy Smith (Not in Mondays), 2011-06-07 18:51:09]

Quick comment; will be coming back later to do a more thorough review.

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:295: REMOVED : "REMOVED",
I'm sorry if this was covered earlier and the state's not in my brain anymore,
but why include the REMOVED status?  I'd rather not do that; a) it's not really
a state of a download, and b) what an C++ observer seeing "REMOVED" should do is
immediately stop observing the download item and remove it from any lists it
contains.   Extending that to an asynchronous renderer interaction seems like
asking for trouble.

[haraken1, 2011-06-08 05:11:49]

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:641: DownloadCreateInfo*
info(new DownloadCreateInfo);
On 2011/06/07 15:38:52, hendrickson_a wrote:
> You need to get the latest code.
> 
> This section will need to be changed to match the other functions.

Done.

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/6905049/diff/38001/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:295: REMOVED : "REMOVED",
I got it. I removed REMOVED state from Download.States. Instead, I introduced
this.fileExternallyRemoved_, which corresponds to
DownloadItem::file_externally_removed_ in C++. 


On 2011/06/07 18:51:10, rdsmith wrote:
> I'm sorry if this was covered earlier and the state's not in my brain anymore,
> but why include the REMOVED status?  I'd rather not do that; a) it's not
really
> a state of a download, and b) what an C++ observer seeing "REMOVED" should do
is
> immediately stop observing the download item and remove it from any lists it
> contains.   Extending that to an asynchronous renderer interaction seems like
> asking for trouble.

[Paweł Hajdan Jr., 2011-06-08 09:58:40]

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:240: return
!Extension::IsExtension(state_info_.target_name);
Shouldn't you check file_externally_removed_ here?

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:138: // Whether it is OK to open a
folder which this file is inside.
nit: Please keep the "Returns true if" style; "whether" might be ambiguous.

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:697: bool removed =
file_util::Delete(new_path, false);
nit: Why a temp variable? Just ASSERT_TRUE(file_util::Delete...).

[haraken1, 2011-06-08 10:35:07]

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:240: return
!Extension::IsExtension(state_info_.target_name);
I fixed it. 

(However, we may not have to check |file_externally_removed_| here, since I feel
that it is not so strange to allow a user to click "Always Open Files of This
Type" even for a removed file.)

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_item.h:138: // Whether it is OK to open a
folder which this file is inside.
On 2011/06/08 09:58:41, Paweł Hajdan Jr. wrote:
> nit: Please keep the "Returns true if" style; "whether" might be ambiguous.

Done.

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6905049/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:697: bool removed =
file_util::Delete(new_path, false);
On 2011/06/08 09:58:41, Paweł Hajdan Jr. wrote:
> nit: Why a temp variable? Just ASSERT_TRUE(file_util::Delete...).

Done.

[Randy Smith (Not in Mondays), 2011-06-08 22:07:54]

erg, could you take a look at the GTK specific sections?  thakis, dmazzoni, at
the Cocoa and Views specific sections?

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/download/dow...
File chrome/browser/download/download_shelf_context_menu.cc (right):

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/download/dow...
chrome/browser/download/download_shelf_context_menu.cc:44: return
download_item_->CanShowInFolder();
This looks wrong to me--why should the open when complete flag be set in the UI
based on CanShowInFolder?

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:190: if (this.state_ !=
Download.States.COMPLETE || this.fileExternallyRemoved_) {
It may be my lack of javascript, but it doesn't look like this.state_ or
this.fileExternallyRemoved_ has been set at this point in the code; where do
those values come from?

[Elliot Glaysher, 2011-06-08 22:13:18]

On 2011/06/08 22:07:54, rdsmith wrote:
> erg, could you take a look at the GTK specific sections?

The GTK code LGTM.

[Nico, 2011-06-08 22:18:33]

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_item_cell.mm:599: [self animation:nil
progressed:0.0];
Don't you want to animate this in if the title is currently not visible?

[haraken1, 2011-06-09 05:25:55]

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/download/dow...
File chrome/browser/download/download_shelf_context_menu.cc (right):

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/download/dow...
chrome/browser/download/download_shelf_context_menu.cc:44: return
download_item_->CanShowInFolder();
You are right. I fixed it.

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:190: if (this.state_ !=
Download.States.COMPLETE || this.fileExternallyRemoved_) {
My mistake. I fixed it.

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):

http://codereview.chromium.org/6905049/diff/49003/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_item_cell.mm:599: [self animation:nil
progressed:0.0];
Done. I added showStatusAnimation_ for the animation.

[haraken1, 2011-06-13 14:05:41]

Would you please take a look at it?
Cocoa change: thakis
Views change: dmazzoni


2011年6月9日14:25  <haraken@google.com>:
>
>
http://codereview.chromium.org/6905049/diff/49003/chrome/browser/download/dow...
> File chrome/browser/download/download_shelf_context_menu.cc (right):
>
>
http://codereview.chromium.org/6905049/diff/49003/chrome/browser/download/dow...
> chrome/browser/download/download_shelf_context_menu.cc:44: return
> download_item_->CanShowInFolder();
> You are right. I fixed it.
>
>
http://codereview.chromium.org/6905049/diff/49003/chrome/browser/resources/do...
> File chrome/browser/resources/downloads.js (right):
>
>
http://codereview.chromium.org/6905049/diff/49003/chrome/browser/resources/do...
> chrome/browser/resources/downloads.js:190: if (this.state_ !=
> Download.States.COMPLETE || this.fileExternallyRemoved_) {
> My mistake. I fixed it.
>
>
http://codereview.chromium.org/6905049/diff/49003/chrome/browser/ui/cocoa/dow...
> File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):
>
>
http://codereview.chromium.org/6905049/diff/49003/chrome/browser/ui/cocoa/dow...
> chrome/browser/ui/cocoa/download/download_item_cell.mm:599: [self
> animation:nil progressed:0.0];
> Done. I added showStatusAnimation_ for the animation.
>
> http://codereview.chromium.org/6905049/
>



-- 
=====================
Kentaro Hara (原健太朗)
http://haraken.info
=====================

[Paweł Hajdan Jr., 2011-06-13 14:34:08]

LGTM

[haraken1, 2011-06-13 16:27:48]

Pawel: Thank you very much for the long discussion.

If randy also says LGTM, I would like to commit.

[Randy Smith (Not in Mondays), 2011-06-13 18:28:49]

LGTM.  Please don't commit without LGTMs from thakis and dmazzoni--I don't
consider myself competent to review the UI files.

[Nico, 2011-06-13 18:35:08]

LGTM

[haraken1, 2011-06-13 23:55:35]

+pkasting

I found dmazzoni is sabbatical.

Pkasting: Would you please take a look at the views code?

chrome/browser/ui/views/download/download_item_view.h
chrome/browser/ui/views/download/download_item_view.cc

[Peter Kasting, 2011-06-14 18:26:06]

views LGTM

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_item_view.cc (right):

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_item_view.cc:855: (box_height_ -
font_.GetHeight()) / 2 : kVerticalPadding);
Nit: I suggest putting the "expr / 2" part in its own set of parens.

You can wrap this all like:

    int y = box_y_ + (status_text_.empty() ?
        ((box_height_ - font_.GetHeight()) / 2) : kVerticalPadding);

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:127: // Add oneself to all
download items as an observer.
Nit: oneself -> ourself

[haraken1, 2011-06-15 00:59:29]

I merged with the latest revision and re-confirmed that this patch works as
expected on all of Windows, Mac and Linux. I committed it.

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_item_view.cc (right):

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_item_view.cc:855: (box_height_ -
font_.GetHeight()) / 2 : kVerticalPadding);
On 2011/06/14 18:26:06, Peter Kasting wrote:
> Nit: I suggest putting the "expr / 2" part in its own set of parens.
> 
> You can wrap this all like:
> 
>     int y = box_y_ + (status_text_.empty() ?
>         ((box_height_ - font_.GetHeight()) / 2) : kVerticalPadding);

Done.

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/6905049/diff/51004/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:127: // Add oneself to all
download items as an observer.
On 2011/06/14 18:26:06, Peter Kasting wrote:
> Nit: oneself -> ourself

Done.

[commit-bot: I haz the power, 2011-06-15 08:17:55]

Change committed as 89148