Converts parts of ios/web to ARC

Converts top-level ios/web files, interstitials and navigation directories to ARC.

Removes property_releaser usage. Includes results of ARCMigrate run.

BUG=624365
TEST=None

Committed: https://crrev.com/eb706fb3fce4cb2ee01ba6725163ab6d52c0f0ad
Cr-Commit-Position: refs/heads/master@{#413717}

[stkhapugin, 2016-08-01 19:49:17]

Description was changed from

==========
files, interstitials/, navigation/

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

to

==========
Converts top-level ios/web files, interstitials and navigation directories to
ARC.

Removes property_releaser usage. Includes results of ARCMigrate run.

BUG=624365
TEST=None
==========

[stkhapugin, 2016-08-01 20:42:09]

stkhapugin@chromium.org changed reviewers:
+ eugenebut@chromium.org

[stkhapugin, 2016-08-01 20:42:09]

PTAL

[Eugene But (OOO till 7-30), 2016-08-01 21:13:47]

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

[Eugene But (OOO till 7-30), 2016-08-01 21:13:48]

Thanks for converting this. Adding Kurt who actually owns navigation (not
formally because everything is in the same directory).

https://codereview.chromium.org/2202623002/diff/20001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/BUILD.gn#newcode71
ios/web/BUILD.gn:71: "web_view_creation_util.mm",
Should |CreateWKWebView| be annotated with something?

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.h (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.h:43: @property(weak, nonatomic,
readonly) CRWSessionEntry* currentEntry;
Shouldn't these be strong instead? If no, please put weak to the end of
attributes list to be consistent with strong properties.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:844: [NSString
stringWithString:(__bridge NSString*)uuidStringRef];
Optional NIT: since you touching this could you please replace C-cast with
static_cast.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:207: return [http_request_headers_
copy];
This looks like a leak because compiler does not know that autorelease is needed
here.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:325:
SetSessionController([navigation_manager->GetSessionController() copy]);
This looks like a leak too.

[stkhapugin, 2016-08-18 17:09:36]

Thanks, PTAL. This is waiting for the function annotation CL.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/BUILD.gn#newcode71
ios/web/BUILD.gn:71: "web_view_creation_util.mm",
On 2016/08/01 21:13:48, Eugene But wrote:
> Should |CreateWKWebView| be annotated with something?

Solved with https://codereview.chromium.org/2239773002/

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.h (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.h:43: @property(weak, nonatomic,
readonly) CRWSessionEntry* currentEntry;
On 2016/08/01 21:13:48, Eugene But wrote:
> Shouldn't these be strong instead? If no, please put weak to the end of
> attributes list to be consistent with strong properties.

Done - it doesn't matter for any of those as they are readonly; but I'll mark
them "strong" because instances of this class "own" these.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:844: [NSString
stringWithString:(__bridge NSString*)uuidStringRef];
On 2016/08/01 21:13:48, Eugene But wrote:
> Optional NIT: since you touching this could you please replace C-cast with
> static_cast.

Done.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:207: return [http_request_headers_
copy];
On 2016/08/01 21:13:48, Eugene But wrote:
> This looks like a leak because compiler does not know that autorelease is
needed
> here.

Autorelease will be inserted by clang with ARC.

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2202623002/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:325:
SetSessionController([navigation_manager->GetSessionController() copy]);
On 2016/08/01 21:13:48, Eugene But wrote:
> This looks like a leak too.

Same here, ARC always balances retains and releases (aside for new- alloc- copy-
and NS_RETURNS_RETAINED methods)

[Eugene But (OOO till 7-30), 2016-08-18 17:51:38]

Please wait for Kurt's lg as well. Also could you please convert smaller portion
of files in the future CLs. This way if we have to revert something we will
revert smaller portion of the code, also reviews will be easier. lgtm

https://codereview.chromium.org/2202623002/diff/40001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2202623002/diff/40001/ios/web/BUILD.gn#newcode45
ios/web/BUILD.gn:45: "interstitials/web_interstitial_impl.mm",
_interstitial used to be weak and now it is strong with ARC.

[stkhapugin, 2016-08-19 09:29:11]

Eugene, thanks for the feedback, I'll try to make my CLs smaller in the future.

https://codereview.chromium.org/2202623002/diff/40001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2202623002/diff/40001/ios/web/BUILD.gn#newcode45
ios/web/BUILD.gn:45: "interstitials/web_interstitial_impl.mm",
On 2016/08/18 17:51:38, Eugene But wrote:
> _interstitial used to be weak and now it is strong with ARC.

Do you refer to: 

// ios/web/interstitials/html_web_interstitial_impl.mm

@implementation CRWWebInterstitialImplWKWebViewDelegate {
  web::HtmlWebInterstitialImpl* _interstitial;
}

-----
If yes, the type of this ivar is not a pointer to a Cocoa object, so weak/strong
semantics from Objective-C don't apply.

[stkhapugin, 2016-08-19 09:29:11]

Eugene, thanks for the feedback, I'll try to make my CLs smaller in the future.

[stkhapugin, 2016-08-19 11:43:05]

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

[stkhapugin, 2016-08-19 11:43:19]

Added compile guards.

[commit-bot: I haz the power, 2016-08-19 11:43:19]

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, 2016-08-19 13:19:35]

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

[commit-bot: I haz the power, 2016-08-19 13:19:36]

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...)

[Eugene But (OOO till 7-30), 2016-08-19 15:18:17]

https://codereview.chromium.org/2202623002/diff/40001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/2202623002/diff/40001/ios/web/BUILD.gn#newcode45
ios/web/BUILD.gn:45: "interstitials/web_interstitial_impl.mm",
On 2016/08/19 09:29:11, stkhapugin wrote:
> On 2016/08/18 17:51:38, Eugene But wrote:
> > _interstitial used to be weak and now it is strong with ARC.
> 
> Do you refer to: 
> 
> // ios/web/interstitials/html_web_interstitial_impl.mm
> 
> @implementation CRWWebInterstitialImplWKWebViewDelegate {
>   web::HtmlWebInterstitialImpl* _interstitial;
> }
> 
> -----
> If yes, the type of this ivar is not a pointer to a Cocoa object, so
weak/strong
> semantics from Objective-C don't apply.
D'oh. Sorry about that.

https://codereview.chromium.org/2202623002/diff/60001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2202623002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:849: CFRelease(uuidStringRef);
Was it actually an extra release? 

From "Transitioning to ARC Release Notes":

"The compiler does not automatically manage the lifetimes of Core Foundation
objects; you must call CFRetain and CFRelease (or the corresponding
type-specific variants) as dictated by the Core Foundation memory management
rules (see Memory Management Programming Guide for Core Foundation)."

https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-Transition...

[stkhapugin, 2016-08-19 15:35:02]

Ping Kurt

https://codereview.chromium.org/2202623002/diff/60001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2202623002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:849: CFRelease(uuidStringRef);
On 2016/08/19 15:18:17, Eugene But wrote:
> Was it actually an extra release? 
> 
> From "Transitioning to ARC Release Notes":
> 
> "The compiler does not automatically manage the lifetimes of Core Foundation
> objects; you must call CFRetain and CFRelease (or the corresponding
> type-specific variants) as dictated by the Core Foundation memory management
> rules (see Memory Management Programming Guide for Core Foundation)."
> 
>
https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-Transition...

It wasn't, but it started being one after I inserted a CFBridgingRelease call :)

[kkhorimoto, 2016-08-19 23:15:43]

Generally looking good, but I have some high-level comments:

1) I think that Objective C objects should stop using scoped_nsobjects to back
properties.  There are several places in CRWSessionController where the memory
management would be much easier to reason about if we simply called the
synthesized setters for properties rather than resetting scoped_nsobject ivars. 
This might require redefining readonly properties as readwrite within the .mm
file, but this seems much more lightweight than declaring our own
scoped_nsobject ivars and managing memory ourselves.  If we're transitioning to
ARC, we might as well use it!

