mac: Stop using __weak.

mac: Stop using __weak.

__weak used to not have semantic meaning in non-GC non-ARC
builds like ours. clang r251041 changed things so that it now has
the meaning of "zeroing weak reference" even in non-ARC builds,
and these require 10.7+.  Since we still target 10.6, just replace the
couple of "__weak"s we have with comments.

For IBOutlets, use retain instead of assign outlets, and use
__unsafe_unretained in the one place where the outlet can't be retain
due to a reference cycle. (This has no semantic effect in non-ARC
builds, but will do the right thing if we ever turn on ARC. Since that
ref is explicitly zeroed in dealloc, it doesn't even seem much worse
than __weak.)

BUG=550066
Committed: https://crrev.com/bd7bc5a7d793e805db45ef7de827399a316116ee
Cr-Commit-Position: refs/heads/master@{#357835}

[Nico, 2015-11-03 02:39:58]

Description was changed from

==========
mac: Stop using __weak.

BUG=550066
==========

to

==========
mac: Stop using __weak.

__weak used to not have semantic meaning in non-GC non-ARC
builds like ours. clang r251041 changed things so that it now has
the meaning of "zeroing weak reference" even in non-ARC builds,
and these require 10.7+.  Since we still target 10.6, just replace the
couple of "__weak"s we have with comments.

For IBOutlets, use readonly instead of assign outlets where possible,
and use __unsafe_unretained in the one place where the outlet
can't be either readonly nor retain due to a reference cycle. (This
has no semantic effect in non-ARC builds, but will do the right thing
if we ever turn on ARC. Since that ref is explicitly zeroed in dealloc,
it doesn't even seem much worse than __weak.)

BUG=550066
==========

[Nico, 2015-11-03 02:40:44]

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

[Nico, 2015-11-03 02:40:44]

Patch set 1 has a few representative compile errors in #if 0 blocks, if you're
curious.

[Robert Sesek, 2015-11-03 15:41:11]

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h (right):

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h:82:
@property(readonly, nonatomic) IBOutlet NSImageView* iconView;
Why the change from assign -> readonly? That doesn't seem right given the
IBOutlet annotation.

[Nico, 2015-11-03 16:01:18]

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h (right):

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h:82:
@property(readonly, nonatomic) IBOutlet NSImageView* iconView;
On 2015/11/03 15:41:11, Robert Sesek wrote:
> Why the change from assign -> readonly? That doesn't seem right given the
> IBOutlet annotation.

It can't stay assign without the __weak. I could change it to retain, but since
they're never changed after being loaded from the nib this seemed like the
semantically better choice.

[Nico, 2015-11-03 21:18:53]

On 2015/11/03 16:01:18, Nico wrote:
>
https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h (right):
> 
>
https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h:82:
> @property(readonly, nonatomic) IBOutlet NSImageView* iconView;
> On 2015/11/03 15:41:11, Robert Sesek wrote:
> > Why the change from assign -> readonly? That doesn't seem right given the
> > IBOutlet annotation.
> 
> It can't stay assign without the __weak. I could change it to retain, but
since
> they're never changed after being loaded from the nib this seemed like the
> semantically better choice.

Hm, looks like the trybot isn't happy with this change. I'll change this to
retain insteda.

[Nico, 2015-11-03 21:21:40]

Done.

[Nico, 2015-11-03 21:48:37]

Description was changed from

==========
mac: Stop using __weak.

__weak used to not have semantic meaning in non-GC non-ARC
builds like ours. clang r251041 changed things so that it now has
the meaning of "zeroing weak reference" even in non-ARC builds,
and these require 10.7+.  Since we still target 10.6, just replace the
couple of "__weak"s we have with comments.

For IBOutlets, use readonly instead of assign outlets where possible,
and use __unsafe_unretained in the one place where the outlet
can't be either readonly nor retain due to a reference cycle. (This
has no semantic effect in non-ARC builds, but will do the right thing
if we ever turn on ARC. Since that ref is explicitly zeroed in dealloc,
it doesn't even seem much worse than __weak.)

BUG=550066
==========

to

==========
mac: Stop using __weak.

__weak used to not have semantic meaning in non-GC non-ARC
builds like ours. clang r251041 changed things so that it now has
the meaning of "zeroing weak reference" even in non-ARC builds,
and these require 10.7+.  Since we still target 10.6, just replace the
couple of "__weak"s we have with comments.

For IBOutlets, use retain instead of assign outlets,
and use __unsafe_unretained in the one place where the outlet
can't be retain due to a reference cycle. (This
has no semantic effect in non-ARC builds, but will do the right thing
if we ever turn on ARC. Since that ref is explicitly zeroed in dealloc,
it doesn't even seem much worse than __weak.)

BUG=550066
==========

[Nico, 2015-11-03 21:49:08]

Description was changed from

==========
mac: Stop using __weak.

__weak used to not have semantic meaning in non-GC non-ARC
builds like ours. clang r251041 changed things so that it now has
the meaning of "zeroing weak reference" even in non-ARC builds,
and these require 10.7+.  Since we still target 10.6, just replace the
couple of "__weak"s we have with comments.

For IBOutlets, use retain instead of assign outlets,
and use __unsafe_unretained in the one place where the outlet
can't be retain due to a reference cycle. (This
has no semantic effect in non-ARC builds, but will do the right thing
if we ever turn on ARC. Since that ref is explicitly zeroed in dealloc,
it doesn't even seem much worse than __weak.)

BUG=550066
==========

to

==========
mac: Stop using __weak.

__weak used to not have semantic meaning in non-GC non-ARC
builds like ours. clang r251041 changed things so that it now has
the meaning of "zeroing weak reference" even in non-ARC builds,
and these require 10.7+.  Since we still target 10.6, just replace the
couple of "__weak"s we have with comments.

For IBOutlets, use retain instead of assign outlets, and use
__unsafe_unretained in the one place where the outlet can't be retain
due to a reference cycle. (This has no semantic effect in non-ARC
builds, but will do the right thing if we ever turn on ARC. Since that
ref is explicitly zeroed in dealloc, it doesn't even seem much worse
than __weak.)

BUG=550066
==========

[Robert Sesek, 2015-11-03 22:03:51]

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h (right):

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h:82:
@property(readonly, nonatomic) IBOutlet NSImageView* iconView;
On 2015/11/03 16:01:18, Nico wrote:
> On 2015/11/03 15:41:11, Robert Sesek wrote:
> > Why the change from assign -> readonly? That doesn't seem right given the
> > IBOutlet annotation.
> 
> It can't stay assign without the __weak. I could change it to retain, but
since
> they're never changed after being loaded from the nib this seemed like the
> semantically better choice.

If they're now retained, won't they be leaked? (Maybe use ObjCPropertyReleaser
?)

[Nico, 2015-11-03 22:15:58]

Done, thanks.

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h (right):

https://codereview.chromium.org/1407103008/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h:82:
@property(readonly, nonatomic) IBOutlet NSImageView* iconView;
On 2015/11/03 22:03:51, Robert Sesek wrote:
> On 2015/11/03 16:01:18, Nico wrote:
> > On 2015/11/03 15:41:11, Robert Sesek wrote:
> > > Why the change from assign -> readonly? That doesn't seem right given the
> > > IBOutlet annotation.
> > 
> > It can't stay assign without the __weak. I could change it to retain, but
> since
> > they're never changed after being loaded from the nib this seemed like the
> > semantically better choice.
> 
> If they're now retained, won't they be leaked? (Maybe use ObjCPropertyReleaser
> ?)

Done.

[Nico, 2015-11-04 16:02:20]

Ping. This is now the only blocker for starting a clang roll attempt.

[Robert Sesek, 2015-11-04 16:14:26]

On 2015/11/04 16:02:20, Nico wrote:
> Ping. This is now the only blocker for starting a clang roll attempt.

LGTM. You had compile failures when I left last night.

[Nico, 2015-11-04 16:34:58]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-11-04 16:35:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407103008/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407103008/100001

[commit-bot: I haz the power, 2015-11-04 17:07:56]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-04 17:09:55]

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