Enable ARC for //ios/web_view/internal/translate/*.m.

Enable ARC for //ios/web_view/internal/translate/*.m.

This CL also (hopefully) fixes occasional SEGV of ios_web_view_shell, by marking WebViewTranslateClient::delegete_ as __weak and also initializing it with nil.

I believe the SEGV happened because:
- Actually web_view_translate_client.h has been already built with ARC *enabled* because it's included by cwv_web_view.mm.
- With ARC enabled, WebViewTranslateClient::delegete_ was a strong reference.
- WebViewTranslateClient::delegete_ was not initialized, so it held an uninitialized pointer initially.
- So call to WebViewTranslateClient::set_translate_delegate() tries to free an uninitialized pointer, causing SEGV.


BUG=703564
Review-Url: https://codereview.chromium.org/2762023003
Cr-Commit-Position: refs/heads/master@{#458688}
Committed: https://chromium.googlesource.com/chromium/src/+/d78e66457cf8aa8a82f0cfc8a387eb8a71655bb7

[Hiroshi Ichikawa, 2017-03-21 09:37:42]

Description was changed from

==========
Enable ARC for //ios/web_view/internal/translate/*.m.

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

to

==========
Enable ARC for //ios/web_view/internal/translate/*.m.

This CL also (hopefully) fixes occasional SEGV of ios_web_view_shell, by marking
WebViewTranslateClient::delegete_ as __weak and also initializing it with nil.

I believe the SEGV happened because:
- Actually web_view_translate_client.h has been already built with ARC *enabled*
because it's included by cwv_web_view.mm.
- With ARC enabled, WebViewTranslateClient::delegete_ was a strong reference.
- WebViewTranslateClient::delegete_ was not initialized, so it held an
uninitialized pointer initially.
- So call to WebViewTranslateClient::set_translate_delegate() tries to free an
uninitialized pointer, causing SEGV.


BUG=703564
==========

[Hiroshi Ichikawa, 2017-03-21 09:38:29]

ichikawa@chromium.org changed reviewers:
+ michaeldo@chromium.org

[michaeldo, 2017-03-21 16:29:18]

Thank you!

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_client.h (right):

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_client.h:75: __weak
id<CWVTranslateDelegate> delegate_;
Please use base::WeakNSProtocol<id<CWVTranslateDelegate>> instead of __weak.
(Note that you will need to import the delegate header and remove the @protocol
from above to avoid a cryptic error.)

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_client.mm (right):

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_client.mm:45: delegate_(nil)
{}
please fix closing paren location and format to line up with translate_manger
line.

However, please test with WeakNSProtocol as I don't think you'll need this
anymore.

[Hiroshi Ichikawa, 2017-03-22 01:32:16]

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_client.h (right):

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_client.h:75: __weak
id<CWVTranslateDelegate> delegate_;
On 2017/03/21 16:29:18, michaeldo wrote:
> Please use base::WeakNSProtocol<id<CWVTranslateDelegate>> instead of __weak.
> (Note that you will need to import the delegate header and remove the
@protocol
> from above to avoid a cryptic error.)

Done. Just curious, what is advantage of using WeakNSProtocol instead of __weak?

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_client.mm (right):

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_client.mm:45: delegate_(nil)
{}
On 2017/03/21 16:29:18, michaeldo wrote:
> please fix closing paren location and format to line up with translate_manger
> line.
> 
> However, please test with WeakNSProtocol as I don't think you'll need this
> anymore.

Removed after switching to WeakNSProtocol.

[michaeldo, 2017-03-22 02:13:47]

Thanks, lgtm

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_client.h (right):

https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_client.h:75: __weak
id<CWVTranslateDelegate> delegate_;
On 2017/03/22 01:32:15, Hiroshi Ichikawa wrote:
> On 2017/03/21 16:29:18, michaeldo wrote:
> > Please use base::WeakNSProtocol<id<CWVTranslateDelegate>> instead of __weak.
> > (Note that you will need to import the delegate header and remove the
> @protocol
> > from above to avoid a cryptic error.)
> 
> Done. Just curious, what is advantage of using WeakNSProtocol instead of
__weak?

Good question, I don't have all the details, but it certainly plays nice with
C++ and has some other guarantees like has unittests to ensure it is nil if
never set. It is modeled after scoped_nsobject which can be used to scope an
Obj-C instance even within a C++ method body. These helpers also predate ARC
adoption where a scoped obj-C obj meant you didn't need to manually release it.

[Hiroshi Ichikawa, 2017-03-22 05:32:37]

On 2017/03/22 02:13:47, michaeldo wrote:
> Thanks, lgtm
> 
>
https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
> File ios/web_view/internal/translate/web_view_translate_client.h (right):
> 
>
https://codereview.chromium.org/2762023003/diff/1/ios/web_view/internal/trans...
> ios/web_view/internal/translate/web_view_translate_client.h:75: __weak
> id<CWVTranslateDelegate> delegate_;
> On 2017/03/22 01:32:15, Hiroshi Ichikawa wrote:
> > On 2017/03/21 16:29:18, michaeldo wrote:
> > > Please use base::WeakNSProtocol<id<CWVTranslateDelegate>> instead of
__weak.
> > > (Note that you will need to import the delegate header and remove the
> > @protocol
> > > from above to avoid a cryptic error.)
> > 
> > Done. Just curious, what is advantage of using WeakNSProtocol instead of
> __weak?
> 
> Good question, I don't have all the details, but it certainly plays nice with
> C++ and has some other guarantees like has unittests to ensure it is nil if
> never set. It is modeled after scoped_nsobject which can be used to scope an
> Obj-C instance even within a C++ method body. These helpers also predate ARC
> adoption where a scoped obj-C obj meant you didn't need to manually release
it.

Hmm it looks like ARC works fine with raw pointer local variables in C++
methods:

class Foo {
public:
  void Bar() {
    Hoge* hoge = [[Hoge alloc] init];
    NSLog(@"hoge = %@", hoge);
    // hoge is dealloced here
  }
};

I agree that it's a benefit that it is automatically initialized with nil.

[Hiroshi Ichikawa, 2017-03-22 05:40:27]

The CQ bit was checked by ichikawa@chromium.org

[commit-bot: I haz the power, 2017-03-22 05:40:50]

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-03-22 07:29:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490161227062760,
"parent_rev": "24c83fb4132db2d3ec271c7ae3ad22f6dcc9b5bc", "commit_rev":
"4ae5342d6cf0c9b6949817bc62135534ec06f49c"}

