New issue
Advanced search Search tips

Issue 738861 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Blocks shouldn't be passed as const-refs in GetAccessToken methods.

Project Member Reported by stkhapugin@chromium.org, Jul 3 2017

Issue description

ChromeIdentityService exposes GetAccessToken APIs that take a const ios::AccessTokenCallback& callback. 
For correct memory management and for enabling ARC compatibility, objc blocks should be passed by pointer. 
 
Fixing this will require multiple CLs:

CL (upstream): add GetAccessTokenArc APIs upstream. Those are exactly like GetAccessToken, but take a pointer to block instead of constref
CL (downstream): implement GetAccessTokenArc APIs
CL (upstream): replace all usage of GetAccessToken with usage of GetAccessTokenArc
CL (downstream): remove implementation of GetAccessToken
CL (upstream): remove declaration of GetAccessToken
CL (upstream): rename GetAccessTokenArc to GetAccessToken
CL (downstream): rename GetAccessTokenArc to GetAccessToken

Alternative solution is to just change the API upstream and roll chromium downstream manually. 

I've talked to downstream ios sheriff, they are ok with me making a manual roll.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/853a46cde856de187f916e7b56734a7bba821b36

commit 853a46cde856de187f916e7b56734a7bba821b36
Author: stkhapugin <stkhapugin@chromium.org>
Date: Mon Jul 03 12:12:36 2017

Change GetAccessToken APIs so that they don't pass const ref to blocks.

Objective-C blocks should not be passed as const ref. This CL replaces
APIs that take const ref to an Obj-C block and instead uses a pointer
to a block. This makes the code compatible with ARC in the future.

BUG= 738861 

Review-Url: https://codereview.chromium.org/2970703002
Cr-Commit-Position: refs/heads/master@{#483992}

[modify] https://crrev.com/853a46cde856de187f916e7b56734a7bba821b36/ios/public/provider/chrome/browser/signin/chrome_identity_service.h
[modify] https://crrev.com/853a46cde856de187f916e7b56734a7bba821b36/ios/public/provider/chrome/browser/signin/chrome_identity_service.mm
[modify] https://crrev.com/853a46cde856de187f916e7b56734a7bba821b36/ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h
[modify] https://crrev.com/853a46cde856de187f916e7b56734a7bba821b36/ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 3 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/ef403cdb9371c1349ab8347fcda405d881ed400e

commit ef403cdb9371c1349ab8347fcda405d881ed400e
Author: stkhapugin <stkhapugin@google.com>
Date: Mon Jul 03 13:39:19 2017

Status: Fixed (was: Started)

Sign in to add a comment