[ObjC ARC] Converts ios/chrome/browser/ui:ui_internal_arc to ARC.

[ObjC ARC] Converts ios/chrome/browser/ui:ui_internal_arc to ARC.

Automatically generated ARCMigrate commit
Notable issues:
 * This temporarily splits ui_internal into two targets to simplify review. The targets
are merged in a dependent CL.

BUG=624363
TEST=None

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

[stkhapugin, 2017-04-05 14:08:40]

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

[commit-bot: I haz the power, 2017-04-05 14:09:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stkhapugin, 2017-04-05 15:18:57]

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

[commit-bot: I haz the power, 2017-04-05 15:19:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stkhapugin, 2017-04-05 15:21:11]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/browser/ui:ui_internal_arc to ARC.

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

to

==========
[ObjC ARC] Converts ios/chrome/browser/ui:ui_internal_arc to ARC.

Automatically generated ARCMigrate commit
Notable issues:
 * This temporarily splits ui_internal into two targets to simplify review. The
targets
are merged in a dependent CL. 

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

[stkhapugin, 2017-04-05 16:14:53]

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

[stkhapugin, 2017-04-05 16:14:53]

PTAL

[stkhapugin, 2017-04-05 16:14:54]

PTAL

[commit-bot: I haz the power, 2017-04-05 16:23:13]

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

