Fire a notification in Windows 8 metro chrome which informs the user about a completed download. Th…

Fire a notification in Windows 8 metro chrome which informs the user about a completed download. This is achieved
by a new class DownloadCompletionObserver which lives in the newly added file download_completion_observer_win.cc/.h
in chrome/browser/download. This class observes the DownloadManager and the DownloadItem to get information about
download items getting created and completing.

BUG=133109
R=cpu,asanka
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150597

[asanka, 2012-08-07 17:31:58]

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_completion_observer_win.cc (right):

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_completion_observer_win.cc:28:
item->AddObserver(this);
Only add an observer if item->IsInProgress() is true since those are the
downloads you are interested in.

Note that OnDownloadCreated() is called for each download item in history when
the history DB is being read. item->IsInProgress() will be false for history
downloads.

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_completion_observer_win.cc:38: 
It is possible that the user has already marked the download to be opened upon
completion (item->GetOpenWhenComplete() is true), or it could be of a file type
that the user wants to open by default (item->ShouldOpenFileBasedOnExtension()
is true). I think you don't want to show notifications in those cases.

Also, ignore downloads that are temporary (item->IsTemporary() is true). Those
may not be meant to be handed over to the user as-is after the download
completes.

In addition, ignore downloads that have already been opened by the time it is
marked as COMPLETE (item->GetAutoOpened() is true). Currently this happens for
extension downloads.

Note that all of the above predicates should be checked after the download is in
the COMPLETE state.

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_completion_observer_win.cc:64: }
I suggest removing the observer when any of the terminal states are hit:
COMPLETE, INTERRUPTED, CANCELLED and also REMOVING. You are only interested in
the the transition from IN_PROGRESS -> COMPLETE.

This also implicitly deals with the possibility of getting another
OnDownloadUpdated() while the download is in the COMPLETE state. Currently that
would trigger a second notification.

[ananta, 2012-08-07 18:08:28]

Hi Asanka

Thanks for the detailed review comments. PTAL

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_completion_observer_win.cc (right):

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_completion_observer_win.cc:28:
item->AddObserver(this);
On 2012/08/07 17:31:58, asanka wrote:
> Only add an observer if item->IsInProgress() is true since those are the
> downloads you are interested in.
> 
> Note that OnDownloadCreated() is called for each download item in history when
> the history DB is being read. item->IsInProgress() will be false for history
> downloads.

Done.

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_completion_observer_win.cc:38: 
On 2012/08/07 17:31:58, asanka wrote:
> It is possible that the user has already marked the download to be opened upon
> completion (item->GetOpenWhenComplete() is true), or it could be of a file
type
> that the user wants to open by default (item->ShouldOpenFileBasedOnExtension()
> is true). I think you don't want to show notifications in those cases.
> 
> Also, ignore downloads that are temporary (item->IsTemporary() is true). Those
> may not be meant to be handed over to the user as-is after the download
> completes.
> 
> In addition, ignore downloads that have already been opened by the time it is
> marked as COMPLETE (item->GetAutoOpened() is true). Currently this happens for
> extension downloads.
> 
> Note that all of the above predicates should be checked after the download is
in
> the COMPLETE state.

Done.

http://codereview.chromium.org/10832174/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_completion_observer_win.cc:64: }
On 2012/08/07 17:31:58, asanka wrote:
> I suggest removing the observer when any of the terminal states are hit:
> COMPLETE, INTERRUPTED, CANCELLED and also REMOVING. You are only interested in
> the the transition from IN_PROGRESS -> COMPLETE.
> 
> This also implicitly deals with the possibility of getting another
> OnDownloadUpdated() while the download is in the COMPLETE state. Currently
that
> would trigger a second notification.
> 

Done.

[asanka, 2012-08-07 19:05:15]

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
File chrome/browser/download/download_completion_observer_win.cc (right):

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
chrome/browser/download/download_completion_observer_win.cc:33:
manager->RemoveObserver(this);
There are some lifetime issues here. Notably, it would be great if we can delete
|this| when we get the ManagerGoingDown() notification. Unfortunately,
ManagerGoingDown() is received prior to OnDownloadUpdate() notifications from
any active downloads that are being torn down as a result of the download
manager going down.

