[ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

[ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

Also convert ios/chrome/browser/ui/sync:eg_tests to ARC, since this
simplifies use of output parameters in sync_test_util.mm.

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

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

[baxley, 2017-05-17 15:40:43]

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

[commit-bot: I haz the power, 2017-05-17 15:41:32]

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, 2017-05-17 15:52:20]

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

[commit-bot: I haz the power, 2017-05-17 15:52:21]

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

[baxley, 2017-05-17 15:58:56]

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

[commit-bot: I haz the power, 2017-05-17 15:59:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[baxley, 2017-05-17 16:41:29]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

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

to

==========
[ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

Also convert ios/chrome/browser/ui/sync:eg_tests to ARC, since this
simplifies use of output parameters in sync_test_util.mm.

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

[baxley, 2017-05-17 16:44:35]

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

[baxley, 2017-05-17 16:44:35]

Let me know what you think. Here are the notable changes I did in addition to
running the conversion script...

1. Combined ios/chrome/browser/ui/sync:eg_tests into this CL since this is the
only caller of sync_test_util, which includes an output parameter.

2. Added DEPS exclusion for ios/web/web_state/ui/crw_web_controller.h, which was
already used in the test, but a forward declaration worked pre-ARC.

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
Eugene, let me know what you think about this.

I didn't change the test to use any different APIs, ARC just requires includes
instead of forward declarations when it needs to know memory semantics of the
selector. (last part of sentence was from Stepan's doc :) )

[Eugene But (OOO till 7-30), 2017-05-17 18:28:44]

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
On 2017/05/17 16:44:35, baxley wrote:
> Eugene, let me know what you think about this.
> 
> I didn't change the test to use any different APIs, ARC just requires includes
> instead of forward declarations when it needs to know memory semantics of the
> selector. (last part of sentence was from Stepan's doc :) )
This is problematic, because tests can now import private API, which they could
not before. |handleLowMemory| is unused in our app and we should not add this
exception just because we want to call API that we don't even need to call.

[commit-bot: I haz the power, 2017-05-17 19:10:56]

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

[commit-bot: I haz the power, 2017-05-17 19:10:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[baxley, 2017-05-17 20:21:02]

I'll follow up with gchatz@ when they're back to see if we need these tests or
if we need to do some refactoring so we can limit the scope of the include to
just one test file.

Thanks!

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
On 2017/05/17 18:28:44, Eugene But wrote:
> On 2017/05/17 16:44:35, baxley wrote:
> > Eugene, let me know what you think about this.
> > 
> > I didn't change the test to use any different APIs, ARC just requires
includes
> > instead of forward declarations when it needs to know memory semantics of
the
> > selector. (last part of sentence was from Stepan's doc :) )
> This is problematic, because tests can now import private API, which they
could
> not before. |handleLowMemory| is unused in our app and we should not add this
> exception just because we want to call API that we don't even need to call.

I filed crbug.com/723812 so we can evaluate if we need these tests.

If we need these tests, then we should split out this method into a separate
file (maybe moving into tab recorder directory), so we can unblock the ARC
conversion.

[stkhapugin, 2017-05-30 17:16:53]

lgtm

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
On 2017/05/17 20:21:02, baxley wrote:
> On 2017/05/17 18:28:44, Eugene But wrote:
> > On 2017/05/17 16:44:35, baxley wrote:
> > > Eugene, let me know what you think about this.
> > > 
> > > I didn't change the test to use any different APIs, ARC just requires
> includes
> > > instead of forward declarations when it needs to know memory semantics of
> the
> > > selector. (last part of sentence was from Stepan's doc :) )
> > This is problematic, because tests can now import private API, which they
> could
> > not before. |handleLowMemory| is unused in our app and we should not add
this
> > exception just because we want to call API that we don't even need to call.
> 
> I filed crbug.com/723812 so we can evaluate if we need these tests.
> 
> If we need these tests, then we should split out this method into a separate
> file (maybe moving into tab recorder directory), so we can unblock the ARC
> conversion.

I don't mind this as a temporary fix, but in this case please add a TODO above
include_rules with the bug tracking the removal of this.

[baxley, 2017-06-09 17:00:32]

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
On 2017/05/30 17:16:53, stkhapugin wrote:
> On 2017/05/17 20:21:02, baxley wrote:
> > On 2017/05/17 18:28:44, Eugene But wrote:
> > > On 2017/05/17 16:44:35, baxley wrote:
> > > > Eugene, let me know what you think about this.
> > > > 
> > > > I didn't change the test to use any different APIs, ARC just requires
> > includes
> > > > instead of forward declarations when it needs to know memory semantics
of
> > the
> > > > selector. (last part of sentence was from Stepan's doc :) )
> > > This is problematic, because tests can now import private API, which they
> > could
> > > not before. |handleLowMemory| is unused in our app and we should not add
> this
> > > exception just because we want to call API that we don't even need to
call.
> > 
> > I filed crbug.com/723812 so we can evaluate if we need these tests.
> > 
> > If we need these tests, then we should split out this method into a separate
> > file (maybe moving into tab recorder directory), so we can unblock the ARC
> > conversion.
> 
> I don't mind this as a temporary fix, but in this case please add a TODO above
> include_rules with the bug tracking the removal of this. 

Eugene, I followed up with Gregory, and some tests can be removed, but not
enough to remove this functionality. However, the helper to evict tabs should
probably be updated, since the current implementation doesn't really work. You
can see discussion in:
https://bugs.chromium.org/p/chromium/issues/detail?id=723812

I didn't see a way to make a DEPS exclusion for a file, it may have to be
directory based (I saw a document mention that, but I don't know if there are
other tricks).

What do you think about doing the following to unblock this ARC conversion...
1. File bug to remove this DEPS exception and add a TODO here.
2. File bug to update the tab eviction code (even if we do it a different way,
I'm afraid it will still rely upon a private web API, unless we can make
something public).

I hate to leave TODOs, so if you have ideas of someone who can fix the issues
I'm open. But I also don't want to block some ARC conversions. Or if you feel
strongly maybe you can take this over when it is your turn to do a conversion?
:)

[Eugene But (OOO till 7-30), 2017-06-12 03:53:55]

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
On 2017/06/09 17:00:32, baxley wrote:
> On 2017/05/30 17:16:53, stkhapugin wrote:
> > On 2017/05/17 20:21:02, baxley wrote:
> > > On 2017/05/17 18:28:44, Eugene But wrote:
> > > > On 2017/05/17 16:44:35, baxley wrote:
> > > > > Eugene, let me know what you think about this.
> > > > > 
> > > > > I didn't change the test to use any different APIs, ARC just requires
> > > includes
> > > > > instead of forward declarations when it needs to know memory semantics
> of
> > > the
> > > > > selector. (last part of sentence was from Stepan's doc :) )
> > > > This is problematic, because tests can now import private API, which
they
> > > could
> > > > not before. |handleLowMemory| is unused in our app and we should not add
> > this
> > > > exception just because we want to call API that we don't even need to
> call.
> > > 
> > > I filed crbug.com/723812 so we can evaluate if we need these tests.
> > > 
> > > If we need these tests, then we should split out this method into a
separate
> > > file (maybe moving into tab recorder directory), so we can unblock the ARC
> > > conversion.
> > 
> > I don't mind this as a temporary fix, but in this case please add a TODO
above
> > include_rules with the bug tracking the removal of this. 
> 
> Eugene, I followed up with Gregory, and some tests can be removed, but not
> enough to remove this functionality. However, the helper to evict tabs should
> probably be updated, since the current implementation doesn't really work. You
> can see discussion in:
> https://bugs.chromium.org/p/chromium/issues/detail?id=723812
> 
> I didn't see a way to make a DEPS exclusion for a file, it may have to be
> directory based (I saw a document mention that, but I don't know if there are
> other tricks).
> 
> What do you think about doing the following to unblock this ARC conversion...
> 1. File bug to remove this DEPS exception and add a TODO here.
> 2. File bug to update the tab eviction code (even if we do it a different way,
> I'm afraid it will still rely upon a private web API, unless we can make
> something public).
> 
> I hate to leave TODOs, so if you have ideas of someone who can fix the issues
> I'm open. But I also don't want to block some ARC conversions. Or if you feel
> strongly maybe you can take this over when it is your turn to do a conversion?
> :)
This is example of file-level exception: ios/chrome/browser/tabs/DEPS
I think it's fine to add exception to allow WebController import only for one
small file, but not for the whole directory.

I can take over this specific test in July if that's fine with you. I will
probably throw it away and write something else, because as it stands, this test
does not seem to be useful :(

[stkhapugin, 2017-06-12 12:23:34]

Baxley, can you remove the test for now and let Eugene write a new one in July
so that you can push this? :)

[baxley, 2017-06-12 14:24:53]

On 2017/06/12 12:23:34, stkhapugin wrote:
> Baxley, can you remove the test for now and let Eugene write a new one in July
> so that you can push this? :)

Yes, I will land it today (either as is, or deleted). There are several tests
that rely upon this, so it would involve remove multiple tests. I'll either
disable it and remove the dependency, or land with a DEPS allowance. I'll chat
with Eugene. Thanks!

[baxley, 2017-06-12 20:38:26]

I made the DEPS exception for just one file. PTAL!

If LG, feel free to send to CQ, otherwise I'll check it out first thing
tomorrow.

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEPS
File ios/chrome/test/app/DEPS (right):

https://codereview.chromium.org/2888173002/diff/20001/ios/chrome/test/app/DEP...
ios/chrome/test/app/DEPS:2: "+ios/web/web_state/ui/crw_web_controller.h",
On 2017/06/12 03:53:55, Eugene But (OOO till June 11) wrote:
> On 2017/06/09 17:00:32, baxley wrote:
> > On 2017/05/30 17:16:53, stkhapugin wrote:
> > > On 2017/05/17 20:21:02, baxley wrote:
> > > > On 2017/05/17 18:28:44, Eugene But wrote:
> > > > > On 2017/05/17 16:44:35, baxley wrote:
> > > > > > Eugene, let me know what you think about this.
> > > > > > 
> > > > > > I didn't change the test to use any different APIs, ARC just
requires
> > > > includes
> > > > > > instead of forward declarations when it needs to know memory
semantics
> > of
> > > > the
> > > > > > selector. (last part of sentence was from Stepan's doc :) )
> > > > > This is problematic, because tests can now import private API, which
> they
> > > > could
> > > > > not before. |handleLowMemory| is unused in our app and we should not
add
> > > this
> > > > > exception just because we want to call API that we don't even need to
> > call.
> > > > 
> > > > I filed crbug.com/723812 so we can evaluate if we need these tests.
> > > > 
> > > > If we need these tests, then we should split out this method into a
> separate
> > > > file (maybe moving into tab recorder directory), so we can unblock the
ARC
> > > > conversion.
> > > 
> > > I don't mind this as a temporary fix, but in this case please add a TODO
> above
> > > include_rules with the bug tracking the removal of this. 
> > 
> > Eugene, I followed up with Gregory, and some tests can be removed, but not
> > enough to remove this functionality. However, the helper to evict tabs
should
> > probably be updated, since the current implementation doesn't really work.
You
> > can see discussion in:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=723812
> > 
> > I didn't see a way to make a DEPS exclusion for a file, it may have to be
> > directory based (I saw a document mention that, but I don't know if there
are
> > other tricks).
> > 
> > What do you think about doing the following to unblock this ARC
conversion...
> > 1. File bug to remove this DEPS exception and add a TODO here.
> > 2. File bug to update the tab eviction code (even if we do it a different
way,
> > I'm afraid it will still rely upon a private web API, unless we can make
> > something public).
> > 
> > I hate to leave TODOs, so if you have ideas of someone who can fix the
issues
> > I'm open. But I also don't want to block some ARC conversions. Or if you
feel
> > strongly maybe you can take this over when it is your turn to do a
conversion?
> > :)
> This is example of file-level exception: ios/chrome/browser/tabs/DEPS
> I think it's fine to add exception to allow WebController import only for one
> small file, but not for the whole directory.
Done, thank you for the pointer.
> 
> I can take over this specific test in July if that's fine with you. I will
> probably throw it away and write something else, because as it stands, this
test
> does not seem to be useful :(
I talked with gchatz, and they thought there was still some value with the
tests. Even if they're doing things how they shouldn't, it is still providing
some level of integration coverage. Whether it's enough value to run the tests
is another discussion.

[Eugene But (OOO till 7-30), 2017-06-12 23:14:13]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-06-12 23:14:13]

lgtm

[Eugene But (OOO till 7-30), 2017-06-12 23:14:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org
Link to the patchset: https://codereview.chromium.org/2888173002/#ps40001
(title: "tighten deps exclusion")

[commit-bot: I haz the power, 2017-06-12 23:15:03]

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-06-13 00:48:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497309253003520,
"parent_rev": "3d6f939efc594429f36fa7ce9d2e2ebb97d34e01", "commit_rev":
"c3cb3cf56a018f1b82a8e091f81147340dc5762c"}

[commit-bot: I haz the power, 2017-06-13 00:48:41]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

Also convert ios/chrome/browser/ui/sync:eg_tests to ARC, since this
simplifies use of output parameters in sync_test_util.mm.

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

to

==========
[ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

Also convert ios/chrome/browser/ui/sync:eg_tests to ARC, since this
simplifies use of output parameters in sync_test_util.mm.

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

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

[commit-bot: I haz the power, 2017-06-13 00:48:42]

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