Mac: Make ScopedTypeRef require explicit constructors

Mac: Make ScopedTypeRef require explicit constructors

Style guide requires explicit constructors.

Prior to this patch, a T* can be passed into a function that requires
a scoped_nsobject<T>, and the object will be released but not retained.

Also make the copy constructor that takes a different typename more
generic, allowing for different types (e.g, subclasses) in addition to
different traits.

Clean up all instances where we relied on this behavior.

BUG=

Committed: https://crrev.com/1c9b983a8576851000408b49bf73090c4986240a
Cr-Commit-Position: refs/heads/master@{#397847}

[ccameron, 2016-06-01 00:39:10]

ccameron@chromium.org changed reviewers:
+ eugenebut@chromium.org, mark@chromium.org, reillyg@chromium.org,
sky@chromium.org

[ccameron, 2016-06-01 00:39:11]

Adding mark@ for base/mac/ review.

Need OWNERs from:
sky@: chrome/, components/bookmarks/
eugenebut@: ios/
reillyg@: device/

https://codereview.chromium.org/2028823003/diff/1/base/mac/scoped_typeref.h
File base/mac/scoped_typeref.h (left):

https://codereview.chromium.org/2028823003/diff/1/base/mac/scoped_typeref.h#o...
base/mac/scoped_typeref.h:72: // ASSUME ownership of A, when a retain is what
was needed.
Note that this is not true anymore, now that we have added the "explicit" to the
constructor above.

[Eugene But (OOO till 7-30), 2016-06-01 01:00:16]

FYI: ios fails to compile

[Reilly Grant (use Gerrit), 2016-06-01 01:05:15]

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
I believe this retain was and still is unnecessary.

[ccameron, 2016-06-01 01:59:31]

Thanks! Fixed the remaining iOS failure.

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
On 2016/06/01 01:05:15, Reilly Grant wrote:
> I believe this retain was and still is unnecessary.

Yeah, I suspected. Removed.

[ortuno, 2016-06-01 02:43:53]

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

[ortuno, 2016-06-01 02:43:53]

cc: jlebel who manages bluetooth's osx code

[jlebel, 2016-06-01 02:50:01]

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

[jlebel, 2016-06-01 02:50:02]

lgtm

LGTM

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:44: scoped_nsobject<NSDictionary>
CreateAdvertisementData(NSString* name,
Nit: Since there is no question about retain/release now, this function can be
renamed to AdvertisementData()

[sky, 2016-06-01 03:35:33]

https://codereview.chromium.org/2028823003/diff/60001/chrome/browser/local_di...
File chrome/browser/local_discovery/service_discovery_client_mac_unittest.mm
(right):

https://codereview.chromium.org/2028823003/diff/60001/chrome/browser/local_di...
chrome/browser/local_discovery/service_discovery_client_mac_unittest.mm:143:
base::scoped_nsobject<NSNetService>(test_service));
Is it worth a template like WrapUnique so we don't need the <class> everywhere?

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>(advertisement_data);
This seems weird. is there nothing like std::move for scoped_nsobject?

[Eugene But (OOO till 7-30), 2016-06-01 05:12:07]

ios lgtm, but I think you have a crash now...

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
On 2016/06/01 01:05:15, Reilly Grant wrote:
> I believe this retain was and still is unnecessary.
Wait... this retain is still necessary (and actually was necessary).
1.) |dictionaryWithDictionary:| returns autoreleased objects 
2.) scoped_nsobject takes ownership over |advertisement_data|
3.) scoped_nsobject releases |advertisement_data| on destruction
4.) autorelease pool releases |advertisement_data| when it's drained

So dropping |retain| will lead to a crash, unless I'm missing something.

https://codereview.chromium.org/2028823003/diff/40001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge_unittest.mm (right):

https://codereview.chromium.org/2028823003/diff/40001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge_unittest.mm:56:
context_menu_params.view.reset([UIView new]);
Optional NIT: Since you touching this, could you please s/[UIView new]/[[UIView
alloc] init]

https://google.github.io/styleguide/objcguide.xml#Avoid_+new

[Reilly Grant (use Gerrit), 2016-06-01 15:34:43]

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
I only pretend to understand Objective-C memory management. The documentation
doesn't *say* that it returns an autoreleased object but that makes sense.

[Eugene But (OOO till 7-30), 2016-06-01 15:58:35]

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
On 2016/06/01 15:34:42, Reilly Grant wrote:
> I only pretend to understand Objective-C memory management. The documentation
> doesn't *say* that it returns an autoreleased object but that makes sense.
In Objective-C the method prefix tell if you are responsible for releasing the
object or not.
The following prefixes require callers to own the result:
- new
- alloc
- copy
- mutableCopy

https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Memory...

[jlebel, 2016-06-02 23:07:34]

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
On 2016/06/01 15:58:35, Eugene But wrote:
> On 2016/06/01 15:34:42, Reilly Grant wrote:
> > I only pretend to understand Objective-C memory management. The
documentation
> > doesn't *say* that it returns an autoreleased object but that makes sense.
> In Objective-C the method prefix tell if you are responsible for releasing the
> object or not.
> The following prefixes require callers to own the result:
> - new
> - alloc
> - copy
> - mutableCopy
> 
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Memory...


And "create" when we are using functions. That's the way it works for
CoreFoundation.

[ccameron, 2016-06-02 23:11:19]

Updated -- ptal

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>([advertisement_data retain]);
On 2016/06/01 15:58:35, Eugene But wrote:
> On 2016/06/01 15:34:42, Reilly Grant wrote:
> > I only pretend to understand Objective-C memory management. The
documentation
> > doesn't *say* that it returns an autoreleased object but that makes sense.
> In Objective-C the method prefix tell if you are responsible for releasing the
> object or not.
> The following prefixes require callers to own the result:
> - new
> - alloc
> - copy
> - mutableCopy
> 
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Memory...