So for proper cleanup, you might have to keep a list of DownloadItems that are
being observed and call RemoveObserver() on them during ManagerGoingDown(). Then
you can properly dispose of |this| here.

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
chrome/browser/download/download_completion_observer_win.cc:65: "",
Nit: Indent

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
File chrome/browser/download/download_completion_observer_win.h (right):

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
chrome/browser/download/download_completion_observer_win.h:19:
DownloadCompletionObserver() {}
Nit: Perhaps provide a DownloadManager* to the constructor and get rid of the
Initialize() call.

[ananta, 2012-08-07 21:40:31]

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
File chrome/browser/download/download_completion_observer_win.cc (right):

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
chrome/browser/download/download_completion_observer_win.cc:33:
manager->RemoveObserver(this);
On 2012/08/07 19:05:15, asanka wrote:
> There are some lifetime issues here. Notably, it would be great if we can
delete
> |this| when we get the ManagerGoingDown() notification. Unfortunately,
> ManagerGoingDown() is received prior to OnDownloadUpdate() notifications from
> any active downloads that are being torn down as a result of the download
> manager going down.
> 
> So for proper cleanup, you might have to keep a list of DownloadItems that are
> being observed and call RemoveObserver() on them during ManagerGoingDown().
Then
> you can properly dispose of |this| here.

Done. Maintained the list of items in a set.

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
chrome/browser/download/download_completion_observer_win.cc:65: "",
On 2012/08/07 19:05:15, asanka wrote:
> Nit: Indent

Done.

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
File chrome/browser/download/download_completion_observer_win.h (right):

http://codereview.chromium.org/10832174/diff/8002/chrome/browser/download/dow...
chrome/browser/download/download_completion_observer_win.h:19:
DownloadCompletionObserver() {}
On 2012/08/07 19:05:15, asanka wrote:
> Nit: Perhaps provide a DownloadManager* to the constructor and get rid of the
> Initialize() call.

Done.

[asanka, 2012-08-07 23:29:59]

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
File chrome/browser/download/download_completion_observer_win.cc (right):

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:29:
ClearDownloadItems();
If you are going to delete |this| in ManagerGoingDown() you don't need to call
ClearDownloadItems() here. Instead, perhaps a DCHECK that the set is empty?

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:42:
manager->RemoveObserver(this);
Did you intend to delete |this| here?

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:81: if
(ContainsKey(download_items_, download)) {
Nit: This should be a DCHECK() and the .erase() and RemoveObserver() calls
should be unconditional.

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:91: if
(ContainsKey(download_items_, download)) {
Ditto.

[ananta, 2012-08-07 23:44:25]

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
File chrome/browser/download/download_completion_observer_win.cc (right):

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:29:
ClearDownloadItems();
On 2012/08/07 23:29:59, asanka wrote:
> If you are going to delete |this| in ManagerGoingDown() you don't need to call
> ClearDownloadItems() here. Instead, perhaps a DCHECK that the set is empty?

Done.

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:42:
manager->RemoveObserver(this);
On 2012/08/07 23:29:59, asanka wrote:
> Did you intend to delete |this| here?
> 
Yeah. Thanks for catching this

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:81: if
(ContainsKey(download_items_, download)) {
On 2012/08/07 23:29:59, asanka wrote:
> Nit: This should be a DCHECK() and the .erase() and RemoveObserver() calls
> should be unconditional.

Done. Yeah. We should not be receiving status updates for items we have not
setup observers on.

http://codereview.chromium.org/10832174/diff/11001/chrome/browser/download/do...
chrome/browser/download/download_completion_observer_win.cc:91: if
(ContainsKey(download_items_, download)) {
On 2012/08/07 23:29:59, asanka wrote:
> Ditto.

Done.

[asanka, 2012-08-08 17:37:51]

LGTM

[jam, 2012-08-08 18:05:24]

gyp lgtm