[Mac] DownloadShelf should be notified when autoclosing.

[Mac] DownloadShelf should be notified when autoclosing.

If the download shelf is already hidden, then the autoclosing
logic on Mac wouldn't notify the DownloadShelf. If the shelf was
hidden temporarily due to the browser entering fullscreen mode,
then the shelf would be restored on exiting fullscreen mode.

Also fix UMA logging of download shelf closures on Mac and
assorted cleanup.

XIB changes:
* Connect close button to |-handleClose:| instead of |-hide:|.

BUG=222360
TEST=(1) Download something that is marked for opening
         automatically (e.g. a browser extension).
     (2) Enter fullscreen mode before download completes.
     (3) Wait for download to complete.
     (4) Exit fullscreen mode. The download shelf should be hidden.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191618

[asanka, 2013-03-22 20:11:00]

thakis: Could you take a look?

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (left):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:155: -
(void)viewFrameDidChange:(NSNotification*)notification {
Methods were re-ordered to match declaration order.

[asanka, 2013-03-22 20:19:19]

On 2013/03/22 20:11:00, asanka wrote:
> thakis: Could you take a look?

.. at chrome/app and chrome/browser/ui/cocoa/.

[Nico, 2013-03-22 20:24:11]

I looked at everything except download_shelf_controller.mm. All the files I
looked at look good.

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/download/do...
File chrome/browser/download/download_shelf.h (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/download/do...
chrome/browser/download/download_shelf.h:23: AUTOMATIC,
Maybe you can add a comment that explains when AUTOMATIC is used.

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (left):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:155: -
(void)viewFrameDidChange:(NSNotification*)notification {
On 2013/03/22 20:11:01, asanka wrote:
> Methods were re-ordered to match declaration order.

Can you land that part as a separate CL? That would make review much easier.

(Also, I think chromium-dev consensus on this topic is that the damage done to
`git blame` output is not worth the gain in orderliness, so maybe just revert
the shuffling altogether. But if you really want to do it, please do it in a
different CL and then rebase this CL on top of the shuffling. I'll stamp the
shuffle CL within minutes, so this shouldn't slow things down much.)

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_mac.mm (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_mac.mm:39: isUserAction:(reason
== USER_ACTION)];
nit: ne need for the parens

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/gtk/down...
File chrome/browser/ui/gtk/download/download_shelf_gtk.cc (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/gtk/down...
chrome/browser/ui/gtk/download/download_shelf_gtk.cc:213:
download_items_.size(), num_in_progress, (reason == AUTOMATIC));
parens also not needed

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_shelf_view.cc:410: (reason ==
AUTOMATIC));
ditto

[asanka, 2013-03-22 21:22:13]

Thanks for the quick review!

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/download/do...
File chrome/browser/download/download_shelf.h (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/download/do...
chrome/browser/download/download_shelf.h:23: AUTOMATIC,
On 2013/03/22 20:24:11, Nico wrote:
> Maybe you can add a comment that explains when AUTOMATIC is used.

Done.

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (left):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:155: -
(void)viewFrameDidChange:(NSNotification*)notification {
On 2013/03/22 20:24:11, Nico wrote:
> On 2013/03/22 20:11:01, asanka wrote:
> > Methods were re-ordered to match declaration order.
> 
> Can you land that part as a separate CL? That would make review much easier.
> 
> (Also, I think chromium-dev consensus on this topic is that the damage done to
> `git blame` output is not worth the gain in orderliness, so maybe just revert
> the shuffling altogether. But if you really want to do it, please do it in a
> different CL and then rebase this CL on top of the shuffling. I'll stamp the
> shuffle CL within minutes, so this shouldn't slow things down much.)

I undid the re-ordering. I see the point about messing up `git blame`, so I'm
inclined to leave the ordering as-is for now.

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_mac.mm (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_mac.mm:39: isUserAction:(reason
== USER_ACTION)];
On 2013/03/22 20:24:11, Nico wrote:
> nit: ne need for the parens

Done.

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/gtk/down...
File chrome/browser/ui/gtk/download/download_shelf_gtk.cc (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/gtk/down...
chrome/browser/ui/gtk/download/download_shelf_gtk.cc:213:
download_items_.size(), num_in_progress, (reason == AUTOMATIC));
On 2013/03/22 20:24:11, Nico wrote:
> parens also not needed

Done.

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

https://codereview.chromium.org/12995025/diff/3019/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_shelf_view.cc:410: (reason ==
AUTOMATIC));
On 2013/03/22 20:24:11, Nico wrote:
> ditto

Done.

[Nico, 2013-03-22 21:29:17]

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:194: [self
maybeAutoCloseAfterDelay];
maybeACAD installs a tracking area to check if the user mouses out of the
download shelf, but for remove: calls it's not a given that the cursor is in the
download shelf, right?

[asanka, 2013-03-22 21:33:23]

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:194: [self
maybeAutoCloseAfterDelay];
On 2013/03/22 21:29:17, Nico wrote:
> maybeACAD installs a tracking area to check if the user mouses out of the
> download shelf, but for remove: calls it's not a given that the cursor is in
the
> download shelf, right?

It's not a given. This is true for all calls of maybeAutoCloseAfterDelay since
both download opening and removal can happen without the user interacting with
the shelf.

[Nico, 2013-03-22 21:42:47]

lgtm

Since I worked with tracking areas a bit recently, a few comments on the
existing patch. Feel free to either ignore these completely, or address them in
a separate patch.

In addition to the below comments:

* Does the tracking area get removed from the view anywhere if the autoclose is
_not_ cancelled (but happens)?

* We have CrTrackingArea, which fixes some subtle issues with NSTrackingArea.
You might want to use this instead (it's as easy as replacing NS with Cr I
think).

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:391: // shelf
after the user moves the mouse out of the download shelf.
Might want to mention that this is somewhat optimistically done as the cursor
isn't necessarily over the shelf. (Should we detect that and not install a
tracking area in that case?)

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:415:
NSTrackingActiveAlways
BTW: Do you to pass NSTrackingInVisibleRect here too, to make sure the tracking
area tracks the size of the shelf view?

[asanka, 2013-03-29 15:56:20]

On 2013/03/22 21:42:47, Nico wrote:
> lgtm
> 
> Since I worked with tracking areas a bit recently, a few comments on the
> existing patch. Feel free to either ignore these completely, or address them
in
> a separate patch.

Thanks! The use of CrTrackingArea is addressed in the follow up CL
https://codereview.chromium.org/13318002/

> In addition to the below comments:
> 
> * Does the tracking area get removed from the view anywhere if the autoclose
is
> _not_ cancelled (but happens)?

If the shelf is closed via some other mechanism (i.e. full screen switch or
window closed), then call would go through showDownloadShelf: or exiting:, both
remove the tracking area.

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:391: // shelf
after the user moves the mouse out of the download shelf.
On 2013/03/22 21:42:47, Nico wrote:
> Might want to mention that this is somewhat optimistically done as the cursor
> isn't necessarily over the shelf. (Should we detect that and not install a
> tracking area in that case?)

I added a comment describing the current behavior.

Detecting the case and closing immediately is done in
https://codereview.chromium.org/13318002/

https://codereview.chromium.org/12995025/diff/4019/chrome/browser/ui/cocoa/do...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:415:
NSTrackingActiveAlways
On 2013/03/22 21:42:47, Nico wrote:
> BTW: Do you to pass NSTrackingInVisibleRect here too, to make sure the
tracking
> area tracks the size of the shelf view?

Done.

[commit-bot: I haz the power, 2013-04-01 14:53:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/12995025/17001

[commit-bot: I haz the power, 2013-04-01 17:29:19]

Change committed as 191618