Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880373002:
[ObjC ARC] Converts ios/chrome/browser/ui/qr_scanner:eg_tests to ARC. (Closed)
|
Created:
3 years, 7 months ago by baxley Modified:
3 years, 6 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ObjC ARC] Converts ios/chrome/browser/ui/qr_scanner:eg_tests to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Review-Url: https://codereview.chromium.org/2880373002
Cr-Commit-Position: refs/heads/master@{#476665}
Committed: https://chromium.googlesource.com/chromium/src/+/6666e3d76a0f228dc3631411235c6e20adfdc37a
Patch Set 1 #Patch Set 2 : remove comment #
Total comments: 5
Patch Set 3 : rebase #
Messages
Total messages: 30 (19 generated)
The CQ bit was checked by baxley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
baxley@chromium.org changed reviewers: + eugenebut@chromium.org
Just adds the arc configs, and removes a retain. Let me know if you see any issues, thanks!
lgtm
[ObjC ARC] Converts ios/chrome/browser/translate:external_url_eg_tests to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None
The CQ bit was checked by baxley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... File ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm (left): https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm:401: return [cameraControllerMock retain]; So we need to keep the retain count at 1. Current way is crashing, which makes sense after reading the comment. :( I'll take a look at it tomorrow, but Stepan, if you have any quick tips, let me know! Update: I chatted with Eugene and two options are... 1. Wrap it in a method with the prefix new, so a retain is added. 2. Refactor the test Stepan, I don't know the scope of making tests just work, or refactoring them, so if you alternative suggestions I'd love to hear it!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
baxley@chromium.org changed reviewers: + stkhapugin@chromium.org
Actually adding Stepan as a reviewer now.
stkhapugin@chromium.org changed reviewers: + jif@chromium.org
+jif https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... File ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm (left): https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm:401: return [cameraControllerMock retain]; On 2017/05/16 00:51:36, baxley wrote: > So we need to keep the retain count at 1. Current way is crashing, which makes > sense after reading the comment. :( > > I'll take a look at it tomorrow, but Stepan, if you have any quick tips, let me > know! > > Update: I chatted with Eugene and two options are... > 1. Wrap it in a method with the prefix new, so a retain is added. > 2. Refactor the test > > Stepan, I don't know the scope of making tests just work, or refactoring them, > so if you alternative suggestions I'd love to hear it! This is not the first time ARC is blocked with this. +jif who planned to refactor this segment so that it's ARC-compatible.
https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... File ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm (left): https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm:401: return [cameraControllerMock retain]; On 2017/05/30 14:13:56, stkhapugin wrote: > On 2017/05/16 00:51:36, baxley wrote: > > So we need to keep the retain count at 1. Current way is crashing, which makes > > sense after reading the comment. :( > > > > I'll take a look at it tomorrow, but Stepan, if you have any quick tips, let > me > > know! > > > > Update: I chatted with Eugene and two options are... > > 1. Wrap it in a method with the prefix new, so a retain is added. > > 2. Refactor the test > > > > Stepan, I don't know the scope of making tests just work, or refactoring them, > > so if you alternative suggestions I'd love to hear it! > > This is not the first time ARC is blocked with this. > > +jif who planned to refactor this segment so that it's ARC-compatible. Fundamentally, when running tests we need the BVC to instantiate a mock CameraController. This could be done by passing a special BrowserViewControllerDependencyFactory to the BVC. Unfortunately, I did not find where in EG we hook ourself to do that because the BVC is created super early. Eugene's solution (assuming I understand it correctly) is a simpler solution which would finally unblock ARC conversion.
https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... File ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm (left): https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm:401: return [cameraControllerMock retain]; On 2017/05/31 11:32:46, jif wrote: > On 2017/05/30 14:13:56, stkhapugin wrote: > > On 2017/05/16 00:51:36, baxley wrote: > > > So we need to keep the retain count at 1. Current way is crashing, which > makes > > > sense after reading the comment. :( > > > > > > I'll take a look at it tomorrow, but Stepan, if you have any quick tips, let > > me > > > know! > > > > > > Update: I chatted with Eugene and two options are... > > > 1. Wrap it in a method with the prefix new, so a retain is added. > > > 2. Refactor the test > > > > > > Stepan, I don't know the scope of making tests just work, or refactoring > them, > > > so if you alternative suggestions I'd love to hear it! > > > > This is not the first time ARC is blocked with this. > > > > +jif who planned to refactor this segment so that it's ARC-compatible. > > Fundamentally, when running tests we need the BVC to instantiate a mock > CameraController. > This could be done by passing a special BrowserViewControllerDependencyFactory > to the BVC. > Unfortunately, I did not find where in EG we hook ourself to do that because the > BVC is created super early. > > Eugene's solution (assuming I understand it correctly) is a simpler solution > which would finally unblock ARC conversion. Mike, can you try swizzling the implementation with a method, not a block, and call the method newBlah? This might work, but I'm not sure it will. I believe that this should be refactored by injecting a factory to BVC, so can you please file a bug for this? This hack is only acceptable because BVC is so unmaintainable.
https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... File ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm (left): https://codereview.chromium.org/2880373002/diff/20001/ios/chrome/browser/ui/q... ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm:401: return [cameraControllerMock retain]; On 2017/05/31 12:28:40, stkhapugin wrote: > On 2017/05/31 11:32:46, jif wrote: > > On 2017/05/30 14:13:56, stkhapugin wrote: > > > On 2017/05/16 00:51:36, baxley wrote: > > > > So we need to keep the retain count at 1. Current way is crashing, which > > makes > > > > sense after reading the comment. :( > > > > > > > > I'll take a look at it tomorrow, but Stepan, if you have any quick tips, > let > > > me > > > > know! > > > > > > > > Update: I chatted with Eugene and two options are... > > > > 1. Wrap it in a method with the prefix new, so a retain is added. > > > > 2. Refactor the test > > > > > > > > Stepan, I don't know the scope of making tests just work, or refactoring > > them, > > > > so if you alternative suggestions I'd love to hear it! > > > > > > This is not the first time ARC is blocked with this. > > > > > > +jif who planned to refactor this segment so that it's ARC-compatible. > > > > Fundamentally, when running tests we need the BVC to instantiate a mock > > CameraController. > > This could be done by passing a special BrowserViewControllerDependencyFactory > > to the BVC. > > Unfortunately, I did not find where in EG we hook ourself to do that because > the > > BVC is created super early. > > > > Eugene's solution (assuming I understand it correctly) is a simpler solution > > which would finally unblock ARC conversion. > > Mike, can you try swizzling the implementation with a method, not a block, and > call the method newBlah? This might work, but I'm not sure it will. > > I believe that this should be refactored by injecting a factory to BVC, so can > you please file a bug for this? This hack is only acceptable because BVC is so > unmaintainable. Landed the quick fix (https://codereview.chromium.org/2912373002/), so you'll need to rebase.
The CQ bit was checked by baxley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by baxley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2880373002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496417046473930,
"parent_rev": "4053529b6fe405697beaac4ff45f759a61f833c3", "commit_rev":
"6666e3d76a0f228dc3631411235c6e20adfdc37a"}
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts ios/chrome/browser/ui/qr_scanner:eg_tests to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/chrome/browser/ui/qr_scanner:eg_tests to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None Review-Url: https://codereview.chromium.org/2880373002 Cr-Commit-Position: refs/heads/master@{#476665} Committed: https://chromium.googlesource.com/chromium/src/+/6666e3d76a0f228dc3631411235c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6666e3d76a0f228dc3631411235c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
