Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2920853006:
Removes usage of signin APIs that return scoped_nsobjects. (Closed)
|
Created:
3 years, 6 months ago by stkhapugin 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoves usage of signin APIs that return scoped_nsobjects.
Returning scoped_nsobjects is against the philosophy of autoreleased
objects being passed around. Refactoring methods and functions vending
scoped_nsobjects allows for a cleaner transition to ARC later.
This is CL 3/5.
BUG=None
TEST=None
Review-Url: https://codereview.chromium.org/2920853006
Cr-Commit-Position: refs/heads/master@{#479691}
Committed: https://chromium.googlesource.com/chromium/src/+/f46f8cdc0ebf789cc6be1ee524955cef93229111
Patch Set 1 #
Total comments: 8
Patch Set 2 : Reparent #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : rebase_fix #
Messages
Total messages: 34 (18 generated)
The CQ bit was checked by stkhapugin@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...
stkhapugin@chromium.org changed reviewers: + msarda@chromium.org
PTAL
stkhapugin@chromium.org changed reviewers: + lpromero@chromium.org
+lpromero for settings OWNER, ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Some comments and nits. https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/signin/chrome_identity_service.h (right): https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/chrome_identity_service.h:12: #include "base/mac/scoped_nsobject.h" Remove this include. https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/chrome_identity_service.h:105: virtual UINavigationController* CreateAccountDetailsController( It is not clear to me how this changes will be rolled by the downstream roller (see comment on downstream CL as well). https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h (right): https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h:10: #include "base/mac/scoped_nsobject.h" Remove this include. https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm (right): https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:161: ChromeIdentityInteractionManager* manager = Why is this change from scoped_nsobject to autorelease needed?
Some comments and nits that are not crucial. I am traveling tomorrow and I think I have already reviewed this very late. I am LGTM-ing this CL to unblock this work. Once you address my comments, feel free to get a second opinion if you think that is necessary.
Description was changed from ========== Cleans up signin provider APIs to not return scoped_nsobjects. Returning scoped_nsobjects is against the philosophy of autoreleased objects being passed around. Refactoring methods and functions vending scoped_nsobjects allows for a cleaner transition to ARC later. BUG=None TEST=None ========== to ========== Removes usage of signin APIs that return scoped_nsobjects. Returning scoped_nsobjects is against the philosophy of autoreleased objects being passed around. Refactoring methods and functions vending scoped_nsobjects allows for a cleaner transition to ARC later. This is CL 3/5. BUG=None TEST=None ==========
https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/signin/chrome_identity_service.h (right): https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/chrome_identity_service.h:12: #include "base/mac/scoped_nsobject.h" On 2017/06/08 23:21:06, msarda wrote: > Remove this include. With 5 CLs required to land this cleanly, this will move to CL #5 that will remove the old API and this include. https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/chrome_identity_service.h:105: virtual UINavigationController* CreateAccountDetailsController( On 2017/06/08 23:21:06, msarda wrote: > It is not clear to me how this changes will be rolled by the downstream roller > (see comment on downstream CL as well). discussed offline, using 5 CLs https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h (right): https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h:10: #include "base/mac/scoped_nsobject.h" On 2017/06/08 23:21:07, msarda wrote: > Remove this include. ditto https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm (right): https://codereview.chromium.org/2920853006/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:161: ChromeIdentityInteractionManager* manager = On 2017/06/08 23:21:07, msarda wrote: > Why is this change from scoped_nsobject to autorelease needed? It is not strictly necessary, but it's equivalent, so I'd keep it if you don't mind.
lgtm https://codereview.chromium.org/2920853006/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm (right): https://codereview.chromium.org/2920853006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:512: UIViewController* accountDetails = Just out of curiosity, was this leaking before?
https://codereview.chromium.org/2920853006/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm (right): https://codereview.chromium.org/2920853006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:512: UIViewController* accountDetails = On 2017/06/13 12:31:07, msarda wrote: > Just out of curiosity, was this leaking before? Seems like it was :)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2920853006/#ps20001 (title: "Reparent")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2920853006/#ps40001 (title: "rebase")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2920853006/#ps60001 (title: "rebase_fix")
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": 1497534473981870,
"parent_rev": "e5a9cd7d13c77e6599a35aaaf62451d1925d7d8b", "commit_rev":
"f46f8cdc0ebf789cc6be1ee524955cef93229111"}
Message was sent while issue was closed.
Description was changed from ========== Removes usage of signin APIs that return scoped_nsobjects. Returning scoped_nsobjects is against the philosophy of autoreleased objects being passed around. Refactoring methods and functions vending scoped_nsobjects allows for a cleaner transition to ARC later. This is CL 3/5. BUG=None TEST=None ========== to ========== Removes usage of signin APIs that return scoped_nsobjects. Returning scoped_nsobjects is against the philosophy of autoreleased objects being passed around. Refactoring methods and functions vending scoped_nsobjects allows for a cleaner transition to ARC later. This is CL 3/5. BUG=None TEST=None Review-Url: https://codereview.chromium.org/2920853006 Cr-Commit-Position: refs/heads/master@{#479691} Committed: https://chromium.googlesource.com/chromium/src/+/f46f8cdc0ebf789cc6be1ee52495... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f46f8cdc0ebf789cc6be1ee52495...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2940063004/ by mrefaat@chromium.org. The reason for reverting is: This broke ios_chrome_settings_egtests AccountCollectionsTest https://uberchromegw.corp.google.com/i/internal.bling.main/builders/ipad9-sim... .
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
Hi. In the future, please associate CLs with multiple steps with a bug number. Reverting this patch broke the waterfall, but it's not easy to find the rest of the CLs that need to be reverted. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
