[ObjC ARC] Converts components/signin/ios/browser:browser to ARC.

[ObjC ARC] Converts components/signin/ios/browser:browser to ARC.

Automatically generated ARCMigrate commit

Notable issues: Observer bridge classes need to explicitly declare their delegates weak.

BUG=624363
TEST=None

Review-Url: https://codereview.chromium.org/2928383002
Cr-Commit-Position: refs/heads/master@{#479670}
Committed: https://chromium.googlesource.com/chromium/src/+/a582bda6805aebbb9cc2e3d360aea2e7e9522161

[marq (ping after 24h), 2017-06-12 12:10:01]

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

[commit-bot: I haz the power, 2017-06-12 12:10:10]

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-06-12 13:14:19]

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

[commit-bot: I haz the power, 2017-06-12 13:14:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[marq (ping after 24h), 2017-06-13 07:54:59]

Made delegate explicitly weak.

[marq (ping after 24h), 2017-06-13 07:59:08]

Description was changed from

==========
[ObjC ARC] Converts components/signin/ios/browser:browser to ARC.

Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
==========

to

==========
[ObjC ARC] Converts components/signin/ios/browser:browser to ARC.

Automatically generated ARCMigrate commit

Notable issues: Observer bridge classes need to explicitly declare their
delegates weak.

BUG=624363
TEST=None
==========

[marq (ping after 24h), 2017-06-13 08:06:55]

Made other delegate explicitly weak.

[marq (ping after 24h), 2017-06-13 08:12:20]

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

[commit-bot: I haz the power, 2017-06-13 08:12:33]

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-06-13 08:23:22]

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

[commit-bot: I haz the power, 2017-06-13 09:18:34]

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

[commit-bot: I haz the power, 2017-06-13 09:18:34]

Dry run: This issue passed the CQ dry run.

[stkhapugin, 2017-06-13 12:18:55]

lgtm

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/merge_session_observer_bridge.h (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/merge_session_observer_bridge.h:39:
base::WeakNSProtocol<id<MergeSessionObserverBridgeDelegate>> delegate_;
This seems to be included only from the corresponding .mm, so you can try making
this __weak instead.

[msarda, 2017-06-13 13:15:30]

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/account_consistency_service.h (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/account_consistency_service.h:14: #include
"base/mac/scoped_nsobject.h"
Should this include be removed?

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/account_consistency_service.h:181:
base::scoped_nsobject<WKWebView> web_view_;
Should this change and be now a WKWebView*?

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/account_consistency_service.mm (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/account_consistency_service.mm:396:
web_view_.reset(BuildWKWebView());
I think this will assign an object that is autoreleased to a scoped_nsobject.

This code is very tricky - may I ask you to do the following test:
* Fresh install Chrome.
* Start Chrome and from settings sign in to Chrome with any Google account.
* Go to mail.google.com and observe if you are signed in to gmail.

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/merge_session_observer_bridge.h (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/merge_session_observer_bridge.h:39:
base::WeakNSProtocol<id<MergeSessionObserverBridgeDelegate>> delegate_;
I do not know that much about ARC transition, so this is probably a newbie
question: Shouldn't this be __weak (or maybe a weak property) instead of
base::WeakNSProtocol?

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/oauth2_token_service_observer_bridge.h
(right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/oauth2_token_service_observer_bridge.h:53:
base::WeakNSProtocol<id<OAuth2TokenServiceObserverBridgeDelegate>> delegate_;
I do not know that much about ARC transition, so this is probably a newbie
question: Shouldn't this be __weak (or maybe a weak property) instead of
base::WeakNSProtocol?

[marq (ping after 24h), 2017-06-15 09:25:29]

Review fixes.

[marq (ping after 24h), 2017-06-15 09:27:33]

Thanks, PTAL.

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/account_consistency_service.h (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/account_consistency_service.h:14: #include
"base/mac/scoped_nsobject.h"
On 2017/06/13 13:15:30, msarda wrote:
> Should this include be removed?

Done.

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/account_consistency_service.h:181:
base::scoped_nsobject<WKWebView> web_view_;
On 2017/06/13 13:15:30, msarda wrote:
> Should this change and be now a WKWebView*?

We can only do that in header files if the header isn't included in any non-ARC
targets. It looks like this one isn't used outside of this target, so it should
be OK to clean up that way.

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/account_consistency_service.mm (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/account_consistency_service.mm:396:
web_view_.reset(BuildWKWebView());
On 2017/06/13 13:15:30, msarda wrote:
> I think this will assign an object that is autoreleased to a scoped_nsobject.

It should have (had I kept it a scoped object), under ARC, seen that the scoped
object keeps a strong reference. But it's just a simple assignment now.
 
> This code is very tricky - may I ask you to do the following test:
> * Fresh install Chrome.
> * Start Chrome and from settings sign in to Chrome with any Google account.
> * Go to http://mail.google.com and observe if you are signed in to gmail.
> 

That work fine, thanks for the test steps!

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/merge_session_observer_bridge.h (right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/merge_session_observer_bridge.h:39:
base::WeakNSProtocol<id<MergeSessionObserverBridgeDelegate>> delegate_;
On 2017/06/13 13:15:30, msarda wrote:
> I do not know that much about ARC transition, so this is probably a newbie
> question: Shouldn't this be __weak (or maybe a weak property) instead of
> base::WeakNSProtocol?

Because it's in a header, it can only be __weak if it's only included in ARC
targets. Otherwise it needs to be a WeakNSProtocol. It looks like we can change
this one, though.

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
File components/signin/ios/browser/oauth2_token_service_observer_bridge.h
(right):

https://codereview.chromium.org/2928383002/diff/40001/components/signin/ios/b...
components/signin/ios/browser/oauth2_token_service_observer_bridge.h:53:
base::WeakNSProtocol<id<OAuth2TokenServiceObserverBridgeDelegate>> delegate_;
On 2017/06/13 13:15:30, msarda wrote:
> I do not know that much about ARC transition, so this is probably a newbie
> question: Shouldn't this be __weak (or maybe a weak property) instead of
> base::WeakNSProtocol?

This header is included in non-arc targets, so it has to stay a WeakNSProtocol.

[msarda, 2017-06-15 09:31:48]

lgtm

[marq (ping after 24h), 2017-06-15 09:53:57]

The CQ bit was checked by marq@chromium.org

[marq (ping after 24h), 2017-06-15 09:53:58]

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/2928383002/#ps60001
(title: "Review fixes.")

[commit-bot: I haz the power, 2017-06-15 09:54:13]

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-06-15 11:57:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497520437761030,
"parent_rev": "61479a854982e3749c4322a4c5dede07b713a163", "commit_rev":
"a582bda6805aebbb9cc2e3d360aea2e7e9522161"}

[commit-bot: I haz the power, 2017-06-15 11:57:44]

Description was changed from

==========
[ObjC ARC] Converts components/signin/ios/browser:browser to ARC.

Automatically generated ARCMigrate commit

Notable issues: Observer bridge classes need to explicitly declare their
delegates weak.

BUG=624363
TEST=None
==========

to

==========
[ObjC ARC] Converts components/signin/ios/browser:browser to ARC.

Automatically generated ARCMigrate commit

Notable issues: Observer bridge classes need to explicitly declare their
delegates weak.

BUG=624363
TEST=None

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

[commit-bot: I haz the power, 2017-06-15 11:57:45]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a582bda6805aebbb9cc2e3d360ae...