LabelObserver is no longer retained by the label

LabelObserver is no longer retained by the label

Having the LabelObserver being retained by the label as associated object
creates a crash when compiling with ARC.
This CL prepares the LabelObserver to be converted to ARC by changing the
associated link from strong to link. The LabelObserver now needs to be retained
by an external object, and the observing needs to be started/stopped.

BUG=713169
Review-Url: https://codereview.chromium.org/2828743002
Cr-Commit-Position: refs/heads/master@{#466283}
Committed: https://chromium.googlesource.com/chromium/src/+/3a05731eee64a75591868d5dfb4baf4bf42bd3a1

[gambard, 2017-04-19 14:27:30]

gambard@chromium.org changed reviewers:
+ marq@chromium.org, stkhapugin@chromium.org

[gambard, 2017-04-19 14:27:30]

PTAL.

[marq (ping after 24h), 2017-04-19 15:25:20]

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm:239: [self.TOSObserver
startObserving];
What do these observers do? This CL doesn't add any actions to them.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.h (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.h:21: +
(instancetype)observerForLabel:(UILabel*)label;
Is this method still needed? It seems like any caller would also have access to
a direct pointer to the observer already, so there should just be an
-initWithLabel: instead.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:41: // The number of time this
observer has been asked to observe the label. When
s/time/times/

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:42: // reaching zero, the label
stop being observed.
s/stop/stops/

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:43: @property(nonatomic, assign)
NSInteger observingCount;
What's the use case for tracking multiple start/stop calls? Each observer
observes a single label, so why can't  the second and subsequent -startObserving
calls be no-ops, and the first -stopObserving call end all observation and every
subsequent one be a no-op?

[gambard, 2017-04-19 15:48:18]

Thanks, PTAL.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm:239: [self.TOSObserver
startObserving];
On 2017/04/19 15:25:19, marq wrote:
> What do these observers do? This CL doesn't add any actions to them.

The actions are added in CRUILabel+AttributeUtils.h, added here with
_TOSLabel.cr_lineHeight =...
I guess I should add a comment.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.h (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.h:21: +
(instancetype)observerForLabel:(UILabel*)label;
On 2017/04/19 15:25:19, marq wrote:
> Is this method still needed? It seems like any caller would also have access
to
> a direct pointer to the observer already, so there should just be an
> -initWithLabel: instead.

No, there is one use case in CRUILabel+AttributeUtils.h

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:41: // The number of time this
observer has been asked to observe the label. When
On 2017/04/19 15:25:19, marq wrote:
> s/time/times/

Done.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:42: // reaching zero, the label
stop being observed.
On 2017/04/19 15:25:19, marq wrote:
> s/stop/stops/

Done.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:43: @property(nonatomic, assign)
NSInteger observingCount;
On 2017/04/19 15:25:19, marq wrote:
> What's the use case for tracking multiple start/stop calls? Each observer
> observes a single label, so why can't  the second and subsequent
-startObserving
> calls be no-ops, and the first -stopObserving call end all observation and
every
> subsequent one be a no-op?

Each observer observes a single label but a controller does not know if the
label is already being observed.
For example on the first run view, the label is observed by the
welcome_to_chrome_view, and by the label link controller. And if another
controller stop observing your label, you might not want to stop observing it.

[stkhapugin, 2017-04-19 16:39:40]

Thank you very much for jumping in to do this, especially given that it's just a
side-effect of ARC work.

https://codereview.chromium.org/2828743002/diff/40001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/40001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:115: if (self.observingCount++ ==
0) {
Please don't put two operations - increment and comparison - in one line. 

Time taken to read this:

if (self.observingCount == 0){
  self.observingCount ++;
  [self registerAsObserver];
}

is about 10 times less than decrypting self.observingCount++ == 0, and I'm not
even sure I've done it correctly.

[pkl (ping after 24h if needed), 2017-04-19 16:48:02]

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

[pkl (ping after 24h if needed), 2017-04-19 16:48:03]

https://codereview.chromium.org/2828743002/diff/40001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/40001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:115: if (self.observingCount++ ==
0) {
On 2017/04/19 16:39:40, stkhapugin wrote:
> Please don't put two operations - increment and comparison - in one line. 
> 
> Time taken to read this:
> 
> if (self.observingCount == 0){
>   self.observingCount ++;
>   [self registerAsObserver];
> }
> 
> is about 10 times less than decrypting self.observingCount++ == 0, and I'm not
> even sure I've done it correctly. 

Readability aside, do the two implementations do the same thing?

[gambard, 2017-04-20 08:30:19]

Thanks, PTAL.

https://codereview.chromium.org/2828743002/diff/40001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/40001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:115: if (self.observingCount++ ==
0) {
On 2017/04/19 16:48:03, pklpkl wrote:
> On 2017/04/19 16:39:40, stkhapugin wrote:
> > Please don't put two operations - increment and comparison - in one line. 
> > 
> > Time taken to read this:
> > 
> > if (self.observingCount == 0){
> >   self.observingCount ++;
> >   [self registerAsObserver];
> > }
> > 
> > is about 10 times less than decrypting self.observingCount++ == 0, and I'm
not
> > even sure I've done it correctly. 
> 
> Readability aside, do the two implementations do the same thing?

No, the increment should be outside the if.
Done.

[gambard, 2017-04-20 09:20:02]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 09:20:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marq (ping after 24h), 2017-04-20 11:52:01]

LGTM, but please add comments to the welcome view to clarify why the observers
are being retained.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm:239: [self.TOSObserver
startObserving];
On 2017/04/19 15:48:18, gambard wrote:
> On 2017/04/19 15:25:19, marq wrote:
> > What do these observers do? This CL doesn't add any actions to them.
> 
> The actions are added in CRUILabel+AttributeUtils.h, added here with
> _TOSLabel.cr_lineHeight =...
> I guess I should add a comment.

Yeah, that's non-obvious and kind of clunky.

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:43: @property(nonatomic, assign)
NSInteger observingCount;
On 2017/04/19 15:48:18, gambard wrote:
> On 2017/04/19 15:25:19, marq wrote:
> > What's the use case for tracking multiple start/stop calls? Each observer
> > observes a single label, so why can't  the second and subsequent
> -startObserving
> > calls be no-ops, and the first -stopObserving call end all observation and
> every
> > subsequent one be a no-op?
> 
> Each observer observes a single label but a controller does not know if the
> label is already being observed.
> For example on the first run view, the label is observed by the
> welcome_to_chrome_view, and by the label link controller. And if another
> controller stop observing your label, you might not want to stop observing it.

In practice, this is just so the line-height setting observer(s) used in the
welcome view can be shared with the link controller(s) used on the same labels.
I thought it would be better to just rip out the line-setting class extension
and use a simple helper function to handle this in the welcome view, but doing
that broke the link handling in annoying ways. So I'm OK with this approach
here, but the whole thing is a mess overall.

[stkhapugin, 2017-04-20 15:51:34]

lgtm with comment

https://codereview.chromium.org/2828743002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/util/label_observer.mm:130: if (self.observingCount-- ==
1) {
Please rewrite the condition similar to line 120.

[gambard, 2017-04-21 07:20:40]

Thanks!

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/20001/ios/chrome/browser/ui/u...
ios/chrome/browser/ui/util/label_observer.mm:43: @property(nonatomic, assign)
NSInteger observingCount;
On 2017/04/20 11:52:00, marq wrote:
> On 2017/04/19 15:48:18, gambard wrote:
> > On 2017/04/19 15:25:19, marq wrote:
> > > What's the use case for tracking multiple start/stop calls? Each observer
> > > observes a single label, so why can't  the second and subsequent
> > -startObserving
> > > calls be no-ops, and the first -stopObserving call end all observation and
> > every
> > > subsequent one be a no-op?
> > 
> > Each observer observes a single label but a controller does not know if the
> > label is already being observed.
> > For example on the first run view, the label is observed by the
> > welcome_to_chrome_view, and by the label link controller. And if another
> > controller stop observing your label, you might not want to stop observing
it.
> 
> In practice, this is just so the line-height setting observer(s) used in the
> welcome view can be shared with the link controller(s) used on the same
labels.
> I thought it would be better to just rip out the line-setting class extension
> and use a simple helper function to handle this in the welcome view, but doing
> that broke the link handling in annoying ways. So I'm OK with this approach
> here, but the whole thing is a mess overall.

I agree but refactoring all this might be outside the scope of this CL :)

