Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9937004:
Create an abstract flow class that abstracts the following logical steps of calling an OAuth2 enabl… (Closed)
|
Created:
8 years, 8 months ago by Munjal (Google) Modified:
8 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, jstritar, msw Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionCreate an abstract flow class that abstracts the following logical steps of calling an OAuth2 enabled API:
- Try existing access token to call the API.
- If that does not work, generate a new one.
- Try the new access token
This abstract class should be used by any code that calls OAuth2 APIs.
Examples:
- Getting profile picture
- Cloud print
- App notifications
Future CLs coming to replace existing duplicate code in individual places to use this class.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129970
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 40
Messages
Total messages: 11 (0 generated)
Hi Munjal, I like the overall design. Just one suggestion. Could we minimize the number of public/protected methods? It would be better to keep things simple for now and add things only if we need them. https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:56: virtual void OnGetTokenSuccess(const std::string& access_token); can these 3 functions be private? https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:82: virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(); can we make this private? I don't think anyone needs it right now https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:87: virtual content::URLFetcher* CreateURLFetcher(); same, I think this could be private
https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:56: virtual void OnGetTokenSuccess(const std::string& access_token); On 2012/03/30 17:41:42, sail wrote: > can these 3 functions be private? No, they are callback methods that are invoked by another class. So can't be private. https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:82: virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(); On 2012/03/30 17:41:42, sail wrote: > can we make this private? I don't think anyone needs it right now This is protected so that Mock implementation in testing can override it. Same with the one below. https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:87: virtual content::URLFetcher* CreateURLFetcher(); On 2012/03/30 17:41:42, sail wrote: > same, I think this could be private Same comment as above. It is mocked out in testing.
https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:56: virtual void OnGetTokenSuccess(const std::string& access_token); On 2012/03/30 18:05:41, munjal wrote: > On 2012/03/30 17:41:42, sail wrote: > > can these 3 functions be private? > > No, they are callback methods that are invoked by another class. So can't be > private. Even though it's a callback it's ok to make this private. The callback will still work. https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:82: virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(); On 2012/03/30 18:05:41, munjal wrote: > On 2012/03/30 17:41:42, sail wrote: > > can we make this private? I don't think anyone needs it right now > > This is protected so that Mock implementation in testing can override it. Same > with the one below. I think you can mock private functions. See: http://code.google.com/p/googlemock/wiki/CookBook#Mocking_Private_or_Protecte...
https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:56: virtual void OnGetTokenSuccess(const std::string& access_token); On 2012/03/30 18:08:56, sail wrote: > On 2012/03/30 18:05:41, munjal wrote: > > On 2012/03/30 17:41:42, sail wrote: > > > can these 3 functions be private? > > > > No, they are callback methods that are invoked by another class. So can't be > > private. > > Even though it's a callback it's ok to make this private. The callback will > still work. I see. Thanks for that tip. Even though it might work, I tried it and reverted it back to this for two reasons: 1. Other Chrome code that implement callbacks seem to keep the inhertied method pbulic. 2. If I make these private, I have to make the test class a friend. That is not bizarre by itself. But given #1 it felt inappropriate. Let me know if you feel strongly though. I am not adamant about this. https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:82: virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(); On 2012/03/30 18:08:56, sail wrote: > On 2012/03/30 18:05:41, munjal wrote: > > On 2012/03/30 17:41:42, sail wrote: > > > can we make this private? I don't think anyone needs it right now > > > > This is protected so that Mock implementation in testing can override it. Same > > with the one below. > > I think you can mock private functions. See: > http://code.google.com/p/googlemock/wiki/CookBook#Mocking_Private_or_Protecte... Done. https://chromiumcodereview.appspot.com/9937004/diff/1/chrome/common/net/gaia/... chrome/common/net/gaia/oauth2_api_call_flow.h:87: virtual content::URLFetcher* CreateURLFetcher(); On 2012/03/30 17:41:42, sail wrote: > same, I think this could be private Done.
LGTM! Can't wait to start using this!
LGTM https://chromiumcodereview.appspot.com/9937004/diff/4003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/4003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:28: // Given: refresh token, access token, list of scopes an OAuth2 enabled nit: grammar error here. Suggested edit to something like: "Given a refresh token, access token, and list of scopes, an OAuth2 enabled API is called in the following way:"
https://chromiumcodereview.appspot.com/9937004/diff/4003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/4003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:28: // Given: refresh token, access token, list of scopes an OAuth2 enabled On 2012/03/30 20:15:52, Antony Sargent wrote: > nit: grammar error here. Suggested edit to something like: > > "Given a refresh token, access token, and list of scopes, an OAuth2 enabled API > is called in the following way:" Done.
drive by typo nits: https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:96: // Helper to create an instnace of access token fetcher. nit typo: instnace -> instance https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:105: // Helper methods to implement the state machien for the flow. nit typo: machien -> machine https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:117: // Whether we have already tried minting access token once. nit grammar: minting access token once. -> minting an access token.
Interesting, I'm curious to see how much this simplifies the code of its clients. I'll try integrating this flow into ChromeToMobileService for M20. My comments are mostly nits. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.cc (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:18: using content::URLFetcher; The Google C++ style guide discourages use of 'using'. (here through line 22) https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:37: OAuth2ApiCallFlow::~OAuth2ApiCallFlow() { } The Google C++ style guide says "No spaces inside empty braces." https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:49: return; nit: either return here and move the else-case to top level code or drop the superfluous return statement. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:64: ProcessApiCallFailure(source); Should you set "state_ = ERROR_STATE;"? https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:73: ProcessApiCallFailure(source); Should you set "state_ = ERROR_STATE;"? https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:81: ProcessApiCallFailure(source); Should you set "state_ = ERROR_STATE;"? https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:25: // Base calss for all classes that implement a flow to call OAuth2 nit spelling: "class". https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:42: // Note that access_token can be empty. In that case, the flow will skip optional nit: vertical bars around |access_token|. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:69: // Sub-classes can expose appropriate observer interface by implementing nit grammar: "can expose *an* appropriate". https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:81: // The steps this class performs are: This is a duplicated but less informative version of the comments at lines 28-33. Please remove or relate these steps to the State enum type/values. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:98: virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(); nit: Mention that this is virtual just for unit test mocking. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:101: // The URLFether's method will be GET if body is empty, POST otherwise. nit: perhaps rephrase or mention that "body" indicates the value returned by the pure virtual CreateApiCallBody. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:103: virtual content::URLFetcher* CreateURLFetcher(); nit: Mention that this is virtual just for unit test mocking. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:27: using content::URLFetcher; nit: using (i think this is usually ok in test files) https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:35: static GURL CreateApiUrl() { nit: add a blank line above. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:45: } nit: "} // namespace" and add a blank line above. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:97: // MOCK_METHOD0(CreateURLFetcher, URLFetcher* ()); Did you intend to remove this?
Comments addressed in https://chromiumcodereview.appspot.com/9982015 Please take a look. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.cc (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:18: using content::URLFetcher; On 2012/03/30 23:56:32, msw wrote: > The Google C++ style guide discourages use of 'using'. > (here through line 22) I thought that using is fine in .cc files. Not in .h files. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:37: OAuth2ApiCallFlow::~OAuth2ApiCallFlow() { } On 2012/03/30 23:56:32, msw wrote: > The Google C++ style guide says "No spaces inside empty braces." Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:49: return; On 2012/03/30 23:56:32, msw wrote: > nit: either return here and move the else-case to top level code or drop the > superfluous return statement. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:64: ProcessApiCallFailure(source); On 2012/03/30 23:56:32, msw wrote: > Should you set "state_ = ERROR_STATE;"? Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:73: ProcessApiCallFailure(source); On 2012/03/30 23:56:32, msw wrote: > Should you set "state_ = ERROR_STATE;"? Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.cc:81: ProcessApiCallFailure(source); On 2012/03/30 23:56:32, msw wrote: > Should you set "state_ = ERROR_STATE;"? Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow.h (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:25: // Base calss for all classes that implement a flow to call OAuth2 On 2012/03/30 23:56:32, msw wrote: > nit spelling: "class". Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:42: // Note that access_token can be empty. In that case, the flow will skip On 2012/03/30 23:56:32, msw wrote: > optional nit: vertical bars around |access_token|. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:69: // Sub-classes can expose appropriate observer interface by implementing On 2012/03/30 23:56:32, msw wrote: > nit grammar: "can expose *an* appropriate". Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:81: // The steps this class performs are: On 2012/03/30 23:56:32, msw wrote: > This is a duplicated but less informative version of the comments at lines > 28-33. Please remove or relate these steps to the State enum type/values. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:96: // Helper to create an instnace of access token fetcher. On 2012/03/30 22:54:17, Ryan Sleevi wrote: > nit typo: instnace -> instance Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:98: virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(); On 2012/03/30 23:56:32, msw wrote: > nit: Mention that this is virtual just for unit test mocking. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:101: // The URLFether's method will be GET if body is empty, POST otherwise. On 2012/03/30 23:56:32, msw wrote: > nit: perhaps rephrase or mention that "body" indicates the value returned by the > pure virtual CreateApiCallBody. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:103: virtual content::URLFetcher* CreateURLFetcher(); On 2012/03/30 23:56:32, msw wrote: > nit: Mention that this is virtual just for unit test mocking. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:105: // Helper methods to implement the state machien for the flow. On 2012/03/30 22:54:17, Ryan Sleevi wrote: > nit typo: machien -> machine Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow.h:117: // Whether we have already tried minting access token once. On 2012/03/30 22:54:17, Ryan Sleevi wrote: > nit grammar: minting access token once. -> minting an access token. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... File chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc (right): https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:27: using content::URLFetcher; On 2012/03/30 23:56:32, msw wrote: > nit: using (i think this is usually ok in test files) Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:35: static GURL CreateApiUrl() { On 2012/03/30 23:56:32, msw wrote: > nit: add a blank line above. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:45: } On 2012/03/30 23:56:32, msw wrote: > nit: "} // namespace" and add a blank line above. Done. https://chromiumcodereview.appspot.com/9937004/diff/5003/chrome/common/net/ga... chrome/common/net/gaia/oauth2_api_call_flow_unittest.cc:97: // MOCK_METHOD0(CreateURLFetcher, URLFetcher* ()); On 2012/03/30 23:56:32, msw wrote: > Did you intend to remove this? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