[commit-bot: I haz the power, 2017-04-05 16:23:14]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-04-06 11:23:45]

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/e...
File ios/chrome/browser/ui/external_file_remover.mm (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/e...
ios/chrome/browser/ui/external_file_remover.mm:91: web::WebThread::UI,
FROM_HERE, base::BindBlockArc(^{
Followup:

I looks like all objects involved here are C++ object, so there is little point
using base::BindBlockArc. Can this be instead changed to a base::Bind instead,
like this:

  bool remove_all_files = delay == base::TimeDelta::FromSeconds(0);
  base::ScopedClosureRunner scoped_closure_runner(callback);
  web::WebThread::PostDelayedTask(
      web::WebThread::UI, FROM_HERE,
      base::Bind(
          [](base::WeakPtr<ExternalFileRemover> remover, bool all_files,
             base::ScopedClosureRunner runner) {
            DCHECK(remover.get());
            remover->RemoveFiles(all_files, runner.Release());
          },
          weak_ptr_factory_.GetWeakPtr(), remove_all_files,
          base::Passed(&scoped_closure_runner)),
      delay);

Or if you change the prototype of RemoveFiles to take a ScopedClosureRunner,
then you can just bind the method directly and remove the wrapping lambda.

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/fullscreen_controller.mm (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/fullscreen_controller.mm:199: if (!gEnabledForTests) {
Remove braces as the rest of the file does not use them for simple blocks.

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/preload_controller.h (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/preload_controller.h:54: @property(nonatomic,
unsafe_unretained) id<PreloadControllerDelegate> delegate;
Can't this be weak?

[stkhapugin, 2017-04-07 13:02:09]

Thanks, ptal

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/e...
File ios/chrome/browser/ui/external_file_remover.mm (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/e...
ios/chrome/browser/ui/external_file_remover.mm:91: web::WebThread::UI,
FROM_HERE, base::BindBlockArc(^{
On 2017/04/06 11:23:45, sdefresne wrote:
> Followup:
> 
> I looks like all objects involved here are C++ object, so there is little
point
> using base::BindBlockArc. Can this be instead changed to a base::Bind instead,
> like this:
> 
>   bool remove_all_files = delay == base::TimeDelta::FromSeconds(0);
>   base::ScopedClosureRunner scoped_closure_runner(callback);
>   web::WebThread::PostDelayedTask(
>       web::WebThread::UI, FROM_HERE,
>       base::Bind(
>           [](base::WeakPtr<ExternalFileRemover> remover, bool all_files,
>              base::ScopedClosureRunner runner) {
>             DCHECK(remover.get());
>             remover->RemoveFiles(all_files, runner.Release());
>           },
>           weak_ptr_factory_.GetWeakPtr(), remove_all_files,
>           base::Passed(&scoped_closure_runner)),
>       delay);
> 
> Or if you change the prototype of RemoveFiles to take a ScopedClosureRunner,
> then you can just bind the method directly and remove the wrapping lambda.

I agree with your assessment, but I'd like to do this in a separate CL, since
BindBlockArc makes as much sense as BindBlock before this CL.

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/fullscreen_controller.mm (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/fullscreen_controller.mm:199: if (!gEnabledForTests) {
On 2017/04/06 11:23:45, sdefresne wrote:
> Remove braces as the rest of the file does not use them for simple blocks.

Done.

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/preload_controller.h (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/preload_controller.h:54: @property(nonatomic,
unsafe_unretained) id<PreloadControllerDelegate> delegate;
On 2017/04/06 11:23:45, sdefresne wrote:
> Can't this be weak?

Done. Thank you for pointing this out!

[sdefresne, 2017-04-10 09:32:05]

lgtm

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/e...
File ios/chrome/browser/ui/external_file_remover.mm (right):

https://codereview.chromium.org/2804703002/diff/20001/ios/chrome/browser/ui/e...
ios/chrome/browser/ui/external_file_remover.mm:91: web::WebThread::UI,
FROM_HERE, base::BindBlockArc(^{
On 2017/04/07 13:02:09, stkhapugin wrote:
> On 2017/04/06 11:23:45, sdefresne wrote:
> > Followup:
> > 
> > I looks like all objects involved here are C++ object, so there is little
> point
> > using base::BindBlockArc. Can this be instead changed to a base::Bind
instead,
> > like this:
> > 
> >   bool remove_all_files = delay == base::TimeDelta::FromSeconds(0);
> >   base::ScopedClosureRunner scoped_closure_runner(callback);
> >   web::WebThread::PostDelayedTask(
> >       web::WebThread::UI, FROM_HERE,
> >       base::Bind(
> >           [](base::WeakPtr<ExternalFileRemover> remover, bool all_files,
> >              base::ScopedClosureRunner runner) {
> >             DCHECK(remover.get());
> >             remover->RemoveFiles(all_files, runner.Release());
> >           },
> >           weak_ptr_factory_.GetWeakPtr(), remove_all_files,
> >           base::Passed(&scoped_closure_runner)),
> >       delay);
> > 
> > Or if you change the prototype of RemoveFiles to take a ScopedClosureRunner,
> > then you can just bind the method directly and remove the wrapping lambda.
> 
> I agree with your assessment, but I'd like to do this in a separate CL, since
> BindBlockArc makes as much sense as BindBlock before this CL. 

This is why I used "Followup:" to introduce my comment :-)

[stkhapugin, 2017-04-10 11:20:17]

The CQ bit was checked by stkhapugin@chromium.org

[commit-bot: I haz the power, 2017-04-10 11:20:28]

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-04-10 12:31:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491823217658890,
"parent_rev": "e64c5d36227046633521b638ffc2f0b9a903590f", "commit_rev":
"e757b63aa11bc5c4c3c30019c70e285d1266c9c2"}

[commit-bot: I haz the power, 2017-04-10 12:32:39]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/browser/ui:ui_internal_arc to ARC.

Automatically generated ARCMigrate commit
Notable issues:
 * This temporarily splits ui_internal into two targets to simplify review. The
targets
are merged in a dependent CL. 

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

to

==========
[ObjC ARC] Converts ios/chrome/browser/ui:ui_internal_arc to ARC.

Automatically generated ARCMigrate commit
Notable issues:
 * This temporarily splits ui_internal into two targets to simplify review. The
targets
are merged in a dependent CL.

BUG=624363
TEST=None

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

[commit-bot: I haz the power, 2017-04-10 12:32:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e757b63aa11bc5c4c3c30019c70e...