Download shelf autohides on showing in shell, just same as regular open

Download shelf autohides on showing in shell, just same as regular open

Autohide is activated only if user has 'shown' the download item in shell after download has been completed.

BUG=89924
R=benjhayden@chromium.org

[DukeXar, 2014-03-24 00:25:13]

Could you please review this patch.

[benjhayden, 2014-03-24 18:07:45]

+felt, asanka, rdsmith: Can one of you help us figure out what the shelf should
do when items are Shown?

I'm not sure if it makes sense to hide the shelf when an item is Shown. I
thought that Showing an item in a file manager doesn't necessarily draw
attention to the particular file being Shown, which could be one among many
similar files. It's been a while since I worked on the shelf, so I'll have to
forward you to some people who have worked on it more recently and can say more
certainly whether the shelf should be closed when all items have been either
Shown or Opened.

Even if the shelf should be hidden, most observers of the open event probably
should not treat Show the same as Open, so if there are any observers that need
to know when an item is Shown, then I'd advise adding a separate Observer
method. If there are not any observers that need to know when an item is Shown,
then this patch may be unnecessary...

This patch is a straightforward copy-paste of several complex lines, so the
copied code would need to be refactored into a new private method that is called
in two places. If you add a separate Observer method, then the copied code may
be different enough that you don't need to refactor, or you may need to refactor
less of the copied code.

Thanks for contributing to Chromium!

[Randy Smith (Not in Mondays), 2014-03-24 18:12:20]

I think Asanka's the right person to review this patch; I'll stay out of it
unless he requests my opinion.

[DukeXar, 2014-03-24 18:31:23]

On 2014/03/24 18:07:45, benjhayden_chromium wrote:
> +felt, asanka, rdsmith: Can one of you help us figure out what the shelf
should
> do when items are Shown?
> 
> I'm not sure if it makes sense to hide the shelf when an item is Shown. I
> thought that Showing an item in a file manager doesn't necessarily draw
> attention to the particular file being Shown, which could be one among many
> similar files. It's been a while since I worked on the shelf, so I'll have to
> forward you to some people who have worked on it more recently and can say
more
> certainly whether the shelf should be closed when all items have been either
> Shown or Opened.
> 
> Even if the shelf should be hidden, most observers of the open event probably
> should not treat Show the same as Open, so if there are any observers that
need
> to know when an item is Shown, then I'd advise adding a separate Observer
> method. If there are not any observers that need to know when an item is
Shown,
> then this patch may be unnecessary...
> 
> This patch is a straightforward copy-paste of several complex lines, so the
> copied code would need to be refactored into a new private method that is
called
> in two places. If you add a separate Observer method, then the copied code may
> be different enough that you don't need to refactor, or you may need to
refactor
> less of the copied code.
> 
> Thanks for contributing to Chromium!

I started working on this bug because I found that it would help me in my
everyday
use of the browser, as I use the 'show in folder' function quite frequently, and
the mentioned 'autohide' functionality for 'Shown' would be quite convenient.

The line 467-473 were definitely copy-pasted, I had no idea on how to name the
method for those lines, as they do not do a single job.

I like the idea of adding separate Observer method for Shown. I have considered
that but didn't know if that fits the design, as you have already mentioned ("if
there are any Observers that need to know when an item is Shown").

Considering all of that, I decided to send the patch, so you you can forward me
in a right direction.

Thanks!

[asanka, 2014-03-25 19:54:33]

All comments are welcome!

I'm in favor of this behavior change. But there are a few details of the
behavior I'd like to hash out and also I'm not fond of overloading the meaning
of 'Open' to also mean 'opened the folder containing the download.'

I propose moving the UX discussion to the issue and then coming back here to
talk about coding issues.

https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
File content/browser/download/download_item_impl.cc (right):

https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
content/browser/download/download_item_impl.cc:464: return;
We support 'Open in shell/finder' for downloads that are still in progress. This
opens a shell and focuses the intermediate file.

[DukeXar, 2014-03-31 10:51:22]

On 2014/03/25 19:54:33, asanka wrote:
> All comments are welcome!
> 
> I'm in favor of this behavior change. But there are a few details of the
> behavior I'd like to hash out and also I'm not fond of overloading the meaning
> of 'Open' to also mean 'opened the folder containing the download.'
> 
> I propose moving the UX discussion to the issue and then coming back here to
> talk about coding issues.
> 
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> File content/browser/download/download_item_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> content/browser/download/download_item_impl.cc:464: return;
> We support 'Open in shell/finder' for downloads that are still in progress.
This
> opens a shell and focuses the intermediate file.