2) If ivars defined within C++ classes follow the same rules as ObjC ivars (i.e.
strong references assumed unless defined with __weak), then we might as well get
rid of the scoped_nsobject usage there as well.  I feel less strongly about this
though, as the memory management is not as clearcut since ivars don't correspond
with properties with retain policy annotations.

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:403:
rendererInitiated:rendererInitiated]);
How do scoped_nsobjects work within an ARC context?  The class expects to
receive an object with a +1 retainCount for calls to reset(), and the session
entry factory method here creates an autoreleased object.

Moreover, why are we using scoped_nsobject backing ivars at all post-ARC? 
Couldn't we just use a synthesized setter for |pendingEntry|?  Using
scoped_nsobjects to manage memory in an ARC environment is pretty confusing.

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:227:
serialized_state_object_.reset(serialized_state_object);
How does ARC function for C++ classes that have NSObject ivars?  Is it just
assumed that the variables use strong references?  If so, this is another place
where it'd be better to remove scoped_nsobjects.

[Eugene But (OOO till 7-30), 2016-08-19 23:23:16]

On 2016/08/19 23:15:43, kkhorimoto_ wrote:
> Generally looking good, but I have some high-level comments:
> 
> 1) I think that Objective C objects should stop using scoped_nsobjects to back
> properties.  There are several places in CRWSessionController where the memory
> management would be much easier to reason about if we simply called the
> synthesized setters for properties rather than resetting scoped_nsobject
ivars. 
> This might require redefining readonly properties as readwrite within the .mm
> file, but this seems much more lightweight than declaring our own
> scoped_nsobject ivars and managing memory ourselves.  If we're transitioning
to
> ARC, we might as well use it!
> 
> 2) If ivars defined within C++ classes follow the same rules as ObjC ivars
(i.e.
> strong references assumed unless defined with __weak), then we might as well
get
> rid of the scoped_nsobject usage there as well.  I feel less strongly about
this
> though, as the memory management is not as clearcut since ivars don't
correspond
> with properties with retain policy annotations.
> 
>
https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/crw_...
> File ios/web/navigation/crw_session_controller.mm (right):
> 
>
https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/crw_...
> ios/web/navigation/crw_session_controller.mm:403:
> rendererInitiated:rendererInitiated]);
> How do scoped_nsobjects work within an ARC context?  The class expects to
> receive an object with a +1 retainCount for calls to reset(), and the session
> entry factory method here creates an autoreleased object.
> 
> Moreover, why are we using scoped_nsobject backing ivars at all post-ARC? 
> Couldn't we just use a synthesized setter for |pendingEntry|?  Using
> scoped_nsobjects to manage memory in an ARC environment is pretty confusing.
> 
>
https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/navi...
> File ios/web/navigation/navigation_item_impl.mm (right):
> 
>
https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/navi...
> ios/web/navigation/navigation_item_impl.mm:227:
> serialized_state_object_.reset(serialized_state_object);
> How does ARC function for C++ classes that have NSObject ivars?  Is it just
> assumed that the variables use strong references?  If so, this is another
place
> where it'd be better to remove scoped_nsobjects.
Since we planning to get rid of CRWSessionController I don't think we should
spend time on refactoring it. But even if we wanted to refactor them, it would
be better to do this in a separate CL. Thoughts?

