Dismiss context menu when download bar is closed.

Dismiss context menu when download bar is closed.

For some downloaded items like Chrome apps, Chromium hides the download
bar once the download completes if there are no other download items in
it. However, if you cause the download item to display its context menu,
the context menu will remain visible even after the bar disappears.

BUG=480884
Committed: https://crrev.com/c93e4d7ee750dace1cee0c90ae94bd713592d4c3
Cr-Commit-Position: refs/heads/master@{#330221}

[shrike, 2015-05-07 00:34:05]

shrike@chromium.org changed reviewers:
+ thakis@chromium.org

[shrike, 2015-05-07 00:35:52]

Hello thakis,

PTAL

[asanka, 2015-05-07 14:30:01]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2015-05-07 14:30:01]

Instead, shall we prevent the download shelf from closing automatically in this
case? The user is clearly still trying to interact with the download, and
actions like "Show in Finder" are still valid.

[shrike, 2015-05-07 16:55:36]

On 2015/05/07 14:30:01, asanka wrote:
> Instead, shall we prevent the download shelf from closing automatically in
this
> case? The user is clearly still trying to interact with the download, and
> actions like "Show in Finder" are still valid.

That may be a better approach - let me investigate the implementation....

[shrike, 2015-05-07 18:40:04]

On 2015/05/07 16:55:36, shrike wrote:
> On 2015/05/07 14:30:01, asanka wrote:
> > Instead, shall we prevent the download shelf from closing automatically in
> this
> > case? The user is clearly still trying to interact with the download, and
> > actions like "Show in Finder" are still valid.
> 
> That may be a better approach - let me investigate the implementation....

OK, this is a webstore download, and the webstore installer automatically
installs the app and deletes the downloaded file. I should be able to prevent
the download item from disappearing from the shelf (and thereby prevent the
shelf from disappearing), but the downloaded file it represents will no longer
exist, so Show in Finder, etc. will no longer be valid. I think force-dismissing
the context menu is the right solution.

[Nico, 2015-05-07 23:20:59]

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

[Nico, 2015-05-07 23:20:59]

(removing myself as asanka already took a look and is owner here too)

[asanka, 2015-05-08 17:11:00]

On 2015/05/07 at 18:40:04, shrike wrote:
> On 2015/05/07 16:55:36, shrike wrote:
> > On 2015/05/07 14:30:01, asanka wrote:
> > > Instead, shall we prevent the download shelf from closing automatically in
> > this
> > > case? The user is clearly still trying to interact with the download, and
> > > actions like "Show in Finder" are still valid.
> > 
> > That may be a better approach - let me investigate the implementation....
> 
> OK, this is a webstore download, and the webstore installer automatically
installs the app and deletes the downloaded file. I should be able to prevent
the download item from disappearing from the shelf (and thereby prevent the
shelf from disappearing), but the downloaded file it represents will no longer
exist, so Show in Finder, etc. will no longer be valid. I think force-dismissing
the context menu is the right solution.

So there are two situations to worry about:

1. A download being programmatically removed (which is what happens with the App
store, and can happen elsewhere as well).

   The code path for this is DownloadItemMac::OnDownloadDestroyed() ->
DownloadItemController remove: ->.. DownloadShelfController removeDownload:

   Since the DownloadItem itself is being removed, it makes sense to dismiss the
context menu. It can be a bit jarring if the user was trying to interact with
the menu, specially if this ends up mistargetting a click. But I'm not convinced
that the amount of work required to fix it is justified. So let's move ahead
with closing the menu in this case.

2. A download being marked as opened, and hence triggering the shelf autoclose
logic. Only a problem for downloads that are marked for opening automatically
after completion.

   It looks like the DownloadShelfController maybeAutoCloseAfterDelay: doesn't
take into consideration that there might be a context menu open. We'd need to
make it consider the showingContextMenu: value. In addition, once a menu is
closed we'd need to check again with the shelf whether the shelf can proceed
with the auto close that was suppressed previously due to the menu being open.
This can duplicate some work if the menu action invoked was "Open".