https://codereview.chromium.org/2828743002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/util/label_observer.mm (right):

https://codereview.chromium.org/2828743002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/util/label_observer.mm:130: if (self.observingCount-- ==
1) {
On 2017/04/20 15:51:34, stkhapugin wrote:
> Please rewrite the condition similar to line 120.

Done.

[gambard, 2017-04-21 07:20:45]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[gambard, 2017-04-21 07:20:51]

The CQ bit was unchecked by gambard@chromium.org

[gambard, 2017-04-21 07:20:51]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-21 07:20:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2828743002/#ps120001
(title: "Address comments")

[commit-bot: I haz the power, 2017-04-21 07:20:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 07:30:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 07:30:30]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-04-21 07:31:52]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-04-21 07:32:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 07:35:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492759912579780,
"parent_rev": "7d42d2d5aea3dfe1bce1c1d458d5d02a2bb3d373", "commit_rev":
"3a05731eee64a75591868d5dfb4baf4bf42bd3a1"}

[commit-bot: I haz the power, 2017-04-21 07:36:02]

Description was changed from

==========
LabelObserver is no longer retained by the label

Having the LabelObserver being retained by the label as associated object
creates a crash when compiling with ARC.
This CL prepares the LabelObserver to be converted to ARC by changing the
associated link from strong to link. The LabelObserver now needs to be retained
by an external object, and the observing needs to be started/stopped.

BUG=713169
==========

to

==========
LabelObserver is no longer retained by the label

Having the LabelObserver being retained by the label as associated object
creates a crash when compiling with ARC.
This CL prepares the LabelObserver to be converted to ARC by changing the
associated link from strong to link. The LabelObserver now needs to be retained
by an external object, and the observing needs to be started/stopped.

BUG=713169

Review-Url: https://codereview.chromium.org/2828743002
Cr-Commit-Position: refs/heads/master@{#466283}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a05731eee64a75591868d5dfb4b...
==========

[commit-bot: I haz the power, 2017-04-21 07:36:03]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3a05731eee64a75591868d5dfb4b...