This proposal looks pretty good for me
https://code.google.com/p/chromium/issues/detail?id=89924#c8
Should I go with implementation, considering the review issues as well?

[asanka, 2014-03-31 20:40:18]

On 2014/03/31 10:51:22, DukeXar wrote:
> On 2014/03/25 19:54:33, asanka wrote:
> > All comments are welcome!
> > 
> > I'm in favor of this behavior change. But there are a few details of the
> > behavior I'd like to hash out and also I'm not fond of overloading the
meaning
> > of 'Open' to also mean 'opened the folder containing the download.'
> > 
> > I propose moving the UX discussion to the issue and then coming back here to
> > talk about coding issues.
> > 
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > File content/browser/download/download_item_impl.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > content/browser/download/download_item_impl.cc:464: return;
> > We support 'Open in shell/finder' for downloads that are still in progress.
> This
> > opens a shell and focuses the intermediate file.
> 
> This proposal looks pretty good for me
> https://code.google.com/p/chromium/issues/detail?id=89924#c8
> Should I go with implementation, considering the review issues as well?

Sure. Looks like there are no objections to the behavior.

[DukeXar, 2014-05-01 18:40:04]

There will be two separate flags in download item: 'opened' and 'shown'. The
observer will have a callback for 'shown', and download shelf will respond to
that callback to autohide on 'shown' items.
The users of download item can distinguish between two different actions, but is
it required to separate these things in history? It is either needed to add a
'shown' flag into history database along with 'opened', or change the meaning of
'opened' (in database) to 'actioned' and have only one flag in download item -
'actioned', that would be set to true when user did something on download item.
So there will be separate callbacks in observer, but one state in item. I don't
think this is a good solution, so I would change the database instead.
What do you think?

On 2014/03/31 20:40:18, asanka wrote:
> On 2014/03/31 10:51:22, DukeXar wrote:
> > On 2014/03/25 19:54:33, asanka wrote:
> > > All comments are welcome!
> > > 
> > > I'm in favor of this behavior change. But there are a few details of the
> > > behavior I'd like to hash out and also I'm not fond of overloading the
> meaning
> > > of 'Open' to also mean 'opened the folder containing the download.'
> > > 
> > > I propose moving the UX discussion to the issue and then coming back here
to
> > > talk about coding issues.
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > File content/browser/download/download_item_impl.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > content/browser/download/download_item_impl.cc:464: return;
> > > We support 'Open in shell/finder' for downloads that are still in
progress.
> > This
> > > opens a shell and focuses the intermediate file.
> > 
> > This proposal looks pretty good for me
> > https://code.google.com/p/chromium/issues/detail?id=89924#c8
> > Should I go with implementation, considering the review issues as well?
> 
> Sure. Looks like there are no objections to the behavior.

[benjhayden, 2014-05-01 18:55:08]

On 2014/05/01 18:40:04, DukeXar wrote:
> There will be two separate flags in download item: 'opened' and 'shown'. The
> observer will have a callback for 'shown', and download shelf will respond to
> that callback to autohide on 'shown' items.
> The users of download item can distinguish between two different actions, but
is
> it required to separate these things in history? It is either needed to add a
> 'shown' flag into history database along with 'opened', or change the meaning
of
> 'opened' (in database) to 'actioned' and have only one flag in download item -
> 'actioned', that would be set to true when user did something on download
item.
> So there will be separate callbacks in observer, but one state in item. I
don't
> think this is a good solution, so I would change the database instead.
> What do you think?
> 
> On 2014/03/31 20:40:18, asanka wrote:
> > On 2014/03/31 10:51:22, DukeXar wrote:
> > > On 2014/03/25 19:54:33, asanka wrote:
> > > > All comments are welcome!
> > > > 
> > > > I'm in favor of this behavior change. But there are a few details of the
> > > > behavior I'd like to hash out and also I'm not fond of overloading the
> > meaning
> > > > of 'Open' to also mean 'opened the folder containing the download.'
> > > > 
> > > > I propose moving the UX discussion to the issue and then coming back
here
> to
> > > > talk about coding issues.
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > > File content/browser/download/download_item_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > > content/browser/download/download_item_impl.cc:464: return;
> > > > We support 'Open in shell/finder' for downloads that are still in
> progress.
> > > This
> > > > opens a shell and focuses the intermediate file.
> > > 
> > > This proposal looks pretty good for me
> > > https://code.google.com/p/chromium/issues/detail?id=89924#c8
> > > Should I go with implementation, considering the review issues as well?
> > 
> > Sure. Looks like there are no objections to the behavior.

Is there a reason that 'shown' needs to be persisted, other than consistency
with 'opened'?

[DukeXar, 2014-05-01 23:02:33]

