Error pages: Break retain cycle NativeContent/StaticHTMLViewController

Error pages: Break retain cycle NativeContent/StaticHTMLViewController

StaticHTMLViewController retains the generator.
The generator is implementated by ErrorPageContent, which retain the
StaticHTMLViewController.
This leads to a leak and the WKWebView is never released.

BUG=693178
Review-Url: https://codereview.chromium.org/2695593011
Cr-Commit-Position: refs/heads/master@{#451609}
Committed: https://chromium.googlesource.com/chromium/src/+/fc155c2704ad95817d98efb4c3dbec239c4c17cb

[Olivier, 2017-02-17 08:53:14]

olivierrobin@chromium.org changed reviewers:
+ eugenebut@chromium.org, marq@chromium.org, stkhapugin@chromium.org

[marq (ping after 24h), 2017-02-17 10:25:57]

lgtm

[stkhapugin, 2017-02-17 13:12:44]

lgtm with comments

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
File ios/chrome/browser/web/error_page_content.h (left):

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
ios/chrome/browser/web/error_page_content.h:20: base::scoped_nsobject<NSString>
html_;
I think you don't need this anymore. If so, please remove (and move the #include
to .mm). If you do, please move it into .mm to @interface ErrorPageContent { ...
}

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
File ios/chrome/browser/web/error_page_content.mm (right):

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
ios/chrome/browser/web/error_page_content.mm:82: @end
nit: Please add a #pragma mark - ErrorPageContent, because two classes in one
file are hard to read.

[Olivier, 2017-02-17 13:58:39]

Thanks

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
File ios/chrome/browser/web/error_page_content.h (left):

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
ios/chrome/browser/web/error_page_content.h:20: base::scoped_nsobject<NSString>
html_;
On 2017/02/17 13:12:44, stkhapugin wrote:
> I think you don't need this anymore. If so, please remove (and move the
#include
> to .mm). If you do, please move it into .mm to @interface ErrorPageContent {
...
> } 

Correct, thanks
Done

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
File ios/chrome/browser/web/error_page_content.mm (right):

https://codereview.chromium.org/2695593011/diff/1/ios/chrome/browser/web/erro...
ios/chrome/browser/web/error_page_content.mm:82: @end
On 2017/02/17 13:12:44, stkhapugin wrote:
> nit: Please add a #pragma mark - ErrorPageContent, because two classes in one
> file are hard to read. 

Done.

[Eugene But (OOO till 7-30), 2017-02-17 18:37:55]

LGTM!

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/error_page_content.mm (right):

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:10: #include
"base/mac/scoped_nsobject.h"
nit: s/include/import

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:26: @interface ErrorPageGenerator :
NSObject<HtmlGenerator>
Please add comments to class and initializer. F.e. it is useful to explain that
ErrorPageGenerator is needed because StaticHtmlViewController retains generator
argument.

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:44: NSString* badURLString =
Optional nit. This would fit 80 symbols:

NSString* badURLSpec = error.userInfo[NSURLErrorFailingURLStringErrorKey];

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:80: 
Optional nit: pragma mark HtmlGenerator

[Olivier, 2017-02-20 08:47:00]

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/error_page_content.mm (right):

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:10: #include
"base/mac/scoped_nsobject.h"
On 2017/02/17 18:37:54, Eugene But wrote:
> nit: s/include/import

Done.

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:26: @interface ErrorPageGenerator :
NSObject<HtmlGenerator>
On 2017/02/17 18:37:54, Eugene But wrote:
> Please add comments to class and initializer. F.e. it is useful to explain
that
> ErrorPageGenerator is needed because StaticHtmlViewController retains
generator
> argument. 

Done.

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:44: NSString* badURLString =
On 2017/02/17 18:37:54, Eugene But wrote:
> Optional nit. This would fit 80 symbols:
> 
> NSString* badURLSpec = error.userInfo[NSURLErrorFailingURLStringErrorKey];

Done.

https://codereview.chromium.org/2695593011/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/error_page_content.mm:80: 
On 2017/02/17 18:37:54, Eugene But wrote:
> Optional nit: pragma mark HtmlGenerator

Done.

[Olivier, 2017-02-20 08:47:04]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-02-20 08:47:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org, marq@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2695593011/#ps40001
(title: "feedback")

[commit-bot: I haz the power, 2017-02-20 08:47: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-02-20 08:54:39]

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

[commit-bot: I haz the power, 2017-02-20 08:54:40]

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

[Olivier, 2017-02-20 11:57:13]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-02-20 11:57:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org, eugenebut@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2695593011/#ps60001
(title: "split ErrorPageGenerator and fix unittests")

[commit-bot: I haz the power, 2017-02-20 11:57:31]

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-02-20 12:59:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487591833112910,
"parent_rev": "5509091b50695c6a3a9a1967d3e2bc8df615863e", "commit_rev":
"fc155c2704ad95817d98efb4c3dbec239c4c17cb"}

[commit-bot: I haz the power, 2017-02-20 12:59:56]

Description was changed from

==========
Error pages: Break retain cycle NativeContent/StaticHTMLViewController

StaticHTMLViewController retains the generator.
The generator is implementated by ErrorPageContent, which retain the
StaticHTMLViewController.
This leads to a leak and the WKWebView is never released.

BUG=693178
==========

to

==========
Error pages: Break retain cycle NativeContent/StaticHTMLViewController

StaticHTMLViewController retains the generator.
The generator is implementated by ErrorPageContent, which retain the
StaticHTMLViewController.
This leads to a leak and the WKWebView is never released.

BUG=693178

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

[commit-bot: I haz the power, 2017-02-20 12:59:58]

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