[commit-bot: I haz the power, 2017-03-22 07:45:30]

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

[commit-bot: I haz the power, 2017-03-22 07:45:32]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[Hiroshi Ichikawa, 2017-03-22 08:50:56]

The CQ bit was checked by ichikawa@chromium.org

[commit-bot: I haz the power, 2017-03-22 08:51:09]

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-03-22 08:55:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490172656417690,
"parent_rev": "2dd1b283c45c4bf5922de632a2b863d3339816db", "commit_rev":
"d78e66457cf8aa8a82f0cfc8a387eb8a71655bb7"}

[commit-bot: I haz the power, 2017-03-22 08:55:44]

Description was changed from

==========
Enable ARC for //ios/web_view/internal/translate/*.m.

This CL also (hopefully) fixes occasional SEGV of ios_web_view_shell, by marking
WebViewTranslateClient::delegete_ as __weak and also initializing it with nil.

I believe the SEGV happened because:
- Actually web_view_translate_client.h has been already built with ARC *enabled*
because it's included by cwv_web_view.mm.
- With ARC enabled, WebViewTranslateClient::delegete_ was a strong reference.
- WebViewTranslateClient::delegete_ was not initialized, so it held an
uninitialized pointer initially.
- So call to WebViewTranslateClient::set_translate_delegate() tries to free an
uninitialized pointer, causing SEGV.


BUG=703564
==========

to

==========
Enable ARC for //ios/web_view/internal/translate/*.m.

This CL also (hopefully) fixes occasional SEGV of ios_web_view_shell, by marking
WebViewTranslateClient::delegete_ as __weak and also initializing it with nil.

I believe the SEGV happened because:
- Actually web_view_translate_client.h has been already built with ARC *enabled*
because it's included by cwv_web_view.mm.
- With ARC enabled, WebViewTranslateClient::delegete_ was a strong reference.
- WebViewTranslateClient::delegete_ was not initialized, so it held an
uninitialized pointer initially.
- So call to WebViewTranslateClient::set_translate_delegate() tries to free an
uninitialized pointer, causing SEGV.


BUG=703564

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

[commit-bot: I haz the power, 2017-03-22 08:55:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d78e66457cf8aa8a82f0cfc8a387...