On 2014/05/01 18:55:08, benjhayden_chromium wrote:
> On 2014/05/01 18:40:04, DukeXar wrote:
> > There will be two separate flags in download item: 'opened' and 'shown'. The
> > observer will have a callback for 'shown', and download shelf will respond
to
> > that callback to autohide on 'shown' items.
> > The users of download item can distinguish between two different actions,
but
> is
> > it required to separate these things in history? It is either needed to add
a
> > 'shown' flag into history database along with 'opened', or change the
meaning
> of
> > 'opened' (in database) to 'actioned' and have only one flag in download item
-
> > 'actioned', that would be set to true when user did something on download
> item.
> > So there will be separate callbacks in observer, but one state in item. I
> don't
> > think this is a good solution, so I would change the database instead.
> > What do you think?
> > 
> > On 2014/03/31 20:40:18, asanka wrote:
> > > On 2014/03/31 10:51:22, DukeXar wrote:
> > > > On 2014/03/25 19:54:33, asanka wrote:
> > > > > All comments are welcome!
> > > > > 
> > > > > I'm in favor of this behavior change. But there are a few details of
the
> > > > > behavior I'd like to hash out and also I'm not fond of overloading the
> > > meaning
> > > > > of 'Open' to also mean 'opened the folder containing the download.'
> > > > > 
> > > > > I propose moving the UX discussion to the issue and then coming back
> here
> > to
> > > > > talk about coding issues.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > > > File content/browser/download/download_item_impl.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > > > content/browser/download/download_item_impl.cc:464: return;
> > > > > We support 'Open in shell/finder' for downloads that are still in
> > progress.
> > > > This
> > > > > opens a shell and focuses the intermediate file.
> > > > 
> > > > This proposal looks pretty good for me
> > > > https://code.google.com/p/chromium/issues/detail?id=89924#c8
> > > > Should I go with implementation, considering the review issues as well?
> > > 
> > > Sure. Looks like there are no objections to the behavior.
> 
> Is there a reason that 'shown' needs to be persisted, other than consistency
> with 'opened'?

I don't see any. Visually (have not checked that part of the code yet), the
"open" flag doesn't have any effects after browser closing and reopening. The
not opened download is not indicated anyhow. It means that "shown" is not going
to have any effects as well.

[DukeXar, 2014-05-13 23:36:36]

On 2014/05/01 23:02:33, DukeXar wrote:
> On 2014/05/01 18:55:08, benjhayden_chromium wrote:
> > On 2014/05/01 18:40:04, DukeXar wrote:
> > > There will be two separate flags in download item: 'opened' and 'shown'.
The
> > > observer will have a callback for 'shown', and download shelf will respond
> to
> > > that callback to autohide on 'shown' items.
> > > The users of download item can distinguish between two different actions,
> but
> > is
> > > it required to separate these things in history? It is either needed to
add
> a
> > > 'shown' flag into history database along with 'opened', or change the
> meaning
> > of
> > > 'opened' (in database) to 'actioned' and have only one flag in download
item
> -
> > > 'actioned', that would be set to true when user did something on download
> > item.
> > > So there will be separate callbacks in observer, but one state in item. I
> > don't
> > > think this is a good solution, so I would change the database instead.
> > > What do you think?
> > > 
> > > On 2014/03/31 20:40:18, asanka wrote:
> > > > On 2014/03/31 10:51:22, DukeXar wrote:
> > > > > On 2014/03/25 19:54:33, asanka wrote:
> > > > > > All comments are welcome!
> > > > > > 
> > > > > > I'm in favor of this behavior change. But there are a few details of
> the
> > > > > > behavior I'd like to hash out and also I'm not fond of overloading
the
> > > > meaning
> > > > > > of 'Open' to also mean 'opened the folder containing the download.'
> > > > > > 
> > > > > > I propose moving the UX discussion to the issue and then coming back
> > here
> > > to
> > > > > > talk about coding issues.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > > > > File content/browser/download/download_item_impl.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/209613002/diff/1/content/browser/downl...
> > > > > > content/browser/download/download_item_impl.cc:464: return;
> > > > > > We support 'Open in shell/finder' for downloads that are still in
> > > progress.
> > > > > This
> > > > > > opens a shell and focuses the intermediate file.
> > > > > 
> > > > > This proposal looks pretty good for me
> > > > > https://code.google.com/p/chromium/issues/detail?id=89924#c8
> > > > > Should I go with implementation, considering the review issues as
well?
> > > > 
> > > > Sure. Looks like there are no objections to the behavior.
> > 
> > Is there a reason that 'shown' needs to be persisted, other than consistency
> > with 'opened'?
> 
> I don't see any. Visually (have not checked that part of the code yet), the
> "open" flag doesn't have any effects after browser closing and reopening. The
> not opened download is not indicated anyhow. It means that "shown" is not
going
> to have any effects as well.