[stkhapugin, 2016-08-22 11:37:01]

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:403:
rendererInitiated:rendererInitiated]);
On 2016/08/19 23:15:43, kkhorimoto_ wrote:
> How do scoped_nsobjects work within an ARC context?  The class expects to
> receive an object with a +1 retainCount for calls to reset(), and the session
> entry factory method here creates an autoreleased object.
> 
> Moreover, why are we using scoped_nsobject backing ivars at all post-ARC? 
> Couldn't we just use a synthesized setter for |pendingEntry|?  Using
> scoped_nsobjects to manage memory in an ARC environment is pretty confusing.

As I said in design docs, PSAs and the lightning talk, scoped_nsobject and
WeakNSObject are correctly functioning with ARC. They are not going to be
rewritten to __strong and __weak in the first round of CLs to keep the code
reviews sanely sized and the flag-flipping process faster. In ARC files, you can
write new code without scoped pointers if you wish.

The process for removing scoped_nsobject/WeakNSObject is mostly there, but will
generate much more code reviews. It is probably going to wait until everything
in ios/ is built with ARC.

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl.mm (right):

https://codereview.chromium.org/2202623002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl.mm:227:
serialized_state_object_.reset(serialized_state_object);
On 2016/08/19 23:15:43, kkhorimoto_ wrote:
> How does ARC function for C++ classes that have NSObject ivars?  Is it just
> assumed that the variables use strong references?  If so, this is another
place
> where it'd be better to remove scoped_nsobjects.

ARC treats all Cocoa object type variables in Objective-C++ files with ARC the
same. See other comment for why this is not going to be fixed now.

[kkhorimoto, 2016-08-22 17:02:14]

lgtm

[stkhapugin, 2016-08-23 11:09:20]

The CQ bit was checked by stkhapugin@chromium.org

[stkhapugin, 2016-08-23 11:09:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2202623002/#ps80001
(title: "Removed extra release")

[commit-bot: I haz the power, 2016-08-23 11:09:33]

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, 2016-08-23 13:17:25]

Description was changed from

==========
Converts top-level ios/web files, interstitials and navigation directories to
ARC.

Removes property_releaser usage. Includes results of ARCMigrate run.

BUG=624365
TEST=None
==========

to

==========
Converts top-level ios/web files, interstitials and navigation directories to
ARC.

Removes property_releaser usage. Includes results of ARCMigrate run.

BUG=624365
TEST=None
==========

[commit-bot: I haz the power, 2016-08-23 13:17:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-23 13:19:26]

Description was changed from

==========
Converts top-level ios/web files, interstitials and navigation directories to
ARC.

Removes property_releaser usage. Includes results of ARCMigrate run.

BUG=624365
TEST=None
==========

to

==========
Converts top-level ios/web files, interstitials and navigation directories to
ARC.

Removes property_releaser usage. Includes results of ARCMigrate run.

BUG=624365
TEST=None

Committed: https://crrev.com/eb706fb3fce4cb2ee01ba6725163ab6d52c0f0ad
Cr-Commit-Position: refs/heads/master@{#413717}
==========

[commit-bot: I haz the power, 2016-08-23 13:19:26]

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