Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786033002:
Expose way to set google api key through CWVWebView class method. (Closed)
|
Created:
3 years, 8 months ago by jzw1 Modified:
3 years, 8 months ago Reviewers:
Roger Tawa OOO till Jul 10th, Eugene But (OOO till 7-30), sdefresne, ichikawa, michaeldo, Hiroshi Ichikawa CC:
chromium-reviews, Eugene But (OOO till 7-30), ios-reviews+web_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose way to set google api key through CWVWebView class method.
BUG=710370
Review-Url: https://codereview.chromium.org/2786033002
Cr-Commit-Position: refs/heads/master@{#465958}
Committed: https://chromium.googlesource.com/chromium/src/+/2d29208212df8151ec07ade59abfc79cd1ca266a
Patch Set 1 #Patch Set 2 : re-done according to sylvain's suggestion #Patch Set 3 : added comment to static method #
Total comments: 10
Patch Set 4 : updated comments and removed unnecessary checks #Patch Set 5 : Added setters to google_api_keys.cc #Patch Set 6 : remove environment based method. #Patch Set 7 : add some guards so this only works for ios. #
Total comments: 26
Patch Set 8 : Addressed Comments #Patch Set 9 : Addressed more comments #
Total comments: 8
Patch Set 10 : Few lints #
Total comments: 8
Patch Set 11 : addressed michaeldo comments #Patch Set 12 : fix typo #Patch Set 13 : added tests #Patch Set 14 : last patch #
Total comments: 6
Patch Set 15 : updated tests #Patch Set 16 : fix dependency #
Messages
Total messages: 50 (15 generated)
jzw@chromium.org changed reviewers: + eugenebut@chromium.org, ichikawa@google.com, michaeldo@chromium.org
PTAL
ichikawa@chromium.org changed reviewers: + ichikawa@chromium.org
https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:82: environment->UnSetVar("GOOGLE_API_KEY"); I'm a bit confused how this code takes effect, by setting environment variables and unsetting immediately. Maybe calling google_apis::GetAPIKey() etc. is a trick, to let it read environment variables and cache them? It should be commented as such, or maybe how about just setting these environment variables here and leave them, without unsetting them? Why do you need to unset them?
eugenebut@chromium.org changed reviewers: + sdefresne@chromium.org
Maybe Sylvain knows why unsetting environment variables is necessary. https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:64: // This is used by to communicate with the Translate API, among others. Could you please explain in the comments what arguments should be passed. Maybe a link to public Google API doc, where APIKey, clientID and clientSecret are described. https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:65: // This method should be called before any CWVWebViews are instantiated. "before any CWVWebViews are instantiated" or "before a page is loaded"?
https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:82: environment->UnSetVar("GOOGLE_API_KEY"); On 2017/03/31 08:15:26, Hiroshi Ichikawa wrote: > I'm a bit confused how this code takes effect, by setting environment variables > and unsetting immediately. Maybe calling google_apis::GetAPIKey() etc. is a > trick, to let it read environment variables and cache them? > > It should be commented as such, or maybe how about just setting these > environment variables here and leave them, without unsetting them? Why do you > need to unset them? You are on the right track. By calling GetAPIKey, it is fetched and cached. See https://cs.chromium.org/chromium/src/google_apis/google_api_keys.cc?l=292 and APIKeyCache constructor on line 100. I actually think the DCHECK is good here to ensure that the API is working as expected. Maybe leaving them set exposes them to the whole environment longer than necessary, but I agree with Eugene, maybe Sylvain can speak to the reasons to unset them. https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:65: // This method should be called before any CWVWebViews are instantiated. On 2017/03/31 18:15:14, Eugene But wrote: > "before any CWVWebViews are instantiated" or "before a page is loaded"? IIRC this needs to happen before the object is instantiated.
https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:82: environment->UnSetVar("GOOGLE_API_KEY"); Can you move google_apis::GetAPIKey() etc. out of DCHECK_EQ()? If I understand correctly, expressions inside DCHECK_EQ() are not executed in release build. So the current code only works on debug build, not release build. On 2017/03/31 20:28:16, michaeldo wrote: > On 2017/03/31 08:15:26, Hiroshi Ichikawa wrote: > > I'm a bit confused how this code takes effect, by setting environment > variables > > and unsetting immediately. Maybe calling google_apis::GetAPIKey() etc. is a > > trick, to let it read environment variables and cache them? > > > > It should be commented as such, or maybe how about just setting these > > environment variables here and leave them, without unsetting them? Why do you > > need to unset them? > > You are on the right track. By calling GetAPIKey, it is fetched and cached. See > https://cs.chromium.org/chromium/src/google_apis/google_api_keys.cc?l=292 and > APIKeyCache constructor on line 100. I see, thanks. It should be definitely commented as such. It's tricky to rely on such side effect of getter functions. > > I actually think the DCHECK is good here to ensure that the API is working as > expected. Maybe leaving them set exposes them to the whole environment longer > than necessary, but I agree with Eugene, maybe Sylvain can speak to the reasons > to unset them. I feel this kind of DCHECK is more like a role of unit tests. So I slightly prefer to extract these DCHECKs into a unit test, but it's up to you. https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:65: // This method should be called before any CWVWebViews are instantiated. On 2017/03/31 20:28:16, michaeldo wrote: > On 2017/03/31 18:15:14, Eugene But wrote: > > "before any CWVWebViews are instantiated" or "before a page is loaded"? > > IIRC this needs to happen before the object is instantiated. Should it happen even before +[CWV configureWithDelegate:]? If so, it should also be documented.
I just re-read the comments in google_api_keys and I'm not sure we should go this route for the API. It specifies that setting the GOOGLE_DEFAULT_CLIENT_ID and GOOGLE_DEFAULT_CLIENT_SECRET environment variables are only expected to be used for development. https://cs.chromium.org/chromium/src/google_apis/google_api_keys.h?rcl=ca2623...
On 2017/04/03 18:31:44, michaeldo wrote: > I just re-read the comments in google_api_keys and I'm not sure we should go > this route for the API. It specifies that setting the GOOGLE_DEFAULT_CLIENT_ID > and GOOGLE_DEFAULT_CLIENT_SECRET environment variables are only expected to be > used for development. > > https://cs.chromium.org/chromium/src/google_apis/google_api_keys.h?rcl=ca2623... John, have you tried just specifying GOOGLE_API_KEY without GOOGLE_DEFAULT_CLIENT_ID and GOOGLE_DEFAULT_CLIENT_SECRET to see if Translate feature still works?
On 2017/04/04 02:25:42, Hiroshi Ichikawa wrote: > On 2017/04/03 18:31:44, michaeldo wrote: > > I just re-read the comments in google_api_keys and I'm not sure we should go > > this route for the API. It specifies that setting the GOOGLE_DEFAULT_CLIENT_ID > > and GOOGLE_DEFAULT_CLIENT_SECRET environment variables are only expected to be > > used for development. > > > > > https://cs.chromium.org/chromium/src/google_apis/google_api_keys.h?rcl=ca2623... > > John, have you tried just specifying GOOGLE_API_KEY without > GOOGLE_DEFAULT_CLIENT_ID and GOOGLE_DEFAULT_CLIENT_SECRET to see if Translate > feature still works? It still works.
On 2017/04/11 06:30:16, jzw1 wrote: > On 2017/04/04 02:25:42, Hiroshi Ichikawa wrote: > > On 2017/04/03 18:31:44, michaeldo wrote: > > > I just re-read the comments in google_api_keys and I'm not sure we should go > > > this route for the API. It specifies that setting the > GOOGLE_DEFAULT_CLIENT_ID > > > and GOOGLE_DEFAULT_CLIENT_SECRET environment variables are only expected to > be > > > used for development. > > > > > > > > > https://cs.chromium.org/chromium/src/google_apis/google_api_keys.h?rcl=ca2623... > > > > John, have you tried just specifying GOOGLE_API_KEY without > > GOOGLE_DEFAULT_CLIENT_ID and GOOGLE_DEFAULT_CLIENT_SECRET to see if Translate > > feature still works? > > It still works. Thanks for the confirmation. Then looks like it's sufficient to add a setter only for GOOGLE_API_KEY.
https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... ios/web_view/internal/cwv_web_view.mm:82: environment->UnSetVar("GOOGLE_API_KEY"); On 2017/04/03 05:28:16, Hiroshi Ichikawa wrote: > Can you move google_apis::GetAPIKey() etc. out of DCHECK_EQ()? If I understand > correctly, expressions inside DCHECK_EQ() are not executed in release build. So > the current code only works on debug build, not release build. > > On 2017/03/31 20:28:16, michaeldo wrote: > > On 2017/03/31 08:15:26, Hiroshi Ichikawa wrote: > > > I'm a bit confused how this code takes effect, by setting environment > > variables > > > and unsetting immediately. Maybe calling google_apis::GetAPIKey() etc. is a > > > trick, to let it read environment variables and cache them? > > > > > > It should be commented as such, or maybe how about just setting these > > > environment variables here and leave them, without unsetting them? Why do > you > > > need to unset them? > > > > You are on the right track. By calling GetAPIKey, it is fetched and cached. > See > > https://cs.chromium.org/chromium/src/google_apis/google_api_keys.cc?l=292 and > > APIKeyCache constructor on line 100. > > I see, thanks. It should be definitely commented as such. It's tricky to rely on > such side effect of getter functions. > > > > > I actually think the DCHECK is good here to ensure that the API is working as > > expected. Maybe leaving them set exposes them to the whole environment longer > > than necessary, but I agree with Eugene, maybe Sylvain can speak to the > reasons > > to unset them. > > I feel this kind of DCHECK is more like a role of unit tests. So I slightly > prefer to extract these DCHECKs into a unit test, but it's up to you. Yeah I'm OK with removing the DCHECKs and the UnSetVars. https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:64: // This is used by to communicate with the Translate API, among others. On 2017/03/31 18:15:14, Eugene But wrote: > Could you please explain in the comments what arguments should be passed. Maybe > a link to public Google API doc, where APIKey, clientID and clientSecret are > described. Done. https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:65: // This method should be called before any CWVWebViews are instantiated. On 2017/04/03 05:28:16, Hiroshi Ichikawa wrote: > On 2017/03/31 20:28:16, michaeldo wrote: > > On 2017/03/31 18:15:14, Eugene But wrote: > > > "before any CWVWebViews are instantiated" or "before a page is loaded"? > > > > IIRC this needs to happen before the object is instantiated. > > Should it happen even before +[CWV configureWithDelegate:]? If so, it should > also be documented. I'd rather not add "before CWV configureWitDelegate" since we're planning on removing it anyways. It's more accurate to say before page load, but I think it's important to communicate to the user that this method should be called before everything, hence I went with the more conservative "before instantiation".
Description was changed from ========== Expose way to set google api key through CWVWebView class method. BUG= ========== to ========== Expose way to set google api key through CWVWebView class method. BUG=710370 ==========
On 2017/04/11 08:28:46, jzw1 wrote: > https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... > File ios/web_view/internal/cwv_web_view.mm (right): > > https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/internal/c... > ios/web_view/internal/cwv_web_view.mm:82: > environment->UnSetVar("GOOGLE_API_KEY"); > On 2017/04/03 05:28:16, Hiroshi Ichikawa wrote: > > Can you move google_apis::GetAPIKey() etc. out of DCHECK_EQ()? If I understand > > correctly, expressions inside DCHECK_EQ() are not executed in release build. > So > > the current code only works on debug build, not release build. > > > > On 2017/03/31 20:28:16, michaeldo wrote: > > > On 2017/03/31 08:15:26, Hiroshi Ichikawa wrote: > > > > I'm a bit confused how this code takes effect, by setting environment > > > variables > > > > and unsetting immediately. Maybe calling google_apis::GetAPIKey() etc. is > a > > > > trick, to let it read environment variables and cache them? > > > > > > > > It should be commented as such, or maybe how about just setting these > > > > environment variables here and leave them, without unsetting them? Why do > > you > > > > need to unset them? > > > > > > You are on the right track. By calling GetAPIKey, it is fetched and cached. > > See > > > https://cs.chromium.org/chromium/src/google_apis/google_api_keys.cc?l=292 > and > > > APIKeyCache constructor on line 100. > > > > I see, thanks. It should be definitely commented as such. It's tricky to rely > on > > such side effect of getter functions. > > > > > > > > I actually think the DCHECK is good here to ensure that the API is working > as > > > expected. Maybe leaving them set exposes them to the whole environment > longer > > > than necessary, but I agree with Eugene, maybe Sylvain can speak to the > > reasons > > > to unset them. > > > > I feel this kind of DCHECK is more like a role of unit tests. So I slightly > > prefer to extract these DCHECKs into a unit test, but it's up to you. > > Yeah I'm OK with removing the DCHECKs and the UnSetVars. > > https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... > File ios/web_view/public/cwv_web_view.h (right): > > https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... > ios/web_view/public/cwv_web_view.h:64: // This is used by to communicate with > the Translate API, among others. > On 2017/03/31 18:15:14, Eugene But wrote: > > Could you please explain in the comments what arguments should be passed. > Maybe > > a link to public Google API doc, where APIKey, clientID and clientSecret are > > described. > > Done. > > https://codereview.chromium.org/2786033002/diff/40001/ios/web_view/public/cwv... > ios/web_view/public/cwv_web_view.h:65: // This method should be called before > any CWVWebViews are instantiated. > On 2017/04/03 05:28:16, Hiroshi Ichikawa wrote: > > On 2017/03/31 20:28:16, michaeldo wrote: > > > On 2017/03/31 18:15:14, Eugene But wrote: > > > > "before any CWVWebViews are instantiated" or "before a page is loaded"? > > > > > > IIRC this needs to happen before the object is instantiated. > > > > Should it happen even before +[CWV configureWithDelegate:]? If so, it should > > also be documented. > > I'd rather not add "before CWV configureWitDelegate" since we're planning on > removing it anyways. > It's more accurate to say before page load, but I think it's important to > communicate to the user that this method should be called before everything, > hence I went with the more conservative "before instantiation". I added the setters just in case we prefer it.
jzw@chromium.org changed reviewers: + rogerta@chromium.org
PTAL everyone.
https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:209: void set_api_key(std::string api_key) { api_key_ = api_key; } const std::string& Same for below. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:330: #if defined(OS_MACOSX) Is this logic MacOS specific? If so, can you make it an error when this is called on non-Mac, probably with NOTREACHED()? Otherwise, remove this #if. Same for below. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:77: void SetAPIKey(std::string api_key); const std::string& would be better. Same for below. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:77: void SetAPIKey(std::string api_key); Can you document that this must be called only before running any code using the API key? I believe sdefresne@ or someone said that many code assumes the API key is immutable. Same for below. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:104: void SetClientID(std::string client_id, OAuth2Client client); SetOAuth2ClientID would be better to be consistent with other function names around. Same for below.
lgtm https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:330: #if defined(OS_MACOSX) Should this be OS_IOS ? https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:69: std::string client_id = base::SysNSStringToUTF8(clientID); client_id Style is used for C++ code, Objective-C code should follow clientID Style
https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:107: void SetClientSecret(std::string client_secret, OAuth2Client client); As discussed over email, let's put all these SetXXX methods inside #ifdef for iOS only. Nit: could we reverse the order of the two args, since it is a mapping from client --> client_id/client_secret? Let's also rename the setters to be consistent with the getters. So: void SetOAuth2ClientID(...) void SetOAuth2ClientSecret(...) https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:76: } Does this need to loop? In what cases will the ios webview need any client other than CLIENT_MAIN?
https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:330: #if defined(OS_MACOSX) On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > Is this logic MacOS specific? If so, can you make it an error when this is > called on non-Mac, probably with NOTREACHED()? Otherwise, remove this #if. Same > for below. Per the comment in the header, the method definition there and the entire method here should be wrapped with #if defined. So on other platforms, this method shouldn't exist at all. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:344: #if defined(OS_MACOSX) Same comment as SetAPIKey, entire method for SetClientID/Secret signatures should be inside the #if/#endif https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:76: } On 2017/04/13 17:42:43, Roger Tawa wrote: > Does this need to loop? In what cases will the ios webview need any client > other than CLIENT_MAIN? I also think this loop is odd here. (If we do need it, can it be moved to google_api_keys.cc and remove the client param all together?)
https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:209: void set_api_key(std::string api_key) { api_key_ = api_key; } On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > const std::string& > > Same for below. Done. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:330: #if defined(OS_MACOSX) On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > Is this logic MacOS specific? If so, can you make it an error when this is > called on non-Mac, probably with NOTREACHED()? Otherwise, remove this #if. Same > for below. I changed to OS_IOS to be more specific https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:330: #if defined(OS_MACOSX) On 2017/04/13 15:06:25, Eugene But wrote: > Should this be OS_IOS ? Yep. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:330: #if defined(OS_MACOSX) On 2017/04/13 19:28:09, michaeldo wrote: > On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > > Is this logic MacOS specific? If so, can you make it an error when this is > > called on non-Mac, probably with NOTREACHED()? Otherwise, remove this #if. > Same > > for below. > > Per the comment in the header, the method definition there and the entire method > here should be wrapped with #if defined. So on other platforms, this method > shouldn't exist at all. Done. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:344: #if defined(OS_MACOSX) On 2017/04/13 19:28:09, michaeldo wrote: > Same comment as SetAPIKey, entire method for SetClientID/Secret signatures > should be inside the #if/#endif Done. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:77: void SetAPIKey(std::string api_key); On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > Can you document that this must be called only before running any code using the > API key? I believe sdefresne@ or someone said that many code assumes the API key > is immutable. Same for below. Done. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:77: void SetAPIKey(std::string api_key); On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > const std::string& would be better. Same for below. Done. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:104: void SetClientID(std::string client_id, OAuth2Client client); On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > SetOAuth2ClientID would be better to be consistent with other function names > around. Same for below. Done. https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.h:107: void SetClientSecret(std::string client_secret, OAuth2Client client); On 2017/04/13 17:42:43, Roger Tawa wrote: > As discussed over email, let's put all these SetXXX methods inside #ifdef for > iOS only. > > Nit: could we reverse the order of the two args, since it is a mapping from > client --> client_id/client_secret? > > Let's also rename the setters to be consistent with the getters. So: > > void SetOAuth2ClientID(...) > void SetOAuth2ClientSecret(...) Done. https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:69: std::string client_id = base::SysNSStringToUTF8(clientID); On 2017/04/13 15:06:25, Eugene But wrote: > client_id Style is used for C++ code, Objective-C code should follow clientID > Style Any suggestions for what I should call it since clientID is already used by the method parameter? Going with clientIDString for now. https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:76: } On 2017/04/13 19:28:09, michaeldo wrote: > On 2017/04/13 17:42:43, Roger Tawa wrote: > > Does this need to loop? In what cases will the ios webview need any client > > other than CLIENT_MAIN? > > I also think this loop is odd here. (If we do need it, can it be moved to > google_api_keys.cc and remove the client param all together?) Currently there's a piece of translate code: https://cs.chromium.org/chromium/src/components/translate/core/browser/transl... that checks if all the client ids and client secrets are defined. if any of them isn't, it won't proceed.
https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:209: void set_api_key(std::string api_key) { api_key_ = api_key; } On 2017/04/14 02:10:32, jzw1 wrote: > On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > > const std::string& > > > > Same for below. > > Done. Looks like this particular one is not fixed yet. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.cc:219: #if defined(OS_IOS) Put a blank line before this line. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.cc:229: #if defined(OS_IOS) Put a blank line before this line. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.h:107: // Sets the client id for the specified client. Add "This should be called as early as possible..." comment here too. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.h:112: // Sets the client secret for the specified client. Add "This should be called as early as possible..." comment here too.
https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/120001/google_apis/google_api... google_apis/google_api_keys.cc:209: void set_api_key(std::string api_key) { api_key_ = api_key; } On 2017/04/14 02:19:44, Hiroshi Ichikawa wrote: > On 2017/04/14 02:10:32, jzw1 wrote: > > On 2017/04/13 06:42:55, Hiroshi Ichikawa wrote: > > > const std::string& > > > > > > Same for below. > > > > Done. > > Looks like this particular one is not fixed yet. Done. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.cc:219: #if defined(OS_IOS) On 2017/04/14 02:19:44, Hiroshi Ichikawa wrote: > Put a blank line before this line. Done. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.cc:229: #if defined(OS_IOS) On 2017/04/14 02:19:44, Hiroshi Ichikawa wrote: > Put a blank line before this line. Done. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.h:107: // Sets the client id for the specified client. On 2017/04/14 02:19:44, Hiroshi Ichikawa wrote: > Add "This should be called as early as possible..." comment here too. Done. https://codereview.chromium.org/2786033002/diff/160001/google_apis/google_api... google_apis/google_api_keys.h:112: // Sets the client secret for the specified client. On 2017/04/14 02:19:44, Hiroshi Ichikawa wrote: > Add "This should be called as early as possible..." comment here too. Done.
lgtm
https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/120001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:69: std::string client_id = base::SysNSStringToUTF8(clientID); On 2017/04/14 02:10:32, jzw1 wrote: > On 2017/04/13 15:06:25, Eugene But wrote: > > client_id Style is used for C++ code, Objective-C code should follow clientID > > Style > > Any suggestions for what I should call it since clientID is already used by the > method parameter? Going with clientIDString for now. Maybe clientIDNSString? But clientIDString looks fine.
lgtm! Thank you! Please see a few nit comments. https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... google_apis/google_api_keys.cc:357: #if defined(OS_IOS) nit: Put both of these methods inside a single #if defined call? https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... google_apis/google_api_keys.h:78: // API key is even accessed. Currently only allowed on iOS. nit: I don't think we need the "Currently only allowed on iOS." portion of the comment as it is just redundant. The #if defined should be clear enough. Same goes for below. https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... google_apis/google_api_keys.h:111: nit: Put both of these methods inside a single #if defined call? https://codereview.chromium.org/2786033002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/180001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:73: static_cast<google_apis::OAuth2Client>(i); Can we prevent the cast if we do this loop inside of google_api_keys.cc?
https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... File google_apis/google_api_keys.cc (right): https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... google_apis/google_api_keys.cc:357: #if defined(OS_IOS) On 2017/04/14 15:25:55, michaeldo wrote: > nit: Put both of these methods inside a single #if defined call? Done. https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... File google_apis/google_api_keys.h (right): https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... google_apis/google_api_keys.h:78: // API key is even accessed. Currently only allowed on iOS. On 2017/04/14 15:25:55, michaeldo wrote: > nit: I don't think we need the "Currently only allowed on iOS." portion of the > comment as it is just redundant. The #if defined should be clear enough. Same > goes for below. Done. https://codereview.chromium.org/2786033002/diff/180001/google_apis/google_api... google_apis/google_api_keys.h:111: On 2017/04/14 15:25:55, michaeldo wrote: > nit: Put both of these methods inside a single #if defined call? Done. https://codereview.chromium.org/2786033002/diff/180001/ios/web_view/internal/... File ios/web_view/internal/cwv_web_view.mm (right): https://codereview.chromium.org/2786033002/diff/180001/ios/web_view/internal/... ios/web_view/internal/cwv_web_view.mm:73: static_cast<google_apis::OAuth2Client>(i); On 2017/04/14 15:25:55, michaeldo wrote: > Can we prevent the cast if we do this loop inside of google_api_keys.cc? I followed the loop example in google_api_keys.cc:315, so I'm going to guess we can't prevent it?
lgtm Thanks John.
The CQ bit was checked by jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, michaeldo@chromium.org, ichikawa@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2786033002/#ps220001 (title: "fix typo")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
Hey guys. I realized I forgot to write some tests. PTAL. On Wed, Apr 19, 2017 at 4:05 PM commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > ) > ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-... > ) > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui... > ) > 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_... > ) > > https://codereview.chromium.org/2786033002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you for the tests. still lgtm https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... File google_apis/google_api_keys_unittest.cc (right): https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... google_apis/google_api_keys_unittest.cc:484: TEST_F(GoogleAPIKeysTest, OverrideAllKeysUsingSetters) { Could you please add a comment explaining what this test does. https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... google_apis/google_api_keys_unittest.cc:487: std::string api_key("setter-API_KEY"); Would it be better to split this test into multiple tests?
lgtm Agreed, thanks for the tests! https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... File google_apis/google_api_keys_unittest.cc (right): https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... google_apis/google_api_keys_unittest.cc:534: } Doesn't this test need to be inside an #ifdef for iOS?
https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... File google_apis/google_api_keys_unittest.cc (right): https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... google_apis/google_api_keys_unittest.cc:484: TEST_F(GoogleAPIKeysTest, OverrideAllKeysUsingSetters) { On 2017/04/19 16:27:14, Eugene But wrote: > Could you please add a comment explaining what this test does. Done. https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... google_apis/google_api_keys_unittest.cc:487: std::string api_key("setter-API_KEY"); On 2017/04/19 16:27:14, Eugene But wrote: > Would it be better to split this test into multiple tests? I feel like this test is set up in a very similar way to the test above it (override all with env), so at least it is consistent. https://codereview.chromium.org/2786033002/diff/260001/google_apis/google_api... google_apis/google_api_keys_unittest.cc:534: } On 2017/04/19 18:32:20, Roger Tawa wrote: > Doesn't this test need to be inside an #ifdef for iOS? Oh dear god yes. Thanks for catching that.
The CQ bit was checked by jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, ichikawa@chromium.org, rogerta@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2786033002/#ps280001 (title: "updated tests")
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 jzw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, eugenebut@chromium.org, michaeldo@chromium.org, ichikawa@chromium.org Link to the patchset: https://codereview.chromium.org/2786033002/#ps300001 (title: "fix dependency")
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": 300001, "attempt_start_ts": 1492676345556480,
"parent_rev": "a8cb8a5214062b2f48d1c3d198d173cbc5c847ac", "commit_rev":
"2d29208212df8151ec07ade59abfc79cd1ca266a"}
Message was sent while issue was closed.
Description was changed from ========== Expose way to set google api key through CWVWebView class method. BUG=710370 ========== to ========== Expose way to set google api key through CWVWebView class method. BUG=710370 Review-Url: https://codereview.chromium.org/2786033002 Cr-Commit-Position: refs/heads/master@{#465958} Committed: https://chromium.googlesource.com/chromium/src/+/2d29208212df8151ec07ade59abf... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/2d29208212df8151ec07ade59abf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
