ios/chrome/test/app/DEPS - Issue 2888173002: [ObjC ARC] Converts ios/chrome/test/app:test_support to ARC.

Side by Side Diff: ios/chrome/test/app/DEPS

Issue 2888173002: [ObjC ARC] Converts ios/chrome/test/app:test_support to ARC. (Closed)

Patch Set: fix test and DEPS Created 3 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
(Empty)
	1 include_rules = [

	2 "+ios/web/web_state/ui/crw_web_controller.h",
	baxley 2017/05/17 16:44:35 Eugene, let me know what you think about this. I 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 This is problematic, because tests can now import Show quoted text 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. baxley 2017/05/17 20:21:02 I filed crbug.com/723812 so we can evaluate if we Show quoted text 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 I don't mind this as a temporary fix, but in this Show quoted text 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 Eugene, I followed up with Gregory, and some tests Show quoted text 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 This is example of file-level exception: ios/chrom Show quoted text 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 :( baxley 2017/06/12 20:38:26 Done, thank you for the pointer. Show quoted text 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. Show quoted text > > 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.
	3 ]

OLD	NEW

« no previous file with comments | « ios/chrome/test/app/BUILD.gn ('k') | ios/chrome/test/app/bookmarks_test_util.mm » ('j') | no next file with comments »