Cleanup ios/web_view.

Cleanup ios/web_view.

Cleanup based on CL: https://codereview.chromium.org/2643773005

BUG=679895
Review-Url: https://codereview.chromium.org/2650323002
Cr-Commit-Position: refs/heads/master@{#446064}
Committed: https://chromium.googlesource.com/chromium/src/+/303e12601e95926ef091ef3760c20b0a85de407b

[michaeldo, 2017-01-24 22:36:18]

Description was changed from

==========
Cleanup ios/web_view.

Cleanup based on CL: https://codereview.chromium.org/2643773005

BUG=none
==========

to

==========
Cleanup ios/web_view.

Cleanup based on CL: https://codereview.chromium.org/2643773005

BUG=679895
==========

[michaeldo, 2017-01-24 22:36:30]

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

[sdefresne, 2017-01-25 09:11:09]

lgtm with some remarks

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_main_parts.mm (right):

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:28: std::string(), NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
nit: NULL -> nullptr

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/shell/shell_ap...
File ios/web_view/shell/shell_app_delegate.mm (right):

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/shell/shell_ap...
ios/web_view/shell/shell_app_delegate.mm:53: [self.window release];
I think setting this to nil will work (since the generated setter dealloc the
old instance) and is more correct (as self.window will not point to a possibly
deallocated object).

  self.window = nil;

Or if you want to have the -release call be explicit, modify the underlying
ivar:

  [_window release];
  _window = nil;

[Eugene But (OOO till 7-30), 2017-01-25 14:28:03]

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

[Eugene But (OOO till 7-30), 2017-01-25 14:28:04]

Driving by...

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/shell/shell_ap...
File ios/web_view/shell/shell_app_delegate.mm (right):

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/shell/shell_ap...
ios/web_view/shell/shell_app_delegate.mm:53: [self.window release];
On 2017/01/25 09:11:09, sdefresne wrote:
> I think setting this to nil will work (since the generated setter dealloc the
> old instance) and is more correct (as self.window will not point to a possibly
> deallocated object).
> 
>   self.window = nil;
> 
> Or if you want to have the -release call be explicit, modify the underlying
> ivar:
> 
>   [_window release];
>   _window = nil;
FWIW: |[self.window release];| or |self.window = nil;| is discouraged by the
style guide:
https://google.github.io/styleguide/objcguide.xml#Avoid_Accessors_During_init...

Calling |[_window release];| is the right suggestion. I don't think |_window =
nil;| adds anything and such constructs is unusual in dealloc.
|

[michaeldo, 2017-01-25 17:23:47]

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/internal/criwv...
File ios/web_view/internal/criwv_web_main_parts.mm (right):

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/internal/criwv...
ios/web_view/internal/criwv_web_main_parts.mm:28: std::string(), NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
On 2017/01/25 09:11:09, sdefresne wrote:
> nit: NULL -> nullptr

Done.

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/shell/shell_ap...
File ios/web_view/shell/shell_app_delegate.mm (right):

https://codereview.chromium.org/2650323002/diff/1/ios/web_view/shell/shell_ap...
ios/web_view/shell/shell_app_delegate.mm:53: [self.window release];
On 2017/01/25 14:28:04, Eugene But wrote:
> On 2017/01/25 09:11:09, sdefresne wrote:
> > I think setting this to nil will work (since the generated setter dealloc
the
> > old instance) and is more correct (as self.window will not point to a
possibly
> > deallocated object).
> > 
> >   self.window = nil;
> > 
> > Or if you want to have the -release call be explicit, modify the underlying
> > ivar:
> > 
> >   [_window release];
> >   _window = nil;
> FWIW: |[self.window release];| or |self.window = nil;| is discouraged by the
> style guide:
>
https://google.github.io/styleguide/objcguide.xml#Avoid_Accessors_During_init...
> 
> Calling |[_window release];| is the right suggestion. I don't think |_window =
> nil;| adds anything and such constructs is unusual in dealloc.
> |

Done.

[michaeldo, 2017-01-25 18:24:36]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-01-25 18:24:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2650323002/#ps20001
(title: "Respond to comments.")

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

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-25 18:35:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485368676556570,
"parent_rev": "3cab0ba1179fcb085af9efb0a2734c51b8d79be3", "commit_rev":
"303e12601e95926ef091ef3760c20b0a85de407b"}

[commit-bot: I haz the power, 2017-01-25 18:36:23]

Description was changed from

==========
Cleanup ios/web_view.

Cleanup based on CL: https://codereview.chromium.org/2643773005

BUG=679895
==========

to

==========
Cleanup ios/web_view.

Cleanup based on CL: https://codereview.chromium.org/2643773005

BUG=679895

Review-Url: https://codereview.chromium.org/2650323002
Cr-Commit-Position: refs/heads/master@{#446064}
Committed:
https://chromium.googlesource.com/chromium/src/+/303e12601e95926ef091ef3760c2...
==========

[commit-bot: I haz the power, 2017-01-25 18:36:24]

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