I have updated the code so that a separate callback is used for 'show'
notification. The change is pretty straightforward, though.

[asanka, 2014-05-14 18:40:59]

https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_shelf_view.cc:164: void
DownloadShelfView::ShownDownload(DownloadItemView* view) {
Shall we rename this DownloadShown and also rename the method above to
DownloadOpened ? Or may be just call then OnDownloadOpened and OnDownloadShown
to match the observer methods.

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
content/browser/download/download_item_impl.cc:157: shownWhenComplete_(false),
Nit: Use hacker_style_.

https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
content/public/browser/download_item.h:332: virtual void
SetShownWhenComplete(bool shown) = 0;
I'd like to get rid of both SetOpened() and SetShownWhenComplete(). Instead we
can opt to track the "acted on" state in DownloadItemModel. It might be that
once we have that bit, then both GetOpened()/GetShownWhenComplete() can also be
removed.

We'd still need the OnDownloadShown() notification since that would then be a
non-state-changing operation that's not detectable from examining the
DownloadItem.

[DukeXar, 2014-06-03 15:27:52]

On 2014/05/14 18:40:59, asanka wrote:
>
https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/download/download_shelf_view.cc (right):
> 
>
https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/download/download_shelf_view.cc:164: void
> DownloadShelfView::ShownDownload(DownloadItemView* view) {
> Shall we rename this DownloadShown and also rename the method above to
> DownloadOpened ? Or may be just call then OnDownloadOpened and OnDownloadShown
> to match the observer methods.
> 
>
https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
> File content/browser/download/download_item_impl.cc (right):
> 
>
https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
> content/browser/download/download_item_impl.cc:157: shownWhenComplete_(false),
> Nit: Use hacker_style_.
> 
>
https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
> File content/public/browser/download_item.h (right):
> 
>
https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
> content/public/browser/download_item.h:332: virtual void
> SetShownWhenComplete(bool shown) = 0;
> I'd like to get rid of both SetOpened() and SetShownWhenComplete(). Instead we
> can opt to track the "acted on" state in DownloadItemModel. It might be that
> once we have that bit, then both GetOpened()/GetShownWhenComplete() can also
be
> removed.
> 
> We'd still need the OnDownloadShown() notification since that would then be a
> non-state-changing operation that's not detectable from examining the
> DownloadItem.

@asanka, I am putting the 'actioned' state into DownloadItemModelData. So that
SetOpened, Set/GetShownWhenComplete goes away from DownloadItem completely. What
do you think? However, this leads to that DownloadItemView will have to provide
access to model_, so that DownloadShelfView will be able to do
download_views_[i]->download_model().SetUserActioned(true).

[asanka, 2014-06-05 19:26:18]

On 2014/06/03 15:27:52, DukeXar wrote:
> On 2014/05/14 18:40:59, asanka wrote:
> >
>
https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/download/download_shelf_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/download/download_shelf_view.cc:164: void
> > DownloadShelfView::ShownDownload(DownloadItemView* view) {
> > Shall we rename this DownloadShown and also rename the method above to
> > DownloadOpened ? Or may be just call then OnDownloadOpened and
OnDownloadShown
> > to match the observer methods.
> > 
> >
>
https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
> > File content/browser/download/download_item_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
> > content/browser/download/download_item_impl.cc:157:
shownWhenComplete_(false),
> > Nit: Use hacker_style_.
> > 
> >
>
https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
> > File content/public/browser/download_item.h (right):
> > 
> >
>
https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
> > content/public/browser/download_item.h:332: virtual void
> > SetShownWhenComplete(bool shown) = 0;
> > I'd like to get rid of both SetOpened() and SetShownWhenComplete(). Instead
we
> > can opt to track the "acted on" state in DownloadItemModel. It might be that
> > once we have that bit, then both GetOpened()/GetShownWhenComplete() can also
> be
> > removed.
> > 
> > We'd still need the OnDownloadShown() notification since that would then be
a
> > non-state-changing operation that's not detectable from examining the
> > DownloadItem.
> 
> @asanka, I am putting the 'actioned' state into DownloadItemModelData. So that
> SetOpened, Set/GetShownWhenComplete goes away from DownloadItem completely.
What
> do you think? However, this leads to that DownloadItemView will have to
provide
> access to model_, so that DownloadShelfView will be able to do
> download_views_[i]->download_model().SetUserActioned(true).

Whenever you have access to a DownloadItem, you can instantiate a model using
DownloadItemModel(download_item). All the instances share the same
DownloadItemModelData, so you can use any of them to call SetUserActioned(true).

[asanka, 2014-06-05 19:26:28]

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
content/browser/download/download_item_impl.h:549: bool shownWhenComplete_;
Nit: unix_hacker_style_

[DukeXar, 2014-06-06 15:07:14]

https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

https://codereview.chromium.org/209613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_shelf_view.cc:164: void
DownloadShelfView::ShownDownload(DownloadItemView* view) {
On 2014/05/14 18:40:59, asanka wrote:
> Shall we rename this DownloadShown and also rename the method above to
> DownloadOpened ? Or may be just call then OnDownloadOpened and OnDownloadShown
> to match the observer methods.

Renamed to match observer methods.

Done.

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
content/browser/download/download_item_impl.cc:157: shownWhenComplete_(false),
On 2014/05/14 18:40:59, asanka wrote:
> Nit: Use hacker_style_.
Removed

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/209613002/diff/20001/content/browser/download...
content/browser/download/download_item_impl.h:549: bool shownWhenComplete_;
On 2014/06/05 19:26:28, asanka wrote:
> Nit: unix_hacker_style_

Well, I have deleted it at all.

https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/209613002/diff/20001/content/public/browser/d...
content/public/browser/download_item.h:332: virtual void
SetShownWhenComplete(bool shown) = 0;
On 2014/05/14 18:40:59, asanka wrote:
> I'd like to get rid of both SetOpened() and SetShownWhenComplete(). Instead we
> can opt to track the "acted on" state in DownloadItemModel. It might be that
> once we have that bit, then both GetOpened()/GetShownWhenComplete() can also
be
> removed.
> 
> We'd still need the OnDownloadShown() notification since that would then be a
> non-state-changing operation that's not detectable from examining the
> DownloadItem.

Done.

[DukeXar, 2014-06-06 15:11:30]

Could you please look at recent patch, please.

I have built and tested this on Linux and Mac.

Not sure why is .gitmodules included in patch.

[asanka, 2014-06-12 21:08:48]

https://codereview.chromium.org/209613002/diff/60001/chrome/browser/download/...
File chrome/browser/download/download_item_model.cc (right):

https://codereview.chromium.org/209613002/diff/60001/chrome/browser/download/...
chrome/browser/download/download_item_model.cc:42: public DownloadItem::Observer
{
Let's try to avoid having the DownloadItemModelData observe the DownloadItem.
Having the DownloadItemModel change state based on observing the DownloadItem is
a bit dangerous because the order in which observers are invoked is undefined.
So another observer that looks at DownloadItemModel during
OnDownloadOpened/Shown would see a different state based on whether it's invoked
before or after the DownloadItemModel's Downloaditem Observer.

Both Open and ShowInFolder actions are ultimately carried out by
ChromeDownloadManagerDelegate. The methods you are looking for are
OpenDownload() and ShowDownloadInShell(). You can have those methods call
DownloadItemModel(download_item).SetUserActed(true) as necessary. The
ShowDownloadInShell() method can also check if the show in folder was invoked
after the download was complete or not.

Pay special attention to the order of events when a download is opened or shown
in the shelf. Currently the obsevers are invoked before the delegate
(ChromeDownloadManagerDelegate). If necessary we can switch the order around as
long as we can verify that it won't break any assumptions.

https://codereview.chromium.org/209613002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (right):

https://codereview.chromium.org/209613002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller.mm:269: return
bridge_->download_model();
This method shouldn't be necessary. If you have a DownloadItem*, you can always
create a DownloadItemModel using it.

https://codereview.chromium.org/209613002/diff/60001/content/public/test/mock...
File content/public/test/mock_download_item.h (right):

https://codereview.chromium.org/209613002/diff/60001/content/public/test/mock...
content/public/test/mock_download_item.h:95:
MOCK_CONST_METHOD0(GetShownWhenComplete, bool());
Not used?

[DukeXar, 2015-11-29 22:46:51]

Hello, I have updated this quite old code review.

I think I have now implemented it in a way that was suggested by @asanka. Can
you please take a look?

I am not sure how tests for this could be implemented for this - please advise.

The changes have not touched download history, download API and stats, so I have
questions about that:
1. Should it record any stats event for 'show' as it does in RecordOpen?
2. Should it be exposed in API (is there any need for that at all, as it seems
that having just 'opened' is probably fine).
3. Should it be recorded in history?

Thank you.

[asanka, 2015-12-01 15:55:54]

On 2015/11/29 at 22:46:51, DukeXar wrote:
> Hello, I have updated this quite old code review.

Code LG so far.

> I think I have now implemented it in a way that was suggested by @asanka. Can
you please take a look?
> 
> I am not sure how tests for this could be implemented for this - please
advise.

See //chrome/browser/download/download_browsertest.cc for some examples.
Specifically the DownloadTestWithShelf.AutoOpen test at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
. You could do something similar where you manually trigger a download to be
shown in the folder and then see if the download shelf is still open.

The tricky bit is to mock out the actual shell invocation so that the browser
test doesn't interact with the actual desktop. We don't want browser_tests to
interact with the desktop or open random application windows. So you'd need to
break up the Open/ShowInFolder logic in ChromeDownloadManagerDelegate does
everything except actually invoke the platform when showing a download in the
shell during the browser_test.

> The changes have not touched download history, download API and stats, so I
have questions about that:
> 1. Should it record any stats event for 'show' as it does in RecordOpen?
> 2. Should it be exposed in API (is there any need for that at all, as it seems
that having just 'opened' is probably fine).
> 3. Should it be recorded in history?
> 
> Thank you.

[asanka, 2015-12-02 15:59:38]

On 2015/12/01 at 15:55:54, asanka wrote:
> On 2015/11/29 at 22:46:51, DukeXar wrote:
> > Hello, I have updated this quite old code review.
> 
> Code LG so far.
> 
> > I think I have now implemented it in a way that was suggested by @asanka.
Can you please take a look?
> > 
> > I am not sure how tests for this could be implemented for this - please
advise.
> 
> See //chrome/browser/download/download_browsertest.cc for some examples.
Specifically the DownloadTestWithShelf.AutoOpen test at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
. You could do something similar where you manually trigger a download to be
shown in the folder and then see if the download shelf is still open.
> 
> The tricky bit is to mock out the actual shell invocation so that the browser
test doesn't interact with the actual desktop. We don't want browser_tests to
interact with the desktop or open random application windows. So you'd need to
break up the Open/ShowInFolder logic in ChromeDownloadManagerDelegate does
everything except actually invoke the platform when showing a download in the
shell during the browser_test.
> 
> > The changes have not touched download history, download API and stats, so I
have questions about that:
> > 1. Should it record any stats event for 'show' as it does in RecordOpen?

Nope. We recorded stats for Open because we want the data to guide how we design
the download open behavior. "Show in folder" doesn't have any plans of factoring
into this decision.

> > 2. Should it be exposed in API (is there any need for that at all, as it
seems that having just 'opened' is probably fine).

Hmm. On the fence on this one. I'm reluctant to change the meaning of 'opened'
to mean 'acted upon' mostly because the assumptions extension writers have made
based on that flag are unknowable at this point. But I see your points that
wiring it through 'opened' is probably fine.

If I had to decide, I'd probably leave it alone.

> > 3. Should it be recorded in history?

No, for the same reason as 1 above. Recording 'open' was a means of getting at
the 'time to first open' statistic.

> > 
> > Thank you.

[benjhayden, 2016-06-07 23:27:55]

benjhayden@chromium.org changed reviewers:
- benjhayden@chromium.org

[asanka, 2016-11-01 17:41:05]

On 2015/12/02 at 15:59:38, asanka wrote:
> On 2015/12/01 at 15:55:54, asanka wrote:
> > On 2015/11/29 at 22:46:51, DukeXar wrote:
> > > Hello, I have updated this quite old code review.
> > 
> > Code LG so far.
> > 
> > > I think I have now implemented it in a way that was suggested by @asanka.
Can you please take a look?
> > > 
> > > I am not sure how tests for this could be implemented for this - please
advise.
> > 
> > See //chrome/browser/download/download_browsertest.cc for some examples.
Specifically the DownloadTestWithShelf.AutoOpen test at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
. You could do something similar where you manually trigger a download to be
shown in the folder and then see if the download shelf is still open.
> > 
> > The tricky bit is to mock out the actual shell invocation so that the
browser test doesn't interact with the actual desktop. We don't want
browser_tests to interact with the desktop or open random application windows.
So you'd need to break up the Open/ShowInFolder logic in
ChromeDownloadManagerDelegate does everything except actually invoke the
platform when showing a download in the shell during the browser_test.
> > 
> > > The changes have not touched download history, download API and stats, so
I have questions about that:
> > > 1. Should it record any stats event for 'show' as it does in RecordOpen?
> 
> Nope. We recorded stats for Open because we want the data to guide how we
design the download open behavior. "Show in folder" doesn't have any plans of
factoring into this decision.
> 
> > > 2. Should it be exposed in API (is there any need for that at all, as it
seems that having just 'opened' is probably fine).
> 
> Hmm. On the fence on this one. I'm reluctant to change the meaning of 'opened'
to mean 'acted upon' mostly because the assumptions extension writers have made
based on that flag are unknowable at this point. But I see your points that
wiring it through 'opened' is probably fine.
> 
> If I had to decide, I'd probably leave it alone.
> 
> > > 3. Should it be recorded in history?
> 
> No, for the same reason as 1 above. Recording 'open' was a means of getting at
the 'time to first open' statistic.
> 
> > > 
> > > Thank you.

Ping?

[DukeXar, 2016-11-02 00:15:38]

On 2016/11/01 17:41:05, asanka wrote:
> On 2015/12/02 at 15:59:38, asanka wrote:
> > On 2015/12/01 at 15:55:54, asanka wrote:
> > > On 2015/11/29 at 22:46:51, DukeXar wrote:
> > > > Hello, I have updated this quite old code review.
> > > 
> > > Code LG so far.
> > > 
> > > > I think I have now implemented it in a way that was suggested by
@asanka.
> Can you please take a look?
> > > > 
> > > > I am not sure how tests for this could be implemented for this - please
> advise.
> > > 
> > > See //chrome/browser/download/download_browsertest.cc for some examples.
> Specifically the DownloadTestWithShelf.AutoOpen test at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
> . You could do something similar where you manually trigger a download to be
> shown in the folder and then see if the download shelf is still open.
> > > 
> > > The tricky bit is to mock out the actual shell invocation so that the
> browser test doesn't interact with the actual desktop. We don't want
> browser_tests to interact with the desktop or open random application windows.
> So you'd need to break up the Open/ShowInFolder logic in
> ChromeDownloadManagerDelegate does everything except actually invoke the
> platform when showing a download in the shell during the browser_test.
> > > 
> > > > The changes have not touched download history, download API and stats,
so
> I have questions about that:
> > > > 1. Should it record any stats event for 'show' as it does in RecordOpen?
> > 
> > Nope. We recorded stats for Open because we want the data to guide how we
> design the download open behavior. "Show in folder" doesn't have any plans of
> factoring into this decision.
> > 
> > > > 2. Should it be exposed in API (is there any need for that at all, as it
> seems that having just 'opened' is probably fine).
> > 
> > Hmm. On the fence on this one. I'm reluctant to change the meaning of
'opened'
> to mean 'acted upon' mostly because the assumptions extension writers have
made
> based on that flag are unknowable at this point. But I see your points that
> wiring it through 'opened' is probably fine.
> > 
> > If I had to decide, I'd probably leave it alone.
> > 
> > > > 3. Should it be recorded in history?
> > 
> > No, for the same reason as 1 above. Recording 'open' was a means of getting
at
> the 'time to first open' statistic.
> > 
> > > > 
> > > > Thank you.
> 
> Ping?

Oh, I am sorry - I have completely lost track of this. Let me take a look at the
tests again.

[DukeXar, 2016-11-14 02:44:44]

On 2016/11/02 00:15:38, DukeXar wrote:
> On 2016/11/01 17:41:05, asanka wrote:
> > On 2015/12/02 at 15:59:38, asanka wrote:
> > > On 2015/12/01 at 15:55:54, asanka wrote:
> > > > On 2015/11/29 at 22:46:51, DukeXar wrote:
> > > > > Hello, I have updated this quite old code review.
> > > > 
> > > > Code LG so far.
> > > > 
> > > > > I think I have now implemented it in a way that was suggested by
> @asanka.
> > Can you please take a look?
> > > > > 
> > > > > I am not sure how tests for this could be implemented for this -
please
> > advise.
> > > > 
> > > > See //chrome/browser/download/download_browsertest.cc for some examples.
> > Specifically the DownloadTestWithShelf.AutoOpen test at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
> > . You could do something similar where you manually trigger a download to be
> > shown in the folder and then see if the download shelf is still open.
> > > > 
> > > > The tricky bit is to mock out the actual shell invocation so that the
> > browser test doesn't interact with the actual desktop. We don't want
> > browser_tests to interact with the desktop or open random application
windows.
> > So you'd need to break up the Open/ShowInFolder logic in
> > ChromeDownloadManagerDelegate does everything except actually invoke the
> > platform when showing a download in the shell during the browser_test.
> > > > 
> > > > > The changes have not touched download history, download API and stats,
> so
> > I have questions about that:
> > > > > 1. Should it record any stats event for 'show' as it does in
RecordOpen?
> > > 
> > > Nope. We recorded stats for Open because we want the data to guide how we
> > design the download open behavior. "Show in folder" doesn't have any plans
of
> > factoring into this decision.
> > > 
> > > > > 2. Should it be exposed in API (is there any need for that at all, as
it
> > seems that having just 'opened' is probably fine).
> > > 
> > > Hmm. On the fence on this one. I'm reluctant to change the meaning of
> 'opened'
> > to mean 'acted upon' mostly because the assumptions extension writers have
> made
> > based on that flag are unknowable at this point. But I see your points that
> > wiring it through 'opened' is probably fine.
> > > 
> > > If I had to decide, I'd probably leave it alone.
> > > 
> > > > > 3. Should it be recorded in history?
> > > 
> > > No, for the same reason as 1 above. Recording 'open' was a means of
getting
> at
> > the 'time to first open' statistic.
> > > 
> > > > > 
> > > > > Thank you.
> > 
> > Ping?
> 
> Oh, I am sorry - I have completely lost track of this. Let me take a look at
the
> tests again.

Uploaded a new changeset.

1. Rebased on master.
2. Changed order of when OnDownloadOpened and OnDownloadShown are called - now
action is taken first, then notification is called, otherwise it is impossible
to track any state.
3. Modified platform_util::ShowItemInFolder to allow disabling shell operations
using existing flag, and enabled it in browser tests.
4. Updated browser tests and unit tests.

What you think?

[DukeXar, 2016-11-23 17:26:47]

On 2016/11/14 02:44:44, DukeXar wrote:
> On 2016/11/02 00:15:38, DukeXar wrote:
> > On 2016/11/01 17:41:05, asanka wrote:
> > > On 2015/12/02 at 15:59:38, asanka wrote:
> > > > On 2015/12/01 at 15:55:54, asanka wrote:
> > > > > On 2015/11/29 at 22:46:51, DukeXar wrote:
> > > > > > Hello, I have updated this quite old code review.
> > > > > 
> > > > > Code LG so far.
> > > > > 
> > > > > > I think I have now implemented it in a way that was suggested by
> > @asanka.
> > > Can you please take a look?
> > > > > > 
> > > > > > I am not sure how tests for this could be implemented for this -
> please
> > > advise.
> > > > > 
> > > > > See //chrome/browser/download/download_browsertest.cc for some
examples.
> > > Specifically the DownloadTestWithShelf.AutoOpen test at
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
> > > . You could do something similar where you manually trigger a download to
be
> > > shown in the folder and then see if the download shelf is still open.
> > > > > 
> > > > > The tricky bit is to mock out the actual shell invocation so that the
> > > browser test doesn't interact with the actual desktop. We don't want
> > > browser_tests to interact with the desktop or open random application
> windows.
> > > So you'd need to break up the Open/ShowInFolder logic in
> > > ChromeDownloadManagerDelegate does everything except actually invoke the
> > > platform when showing a download in the shell during the browser_test.
> > > > > 
> > > > > > The changes have not touched download history, download API and
stats,
> > so
> > > I have questions about that:
> > > > > > 1. Should it record any stats event for 'show' as it does in
> RecordOpen?
> > > > 
> > > > Nope. We recorded stats for Open because we want the data to guide how
we
> > > design the download open behavior. "Show in folder" doesn't have any plans
> of
> > > factoring into this decision.
> > > > 
> > > > > > 2. Should it be exposed in API (is there any need for that at all,
as
> it
> > > seems that having just 'opened' is probably fine).
> > > > 
> > > > Hmm. On the fence on this one. I'm reluctant to change the meaning of
> > 'opened'
> > > to mean 'acted upon' mostly because the assumptions extension writers have
> > made
> > > based on that flag are unknowable at this point. But I see your points
that
> > > wiring it through 'opened' is probably fine.
> > > > 
> > > > If I had to decide, I'd probably leave it alone.
> > > > 
> > > > > > 3. Should it be recorded in history?
> > > > 
> > > > No, for the same reason as 1 above. Recording 'open' was a means of
> getting
> > at
> > > the 'time to first open' statistic.
> > > > 
> > > > > > 
> > > > > > Thank you.
> > > 
> > > Ping?
> > 
> > Oh, I am sorry - I have completely lost track of this. Let me take a look at
> the
> > tests again.
> 
> Uploaded a new changeset.
> 
> 1. Rebased on master.
> 2. Changed order of when OnDownloadOpened and OnDownloadShown are called - now
> action is taken first, then notification is called, otherwise it is impossible
> to track any state.
> 3. Modified platform_util::ShowItemInFolder to allow disabling shell
operations
> using existing flag, and enabled it in browser tests.
> 4. Updated browser tests and unit tests.
> 
> What you think?

PTAL when you have time.