I've added a base::scoped_policy::RETAIN here.

https://codereview.chromium.org/2028823003/diff/60001/chrome/browser/local_di...
File chrome/browser/local_discovery/service_discovery_client_mac_unittest.mm
(right):

https://codereview.chromium.org/2028823003/diff/60001/chrome/browser/local_di...
chrome/browser/local_discovery/service_discovery_client_mac_unittest.mm:143:
base::scoped_nsobject<NSNetService>(test_service));
On 2016/06/01 03:35:33, sky wrote:
> Is it worth a template like WrapUnique so we don't need the <class>
everywhere?

This is a cast from scoped_nsobject<TestNSNetService> to
scoped_nsobject<NSNetService> -- a Wrap would be a good idea to add at some
point, but it wouldn't help here.

Now, because NSNetService is a superclass of TestNSNetService, one could argue
that this should not require an explicit cast. That seems to be to be a good
idea, but it should be implemented at the scoped_nsobject level, because only at
that level are we sure to have the same retain/release behavior.

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:44: scoped_nsobject<NSDictionary>
CreateAdvertisementData(NSString* name,
On 2016/06/01 02:50:01, jlebel wrote:
> Nit: Since there is no question about retain/release now, this function can be
> renamed to AdvertisementData()

n/a now.

https://codereview.chromium.org/2028823003/diff/60001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:56: return
scoped_nsobject<NSDictionary>(advertisement_data);
On 2016/06/01 03:35:33, sky wrote:
> This seems weird. is there nothing like std::move for scoped_nsobject?

This was a cast from NSMutableDictionary to its superclass NSDictionary.

I've changed this to a base::scoped_policy::RETAIN of the sub-class (see
comments from the thread).

[Eugene But (OOO till 7-30), 2016-06-02 23:19:04]

lgtm for memory management fix

[Mark Mentovai, 2016-06-03 00:01:40]

LGTM

[sky, 2016-06-03 15:11:20]

LGTM

[ccameron, 2016-06-03 17:04:18]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-06-03 17:04:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jlebel@chromium.org
Link to the patchset: https://codereview.chromium.org/2028823003/#ps80001
(title: "Re-add retain")

[commit-bot: I haz the power, 2016-06-03 17:04:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028823003/80001

[commit-bot: I haz the power, 2016-06-03 17:13:35]

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

[commit-bot: I haz the power, 2016-06-03 17:13:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2016-06-03 17:16:07]

On 2016/06/01 02:43:53, ortuno wrote:
> cc: jlebel who manages bluetooth's osx code

ortuno@, I still need an lg from an OWNER of the directory. Perhaps we should
add jlebel as an OWNER of that path, for Mac code?

[ortuno, 2016-06-03 17:20:10]

device/bluetooth lgtm. jlebel is not a committer yet. Once he is, we'll add him
to OWNERS.

[ccameron, 2016-06-03 17:25:02]

Ah, I see. Thanks!

[ccameron, 2016-06-03 17:31:06]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-06-03 17:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
ortuno@chromium.org, eugenebut@chromium.org, jlebel@chromium.org,
mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2028823003/#ps100001
(title: "Fix ChooserDialogCocoa default values (not needed)")

[commit-bot: I haz the power, 2016-06-03 17:32:26]

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

[commit-bot: I haz the power, 2016-06-03 17:51:30]

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

[commit-bot: I haz the power, 2016-06-03 17:51:31]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[ccameron, 2016-06-03 20:33:03]

On 2016/06/03 17:51:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

Ack! New uses of the non-explicit constructor are coming in faster than I can
kill them! Hopefully this next patch will get it.

[ccameron, 2016-06-03 22:22:30]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-06-03 22:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
ortuno@chromium.org, eugenebut@chromium.org, jlebel@chromium.org,
mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2028823003/#ps140001
(title: "Fix new instance in unittest")

[commit-bot: I haz the power, 2016-06-03 22:22:59]

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

[commit-bot: I haz the power, 2016-06-03 22:23:01]

This CL has an open dependency (Issue 2028823003 Patch 120001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[ccameron, 2016-06-03 22:24:54]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-06-03 22:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
ortuno@chromium.org, eugenebut@chromium.org, jlebel@chromium.org,
mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2028823003/#ps160001
(title: "Remove dependency")

[commit-bot: I haz the power, 2016-06-03 22:25:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028823003/160001

[commit-bot: I haz the power, 2016-06-03 23:54:25]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-03 23:57:41]

Description was changed from

==========
Mac: Make ScopedTypeRef require explicit constructors

Style guide requires explicit constructors.

Prior to this patch, a T* can be passed into a function that requires
a scoped_nsobject<T>, and the object will be released but not retained.

Also make the copy constructor that takes a different typename more
generic, allowing for different types (e.g, subclasses) in addition to
different traits.

Clean up all instances where we relied on this behavior.

BUG=
==========

to

==========
Mac: Make ScopedTypeRef require explicit constructors

Style guide requires explicit constructors.

Prior to this patch, a T* can be passed into a function that requires
a scoped_nsobject<T>, and the object will be released but not retained.

Also make the copy constructor that takes a different typename more
generic, allowing for different types (e.g, subclasses) in addition to
different traits.

Clean up all instances where we relied on this behavior.

BUG=

Committed: https://crrev.com/1c9b983a8576851000408b49bf73090c4986240a
Cr-Commit-Position: refs/heads/master@{#397847}
==========

[commit-bot: I haz the power, 2016-06-03 23:57:43]

Patchset 9 (id:??) landed as
https://crrev.com/1c9b983a8576851000408b49bf73090c4986240a
Cr-Commit-Position: refs/heads/master@{#397847}