[asanka, 2015-05-08 17:12:47]

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_button.mm (right):

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_button.mm:97: -
(void)viewWillMoveToWindow:(NSWindow *)newWindow {
Shall we trigger the menu close logic on the download removal pathway described
above?

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_button.mm:100: // pop up menu).
What about NSMenu cancelTracking: ?

[shrike, 2015-05-08 17:52:04]

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_button.mm (right):

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_button.mm:97: -
(void)viewWillMoveToWindow:(NSWindow *)newWindow {
On 2015/05/08 17:12:47, asanka wrote:
> Shall we trigger the menu close logic on the download removal pathway
described
> above?

By "described above" do you mean your previous comment about watching for
download items marked as open, or somewhere "above" in the code? I think it
makes sense to incorporate the marked as open case - can you suggest a download
item, or class of items, that would produce this behavior (so that I can test)?

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_button.mm:100: // pop up menu).
On 2015/05/08 17:12:47, asanka wrote:
> What about NSMenu cancelTracking: ?

Indeed, but the problem is that popUpContextMenu: does not return the popup menu
(i.e. there is no way to get the popup menu in order to call cancelTracking: on
it).

[shrike, 2015-05-12 00:15:39]

On 2015/05/08 17:11:00, asanka wrote:
> 2. A download being marked as opened, and hence triggering the shelf autoclose
> logic. Only a problem for downloads that are marked for opening automatically
> after completion.
> 
>    It looks like the DownloadShelfController maybeAutoCloseAfterDelay: doesn't
> take into consideration that there might be a context menu open. We'd need to
> make it consider the showingContextMenu: value. In addition, once a menu is
> closed we'd need to check again with the shelf whether the shelf can proceed
> with the auto close that was suppressed previously due to the menu being open.
> This can duplicate some work if the menu action invoked was "Open".

Hello asanka,

Digging into the code a bit, if I change Chrome to not auto-delete an
auto-opening DownloadItem that's showing its context menu, the act of
auto-opening necessarily makes the auto-opening app the active app, which causes
the DownloadItem's context menu to disappear. You mentioned delaying the
removal/shelf close until the menu goes away, but obviously we don't really want
to do that. So it seems like there are two options:

When an auto-opening item with a visible context menu finishes downloading and
opens...

1. ...Chrome dismisses the context menu and allows the item to remove itself
from the download shelf, possibly hiding the shelf as a result

or

2. ...Chrome prevents the item from removing itself from the download shelf,
leaving the item in the shelf and the shelf visible indefinitely; and as a
result of auto-opening the item, the context menu is no longer visible.

Option 2 seems like it makes more sense (you get a chance to come back to the
item and interact with it at a time of your choosing in the future), but it also
means the shelf won't clean itself up after the download. What are your
thoughts?

[asanka, 2015-05-12 01:27:20]

On 2015/05/12 at 00:15:39, shrike wrote:
> On 2015/05/08 17:11:00, asanka wrote:
> > 2. A download being marked as opened, and hence triggering the shelf
autoclose
> > logic. Only a problem for downloads that are marked for opening
automatically
> > after completion.
> > 
> >    It looks like the DownloadShelfController maybeAutoCloseAfterDelay:
doesn't
> > take into consideration that there might be a context menu open. We'd need
to
> > make it consider the showingContextMenu: value. In addition, once a menu is
> > closed we'd need to check again with the shelf whether the shelf can proceed
> > with the auto close that was suppressed previously due to the menu being
open.
> > This can duplicate some work if the menu action invoked was "Open".
> 
> Hello asanka,
> 
> Digging into the code a bit, if I change Chrome to not auto-delete an
auto-opening DownloadItem that's showing its context menu, the act of
auto-opening necessarily makes the auto-opening app the active app, which causes
the DownloadItem's context menu to disappear. You mentioned delaying the
removal/shelf close until the menu goes away, but obviously we don't really want
to do that. So it seems like there are two options:
> 
> When an auto-opening item with a visible context menu finishes downloading and
opens...
> 
> 1. ...Chrome dismisses the context menu and allows the item to remove itself
from the download shelf, possibly hiding the shelf as a result
> 
> or
> 
> 2. ...Chrome prevents the item from removing itself from the download shelf,
leaving the item in the shelf and the shelf visible indefinitely; and as a
result of auto-opening the item, the context menu is no longer visible.
> 
> Option 2 seems like it makes more sense (you get a chance to come back to the
item and interact with it at a time of your choosing in the future), but it also
means the shelf won't clean itself up after the download. What are your
thoughts?

Chrome doesn't remove the download item after an auto-open. However, after a
successful open, Chrome checks if all the downloads on the shelf have been
opened, and if so, proceeds to close the shelf.

Good point that the context menu will likely go away as a result of the
associated application opening.

With that in mind, dismissing the context menu seems desirable for both the
auto-open and remove cases. Keeping the shelf open if the user had a menu open
during an auto-open even seems desirable. But I feel the added code complexity
along with the lack of cleanup and the fact that we can't avoid the sudden
disappearance of the context menu all tips the scales against it.

[asanka, 2015-05-12 01:27:34]

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_button.mm (right):

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_button.mm:97: -
(void)viewWillMoveToWindow:(NSWindow *)newWindow {
On 2015/05/08 at 17:52:04, shrike wrote:
> On 2015/05/08 17:12:47, asanka wrote:
> > Shall we trigger the menu close logic on the download removal pathway
described
> > above?
> 
> By "described above" do you mean your previous comment about watching for
download items marked as open, or somewhere "above" in the code? I think it
makes sense to incorporate the marked as open case - can you suggest a download
item, or class of items, that would produce this behavior (so that I can test)?

Sorry, I was referring to https://codereview.chromium.org/1125423002#msg9 .

https://codereview.chromium.org/1125423002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_button.mm:100: // pop up menu).
On 2015/05/08 at 17:52:04, shrike wrote:
> On 2015/05/08 17:12:47, asanka wrote:
> > What about NSMenu cancelTracking: ?
> 
> Indeed, but the problem is that popUpContextMenu: does not return the popup
menu (i.e. there is no way to get the popup menu in order to call
cancelTracking: on it).

Can we keep a reference to the DownloadShelfContextMenuController or the NSMenu
that we are passing into popUpContextMenu?

[shrike, 2015-05-12 22:23:24]

Hello asanka,

OK, this cl adheres to the original design (the context menu is always dismissed
for auto-open/install items, and the shelf will hide if possible). I have
changed the download item button to keep a pointer to the context menu so that
it can dismiss it using standard API (sorry I didn't think about us being the
ones supplying the menu to popUpContextMenu:... in the first place).

PTAL

[asanka, 2015-05-13 16:58:55]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
Let's avoid sleeping in tests.

Can we schedule a task (preferably via PostTask) to run [item remove] or
somesuch once the nested run loop starts? Granted it will run pretty early on in
the life of the popup context menu.

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:210:
waitUntilDone:NO];
The real trigger for the removal of the context menu is the removal of the
download. Can you test with [item remove] ?

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:213:
EXPECT_FALSE([downloadItemButton showingContextMenu]);
Given that this line won't be reached until the context menu is dismissed, isn't
this a no-op?

[shrike, 2015-05-13 23:25:52]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
On 2015/05/13 16:58:55, asanka wrote:
> Let's avoid sleeping in tests.
> 
> Can we schedule a task (preferably via PostTask) to run [item remove] or
> somesuch once the nested run loop starts? Granted it will run pretty early on
in
> the life of the popup context menu.

I'm using an NSOperationQueue to execute the code (via a block) while the menu
is popped up. The 0.25 delay gives the menu some time to pop up before the block
executes (actually, not a surefire strategy), but what I didn't realize is that
operations scheduled on the mainQueue are executed on the main thread, and on
passes through the run loop. So, I don't actually have to add any kind of delay:
the block gets added to the queue, but only runs after the menu pops up.

Re: using PostTask(), I tried that but the posted task seemed not to run until
the menu was dismissed? Does it make sense that
base::MessageLoop::current()->PostTask() would block while the modal loop runs?

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:210:
waitUntilDone:NO];
On 2015/05/13 16:58:55, asanka wrote:
> The real trigger for the removal of the context menu is the removal of the
> download. Can you test with [item remove] ?

I agree that makes sense. However, it appears that the shelf is not actually a
DownloadShelfController but is instead an OCMockObject instance, so calling
[item remove] or [(id)shelf remove:item] doesn't actually do anything?

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:213:
EXPECT_FALSE([downloadItemButton showingContextMenu]);
On 2015/05/13 16:58:55, asanka wrote:
> Given that this line won't be reached until the context menu is dismissed,
isn't
> this a no-op?

Acknowledged.

[asanka, 2015-05-14 03:31:45]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
On 2015/05/13 at 23:25:51, shrike wrote:
> On 2015/05/13 16:58:55, asanka wrote:
> > Let's avoid sleeping in tests.
> > 
> > Can we schedule a task (preferably via PostTask) to run [item remove] or
> > somesuch once the nested run loop starts? Granted it will run pretty early
on in
> > the life of the popup context menu.
> 
> I'm using an NSOperationQueue to execute the code (via a block) while the menu
is popped up. The 0.25 delay gives the menu some time to pop up before the block
executes (actually, not a surefire strategy), but what I didn't realize is that
operations scheduled on the mainQueue are executed on the main thread, and on
passes through the run loop. So, I don't actually have to add any kind of delay:
the block gets added to the queue, but only runs after the menu pops up.
> 
> Re: using PostTask(), I tried that but the posted task seemed not to run until
the menu was dismissed? Does it make sense that
base::MessageLoop::current()->PostTask() would block while the modal loop runs?

Hm. Tasks posted via PostTask() for the UI message loop should run even in an
event tracking run loop.

[shrike, 2015-05-15 01:07:04]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
On 2015/05/14 03:31:45, asanka wrote:
> On 2015/05/13 at 23:25:51, shrike wrote:
> > On 2015/05/13 16:58:55, asanka wrote:
> > > Let's avoid sleeping in tests.
> > > 
> > > Can we schedule a task (preferably via PostTask) to run [item remove] or
> > > somesuch once the nested run loop starts? Granted it will run pretty early
> on in
> > > the life of the popup context menu.
> > 
> > I'm using an NSOperationQueue to execute the code (via a block) while the
menu
> is popped up. The 0.25 delay gives the menu some time to pop up before the
block
> executes (actually, not a surefire strategy), but what I didn't realize is
that
> operations scheduled on the mainQueue are executed on the main thread, and on
> passes through the run loop. So, I don't actually have to add any kind of
delay:
> the block gets added to the queue, but only runs after the menu pops up.
> > 
> > Re: using PostTask(), I tried that but the posted task seemed not to run
until
> the menu was dismissed? Does it make sense that
> base::MessageLoop::current()->PostTask() would block while the modal loop
runs?
> 
> Hm. Tasks posted via PostTask() for the UI message loop should run even in an
> event tracking run loop.

Originally you didn't like the [NSTask sleep] call (agreed), but I don't
actually need to do that. Are you wanting me to reimplement using PostTask() or
is the current solution OK?

[asanka, 2015-05-15 14:56:11]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
On 2015/05/15 at 01:07:03, shrike wrote:
> On 2015/05/14 03:31:45, asanka wrote:
> > On 2015/05/13 at 23:25:51, shrike wrote:
> > > On 2015/05/13 16:58:55, asanka wrote:
> > > > Let's avoid sleeping in tests.
> > > > 
> > > > Can we schedule a task (preferably via PostTask) to run [item remove] or
> > > > somesuch once the nested run loop starts? Granted it will run pretty
early
> > on in
> > > > the life of the popup context menu.
> > > 
> > > I'm using an NSOperationQueue to execute the code (via a block) while the
menu
> > is popped up. The 0.25 delay gives the menu some time to pop up before the
block
> > executes (actually, not a surefire strategy), but what I didn't realize is
that
> > operations scheduled on the mainQueue are executed on the main thread, and
on
> > passes through the run loop. So, I don't actually have to add any kind of
delay:
> > the block gets added to the queue, but only runs after the menu pops up.
> > > 
> > > Re: using PostTask(), I tried that but the posted task seemed not to run
until
> > the menu was dismissed? Does it make sense that
> > base::MessageLoop::current()->PostTask() would block while the modal loop
runs?
> > 
> > Hm. Tasks posted via PostTask() for the UI message loop should run even in
an
> > event tracking run loop.
> 
> Originally you didn't like the [NSTask sleep] call (agreed), but I don't
actually need to do that. Are you wanting me to reimplement using PostTask() or
is the current solution OK?


Is there a new patchset?

I'd prefer a PostTask and I thought you were looking into understanding why it
wasn't working. But I'm okay with using NSOperationQueue.

[shrike, 2015-05-15 15:55:51]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
On 2015/05/15 14:56:11, asanka wrote:
> Is there a new patchset?

I think I was waiting on further direction before uploading.

> I'd prefer a PostTask and I thought you were looking into understanding why it
> wasn't working. But I'm okay with using NSOperationQueue.

Alright, I will dig into the PostTask issue. It might be pilot error, but I also
wonder if the unit_test environment differs from the regular browser.

[shrike, 2015-05-15 18:51:49]

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
(right):

https://codereview.chromium.org/1125423002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:205:
[NSThread sleepForTimeInterval:0.25];
On 2015/05/15 14:56:11, asanka wrote:
> On 2015/05/15 at 01:07:03, shrike wrote:
> > On 2015/05/14 03:31:45, asanka wrote:
> > > On 2015/05/13 at 23:25:51, shrike wrote:
> > > > On 2015/05/13 16:58:55, asanka wrote:
> > > > > Let's avoid sleeping in tests.
> > > > > 
> > > > > Can we schedule a task (preferably via PostTask) to run [item remove]
or
> > > > > somesuch once the nested run loop starts? Granted it will run pretty
> early
> > > on in
> > > > > the life of the popup context menu.
> > > > 
> > > > I'm using an NSOperationQueue to execute the code (via a block) while
the
> menu
> > > is popped up. The 0.25 delay gives the menu some time to pop up before the
> block
> > > executes (actually, not a surefire strategy), but what I didn't realize is
> that
> > > operations scheduled on the mainQueue are executed on the main thread, and
> on
> > > passes through the run loop. So, I don't actually have to add any kind of
> delay:
> > > the block gets added to the queue, but only runs after the menu pops up.
> > > > 
> > > > Re: using PostTask(), I tried that but the posted task seemed not to run
> until
> > > the menu was dismissed? Does it make sense that
> > > base::MessageLoop::current()->PostTask() would block while the modal loop
> runs?
> > > 
> > > Hm. Tasks posted via PostTask() for the UI message loop should run even in
> an
> > > event tracking run loop.
> > 
> > Originally you didn't like the [NSTask sleep] call (agreed), but I don't
> actually need to do that. Are you wanting me to reimplement using PostTask()
or
> is the current solution OK?
> 
> 
> Is there a new patchset?
> 
> I'd prefer a PostTask and I thought you were looking into understanding why it
> wasn't working. But I'm okay with using NSOperationQueue.

It looks like the message loop isn't running in the unit test (now that I think
about it, this would have to be a browser test to expect there to be an active
message loop?):

   
../../chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:202:
Failure
    Value of: base::MessageLoop::current()->is_running()
      Actual: false
    Expected: true

[shrike, 2015-05-15 21:00:14]

Here's the reworked unit test, without the call to sleep.

PTAL

[asanka, 2015-05-15 21:17:37]

lgtm

[shrike, 2015-05-15 21:19:05]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2015-05-15 21:20:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125423002/60001

[commit-bot: I haz the power, 2015-05-15 22:45:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-18 11:28:40]

Patchset 4 (id:??) landed as
https://crrev.com/c93e4d7ee750dace1cee0c90ae94bd713592d4c3
Cr-Commit-Position: refs/heads/master@{#330221}