Fix memory management of VoiceSearchBarView.

Fix memory management of VoiceSearchBarView.

The VoiceSearchProvider is written in ARC, so the returned value is
autoreleased despite the function having "Created" in the name.  This
CL correctly retains the returned value to prevent over-releasing the
view.

BUG=679719
Review-Url: https://codereview.chromium.org/2644703003
Cr-Commit-Position: refs/heads/master@{#445769}
Committed: https://chromium.googlesource.com/chromium/src/+/d297fb597145bbfc86fb1c243091669f09dc6df5

[kkhorimoto, 2017-01-18 22:20:06]

kkhorimoto@chromium.org changed reviewers:
+ rohitrao@chromium.org, stkhapugin@chromium.org

[kkhorimoto, 2017-01-18 22:20:06]

PTAL.  I'm curious if either of you have any suggestions for naming this
provider function.  It currently has a "Create" prefix, which suggests that the
returned value should have a +1 retain count, but since we're moving toward ARC
and this doesn't follow the ARC naming conventions, the returned value is
autoreleased.  Is there some kind of annotation we can add to the C++ function
to instruct ARC to not autorelease the return value?  Or should we just rename
the function so it's more obvious that the returned value is autoreleased?

[stkhapugin, 2017-01-19 12:20:01]

lgtm

> Is there an annotation
Yes, but we're not sure it works. We've seen bugs with it before
and those are supposed to have been fixed, but we've never
verified those. You're looking for NS_RETURNS_RETAINED and
NS_CONSUMED annotations.  

Since the annotations were found to be buggy, it's better to always
autorelease the return values. It does not add significant overhead,
especially with ARC. 

Note that "Create" is not a prefix (per style guidelines) that indicates
that an *Objective-C* object is returned retained. In fact, there is no
similar naming convention for C functions and C++ methods, so
writing such a function should be considered an error. 

Create is a prefix that indicates that a returned Core Foundation
object is retained. For Objective-C methods, such prefixes are:
“alloc”, “new”, “copy”, or “mutableCopy”.

[noyau (Ping after 24h), 2017-01-19 12:58:55]

On 2017/01/18 22:20:06, kkhorimoto_ wrote:
> PTAL.  I'm curious if either of you have any suggestions for naming this
> provider function.  It currently has a "Create" prefix, which suggests that
the
> returned value should have a +1 retain count, but since we're moving toward
ARC
> and this doesn't follow the ARC naming conventions, the returned value is
> autoreleased.  Is there some kind of annotation we can add to the C++ function
> to instruct ARC to not autorelease the return value?  Or should we just rename
> the function so it's more obvious that the returned value is autoreleased?

ARC doesn't do anything with Create. But the fact that someone got caught
forgetting a retain is a clear indication that the Create prefix is confusing.
Please rename those functions. s/Create/Build/ is one approach.

[kkhorimoto, 2017-01-19 20:51:23]

Yeah speaking of the annotations, this function actually WAS annotated with the
returns retained annotation.  But maybe since the annotation was in the
superclass it didn't work as intended?  In any case, I removed the annotation
and documented that the function should return an autoreleased value.  I also
updated the Create prefix to Build, as per noyau's request.  I kept the
CreateVoiceSearchController with the Create prefix because it's returning a C++
refptr.

[kkhorimoto, 2017-01-19 20:53:30]

kkhorimoto@chromium.org changed reviewers:
+ noyau@chromium.org

[kkhorimoto, 2017-01-19 20:53:30]

Downstream CL with new prefix: https://chromereviews.googleplex.com/562467013/

[rohitrao (ping after 24h), 2017-01-19 20:56:21]

https://codereview.chromium.org/2644703003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/voice/voice_search_provider.h (right):

https://codereview.chromium.org/2644703003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/voice/voice_search_provider.h:44: virtual
UIView<VoiceSearchBar>* BuildVoiceSearchBar(CGRect frame) const;
Could you add a new BuildVoiceSearchBar() method while leaving the existing one
in place, then implement it downstream, then followup with a final upstream CL
to delete the old method?  It's more work, but it will also keep the autoroller
from breaking.  I'd like to avoid breaking the autoroller when possible, because
it ends up being pretty disruptive.

[kkhorimoto, 2017-01-19 23:02:20]

Converted to the first of a 3-way patch.

[rohitrao (ping after 24h), 2017-01-20 14:00:26]

https://codereview.chromium.org/2644703003/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2644703003/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:2294:
->BuildVoiceSearchBar(frame) retain]);
We can't change callers to the new API until the downstream implementation is in
place, can we?

[kkhorimoto, 2017-01-24 01:36:18]

https://codereview.chromium.org/2644703003/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2644703003/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:2294:
->BuildVoiceSearchBar(frame) retain]);
On 2017/01/20 14:00:26, rohitrao wrote:
> We can't change callers to the new API until the downstream implementation is
in
> place, can we?

Moved it to the third of 3-way patch.

[kkhorimoto, 2017-01-24 06:13:24]

PTAL, Rohit

[rohitrao (ping after 24h), 2017-01-24 14:26:06]

lgtm, sorry for all the back-and-forth on this.

[kkhorimoto, 2017-01-24 18:40:54]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-01-24 18:40:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org
Link to the patchset: https://codereview.chromium.org/2644703003/#ps80001
(title: "Don't update BVC")

[commit-bot: I haz the power, 2017-01-24 18:41:47]

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-01-24 18:57:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485283254318380,
"parent_rev": "d137eeb25ed0e2e5f9cae45f9dc9d21b6ecdd32e", "commit_rev":
"d297fb597145bbfc86fb1c243091669f09dc6df5"}

[commit-bot: I haz the power, 2017-01-24 18:57:38]

Description was changed from

==========
Fix memory management of VoiceSearchBarView.

The VoiceSearchProvider is written in ARC, so the returned value is
autoreleased despite the function having "Created" in the name.  This
CL correctly retains the returned value to prevent over-releasing the
view.

BUG=679719
==========

to

==========
Fix memory management of VoiceSearchBarView.

The VoiceSearchProvider is written in ARC, so the returned value is
autoreleased despite the function having "Created" in the name.  This
CL correctly retains the returned value to prevent over-releasing the
view.

BUG=679719

Review-Url: https://codereview.chromium.org/2644703003
Cr-Commit-Position: refs/heads/master@{#445769}
Committed:
https://chromium.googlesource.com/chromium/src/+/d297fb597145bbfc86fb1c243091...
==========

[commit-bot: I haz the power, 2017-01-24 18:57:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d297fb597145bbfc86fb1c243091...