[Mac] Re-layout download shelf when danger state of item changes.

[Mac] Re-layout download shelf when danger state of item changes.

Updating the width of the DownloadItemController view is not sufficient
since the sibling items also need to be moved around.

In addition, the width of DownloadItemController view should be set
based on its preferred size (already done by
DownloadShelfController:layoutItems) rather than relative to its current
frame. This is because the DownloadItemController may be in the middle
of an animation, and thus its frame may not reflect its desired size.

While we are at it, add some unit tests for DownloadItemController,
refactor the user experience event logic, and deal with the possibility
that the DownloadItem and the DownloadItemController may be destroyed at
different times.

BUG=409999
Committed: https://crrev.com/bc3dd93004f36e6ad0662d0ecf8819c55dddc479
Cr-Commit-Position: refs/heads/master@{#295401}

[asanka, 2014-09-02 20:18:44]

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

[Nico, 2014-09-03 15:19:04]

lgtm

(maybe a test would've been nice)

[asanka, 2014-09-04 00:44:08]

On 2014/09/03 15:19:04, Nico (hiding) wrote:
> lgtm
> 
> (maybe a test would've been nice)

Added tests.

[asanka, 2014-09-04 14:36:18]

On 2014/09/04 00:44:08, asanka wrote:
> On 2014/09/03 15:19:04, Nico (hiding) wrote:
> > lgtm
> > 
> > (maybe a test would've been nice)
> 
> Added tests.

Also meant PTAL :) and apologies for not adding tests earlier.

[asanka, 2014-09-11 19:00:25]

On 2014/09/04 14:36:18, asanka wrote:
> On 2014/09/04 00:44:08, asanka wrote:
> > On 2014/09/03 15:19:04, Nico (hiding) wrote:
> > > lgtm
> > > 
> > > (maybe a test would've been nice)
> > 
> > Added tests.
> 
> Also meant PTAL :) and apologies for not adding tests earlier.

thakis: ping? Could you take a quick peek at the tests?

[mattm, 2014-09-16 17:49:30]

On 2014/09/11 19:00:25, asanka wrote:
> On 2014/09/04 14:36:18, asanka wrote:
> > On 2014/09/04 00:44:08, asanka wrote:
> > > On 2014/09/03 15:19:04, Nico (hiding) wrote:
> > > > lgtm
> > > > 
> > > > (maybe a test would've been nice)
> > > 
> > > Added tests.
> > 
> > Also meant PTAL :) and apologies for not adding tests earlier.
> 
> thakis: ping? Could you take a quick peek at the tests?

multi-ping

[Nico, 2014-09-16 22:48:14]

Thanks for adding a test! Mostly lgtm, one comment about memory management
below.

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

https://codereview.chromium.org/533883002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:22:
@interface DownloadItemController (DownloadItemControllerTest) {
do you need {} here?

https://codereview.chromium.org/533883002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:54: return
[super initWithDownload:downloadItem shelf:shelf navigator:NULL];
the usual pattern is

  if ((self = [super initWithDownload:downloadItem shelf:shelf navigator:NULL]))
    initCallback_ = callback;

As is, you're accessing self (by writing to initCallback_) before calling [super
init]

https://codereview.chromium.org/533883002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:106:
initCallback:run_loop.QuitClosure()] retain];
This looks wrong: alloc returns a refcount of 1, then you retain it for a
refcount of 2, then you hand it to subview which internally also retains and
then only releases once. I think item will leak here. You probably want
s/retain/autorelease/, or use a scoped_nsobject

[asanka, 2014-09-18 01:48:53]

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

https://codereview.chromium.org/533883002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:22:
@interface DownloadItemController (DownloadItemControllerTest) {
On 2014/09/16 22:48:14, Nico (hiding) wrote:
> do you need {} here?

Removed.

https://codereview.chromium.org/533883002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:54: return
[super initWithDownload:downloadItem shelf:shelf navigator:NULL];
On 2014/09/16 22:48:14, Nico (hiding) wrote:
> the usual pattern is
> 
>   if ((self = [super initWithDownload:downloadItem shelf:shelf
navigator:NULL]))
>     initCallback_ = callback;
> 
> As is, you're accessing self (by writing to initCallback_) before calling
[super
> init]

Ah. Right. Fixed.

https://codereview.chromium.org/533883002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:106:
initCallback:run_loop.QuitClosure()] retain];
On 2014/09/16 22:48:14, Nico (hiding) wrote:
> This looks wrong: alloc returns a refcount of 1, then you retain it for a
> refcount of 2, then you hand it to subview which internally also retains and
> then only releases once. I think item will leak here. You probably want
> s/retain/autorelease/, or use a scoped_nsobject

D'oh! Done.

[Nico, 2014-09-18 01:50:47]

lgtm!

[asanka, 2014-09-18 02:01:50]

On 2014/09/18 01:50:47, Nico (hiding) wrote:
> lgtm!

Thanks!

[asanka, 2014-09-18 02:19:00]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-09-18 02:20:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/533883002/40001

[commit-bot: I haz the power, 2014-09-18 02:44:53]

Committed patchset #3 (id:40001) as ba2985021f3d309a302f87b564e7c34b371d7555

[commit-bot: I haz the power, 2014-09-18 02:45:28]

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