Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10836182:
Obfuscated Gaia ID fetcher (Closed)
|
Created:
8 years, 4 months ago by petewil Modified:
8 years, 4 months ago Reviewers:
Pete Williamson, Mihai Parparita -not on Chrome, Munjal (Google), Ben Goodger (Google), dcheng CC:
chromium-reviews, Aaron Boodman, cbentzel+watch_chromium.org, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionObfuscated Gaia ID fetcher
As part of our Push Messaging API, we need to fetch the Obfuscated GAIA ID from the server. This code fetches the Obfuscated GAIA ID, and uses it to construct a channel-id which is used in the push messaging process. The Push Messaging client hands the channel ID to their push messaging server, which uses it when they communicate with the Google back end for push messaging.
BUG=139658
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153108
Patch Set 1 #
Total comments: 71
Patch Set 2 : Fixed fetch by adding scopes #Patch Set 3 : CR changes per Munjal #
Total comments: 24
Patch Set 4 : Implemented getChannelId function #Patch Set 5 : Fix unit tests, style fixes. #Patch Set 6 : CR style changes per Munjal #Patch Set 7 : rebase to master #
Total comments: 127
Patch Set 8 : CR changes per Munjal and DCheng #Patch Set 9 : CR changes per Munjal and DCheng #
Total comments: 56
Patch Set 10 : Style changes per DCheng and Munjal #
Total comments: 29
Patch Set 11 : More style fixes per Munjal and DCheng #
Total comments: 8
Patch Set 12 : More CR changes per DCheng #
Total comments: 18
Patch Set 13 : CR fixes per Mihai #Messages
Total messages: 31 (0 generated)
https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:5: #include "obfuscated_gaia_id_fetcher.h" You need to specify the path relative to src here. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:20: const char kID[] = "id"; Put this inside anonymous namespace. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:22: namespace { Nit: add an empty line here? https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:24: // (here misnamed channel ID). Nit: Drop "This is the". Also, may be make it a bit more succint: "URL of the service to get obfuscated gaia id" https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: // It doesn't seem to get referenced anywhere - what would I need it for? Remove this comment. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:81: } access_token -> obfuscated_id in this method. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:90: LOG(INFO) << "*** ObfuscatedGaiaIdFetcher::CreateApiCallUrl called ***"; Remove LOG statements here and elsewhere in the file. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:95: // encodes all the proper arguments. Take this comment inside the method. Also remove extra spaces between // and "encodes" on second line. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:102: // On subsequent calls, we want to process the ChannelID. I don't understand this comment. Remove it? https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:126: // TODO: what do I need to do here, if anything? Do nothing here right now. Just add a comment that: "We generate a new access token every time instead of storing the access token since access token expire every hour and we expect to get obfuscated gaia id very infrequently." https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:133: // TODO: what do I need to do here, if anything? ReportFailure here. This means we failed to generate the token needed to call the API. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:9: #include <vector> It seems that you are not using vector in the header. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:33: // to get an obfuscated channel ID. The comment is a bit confusing. Let us just say: "Fetches obfuscated gaia id of the Google Account that is logged in to Chrome" https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:41: const std::string& eid); We currently don't use extension id. I think we should remove that. Also, the refresh token passed in here is always the oauth2 login refresh token retrieved from TokenService. I think we should direclty just get it from TokenService. If we do both, we can remove Parameters completely https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:49: // the operation succeeds or fails. Pass your delegate in to May be just say: "Delegate interface that is called when obfuscated gaia id fetch succeeds or fails." https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:51: // want to be notified when a result arrives. Remove second sentence of the comment (i.e. "Pass your delegate...". https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:55: const std::string& access_token) {} This callback should pass back the obfuscated id, not an access token. So rename the parameter to obfuscated_id or such. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:63: ObfuscatedGaiaIdFetcher(net::URLRequestContextGetter* context, If we remove Parameters, we will probably need to pass in Profile* instead of URLRequestContextGetter* so that we can get TokenService instance for that profile. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:77: const std::string& GetCachedObfuscatedGaiaId() { return obfuscated_gaia_id_; } I suggest we remove these two methods from here and keep this class purely an abstraction of calling the API to get the obfuscated id. We should create another class which is a ProfileKeyedService class that caches obfuscated gaia id - lazily or otherwise - and that class can be used by consumers who want obfuscated gaia id. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:99: void ReportSuccess(const std::string& access_token); Parameter name should be obfuscated_id or such. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:104: const std::string& data, std::string* channel_id); Nit: channel_id -> obfuscated_id? https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:10: #include "stub_url_fetcher.h" Include paths should be relative to src/ https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:17: const std::string fakeBadData3("{ 0.5 }"); Put all of these as static const char[] in anonymous namespace. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:19: net::URLFetcher* fakeSource; Do not use non-const global variables. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:21: // delegate class for catching notifications from teh ObfuscatedGaiaIdFetcher Nit teh -> the https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:22: class FetcherDelegate : public ObfuscatedGaiaIdFetcher::Delegate { Name this TestDelegate and put it in an anonymous namespace. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:61: badFetcher3_ = new net::StubURLFetcher(fakeBadData3, true); It is better for each individual test to create their own fetchers. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:67: // TODO: use a scoped pointer to release these at dtor time You missed doing this TODO. I think it is anyway better to define all of this localy in individual tests. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:81: ASSERT_TRUE(channelIdOut == "My-channel-id" && ret); Split the compound check into multiple asserts so that you get better output when the test fails. So do ASSERT_EQ("My-channel-id", channelIdOut); ASSERT_FALSE(ret); Same with rest of the tests. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:82: channelIdOut.clear(); I generally don't link reusing the variables like this. If you forget to clear, some tests might silently pass when they should fail. It is very hard to catch such false positivies. use a new scope to define fresh variables every time. So something like: { // Good response string. std::string channelIdOut; ASSERT_TRUE(fetcher->ParseResponse(fakeGoodData, &channelIdOut)); ASSERT_EQ("My-channel-id", channelIdOut); } { // Invalid JSON. std::string channelIdOut; ASSERT_FALSE(fetcher->ParseResponse(fakeBadData1, &channelIdOut)); ASSERT_EQ("", channelIdOut); } https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:83: ret = fetcher->ParseResponse(fakeBadData1, &channelIdOut); Either replace fakeData1 with "I am not a JSON string" or name fakeData1 something better for readability of tests. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:97: delegate_.Clear(); Create fetcher and delegate locally to eliminate the chance of false positives (and false negatives). https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:106: fetcher->ProcessApiCallSuccess(badFetcher1_); Since you tested ParseResponse thoroughly with each different type of bad response, it is redundant to test all those combinations here also. Just test with one bad response. Also, there are other failure cases - when the API call request fails (i.e. when the base class fails and calls ProcessApiCallFailure). https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:51: }; This seems like temporary code? https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:84: } Seems like all changes ot this file are done for trying out things. So not reviewing them. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h:16: class StubURLFetcher : public URLFetcher { What do you need this class for? If for testing, there are already quite a few classes like FakeURLFetcher, TestURLFetcher ,etc. that you can use in testing. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/common/net/gaia... File chrome/common/net/gaia/oauth2_api_call_flow.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/common/net/gaia... chrome/common/net/gaia/oauth2_api_call_flow.cc:56: LOG(INFO) << "*** OAuth2ApiCallFlow::BeginApiCall called ***"; Remove this (revert this file).
Answers to Munjal's CR comments. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:5: #include "obfuscated_gaia_id_fetcher.h" On 2012/08/09 21:44:29, munjal wrote: > You need to specify the path relative to src here. Done https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:20: const char kID[] = "id"; On 2012/08/09 21:44:29, munjal wrote: > Put this inside anonymous namespace. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:22: namespace { On 2012/08/09 21:44:29, munjal wrote: > Nit: add an empty line here? Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:24: // (here misnamed channel ID). On 2012/08/09 21:44:29, munjal wrote: > Nit: Drop "This is the". Also, may be make it a bit more succint: "URL of the > service to get obfuscated gaia id" Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: // It doesn't seem to get referenced anywhere - what would I need it for? On 2012/08/09 21:44:29, munjal wrote: > Remove this comment. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:81: } On 2012/08/09 21:44:29, munjal wrote: > access_token -> obfuscated_id in this method. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:90: LOG(INFO) << "*** ObfuscatedGaiaIdFetcher::CreateApiCallUrl called ***"; On 2012/08/09 21:44:29, munjal wrote: > Remove LOG statements here and elsewhere in the file. All marked for removal, I'll actually remove them after merging with DCheng's changes and before checkin. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:95: // encodes all the proper arguments. On 2012/08/09 21:44:29, munjal wrote: > Take this comment inside the method. Also remove extra spaces between // and > "encodes" on second line. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:102: // On subsequent calls, we want to process the ChannelID. On 2012/08/09 21:44:29, munjal wrote: > I don't understand this comment. Remove it? Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:126: // TODO: what do I need to do here, if anything? On 2012/08/09 21:44:29, munjal wrote: > Do nothing here right now. Just add a comment that: > "We generate a new access token every time instead of storing the access token > since access token expire every hour and we expect to get obfuscated gaia id > very infrequently." Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:133: // TODO: what do I need to do here, if anything? On 2012/08/09 21:44:29, munjal wrote: > ReportFailure here. This means we failed to generate the token needed to call > the API. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:9: #include <vector> On 2012/08/09 21:44:29, munjal wrote: > It seems that you are not using vector in the header. Now we need it, left in place https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:33: // to get an obfuscated channel ID. On 2012/08/09 21:44:29, munjal wrote: > The comment is a bit confusing. Let us just say: > "Fetches obfuscated gaia id of the Google Account that is logged in to Chrome" Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:41: const std::string& eid); On 2012/08/09 21:44:29, munjal wrote: > We currently don't use extension id. I think we should remove that. > > Also, the refresh token passed in here is always the oauth2 login refresh token > retrieved from TokenService. I think we should direclty just get it from > TokenService. > > If we do both, we can remove Parameters completely To avoid creating more dependencies, and leave my class easy to test, I've replaced the parameters, but in its place I now pass in the refresh token. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:49: // the operation succeeds or fails. Pass your delegate in to On 2012/08/09 21:44:29, munjal wrote: > May be just say: > "Delegate interface that is called when obfuscated gaia id fetch succeeds or > fails." Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:63: ObfuscatedGaiaIdFetcher(net::URLRequestContextGetter* context, On 2012/08/09 21:44:29, munjal wrote: > If we remove Parameters, we will probably need to pass in Profile* instead of > URLRequestContextGetter* so that we can get TokenService instance for that > profile. We don't need a Profile* if we pass in the refresh token https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:77: const std::string& GetCachedObfuscatedGaiaId() { return obfuscated_gaia_id_; } On 2012/08/09 21:44:29, munjal wrote: > I suggest we remove these two methods from here and keep this class purely an > abstraction of calling the API to get the obfuscated id. > > We should create another class which is a ProfileKeyedService class that caches > obfuscated gaia id - lazily or otherwise - and that class can be used by > consumers who want obfuscated gaia id. marked TODO for now. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:99: void ReportSuccess(const std::string& access_token); On 2012/08/09 21:44:29, munjal wrote: > Parameter name should be obfuscated_id or such. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:104: const std::string& data, std::string* channel_id); On 2012/08/09 21:44:29, munjal wrote: > Nit: channel_id -> obfuscated_id? Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:10: #include "stub_url_fetcher.h" On 2012/08/09 21:44:29, munjal wrote: > Include paths should be relative to src/ Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:17: const std::string fakeBadData3("{ 0.5 }"); On 2012/08/09 21:44:29, munjal wrote: > Put all of these as static const char[] in anonymous namespace. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:19: net::URLFetcher* fakeSource; On 2012/08/09 21:44:29, munjal wrote: > Do not use non-const global variables. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:21: // delegate class for catching notifications from teh ObfuscatedGaiaIdFetcher On 2012/08/09 21:44:29, munjal wrote: > Nit teh -> the Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:22: class FetcherDelegate : public ObfuscatedGaiaIdFetcher::Delegate { On 2012/08/09 21:44:29, munjal wrote: > Name this TestDelegate and put it in an anonymous namespace. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:61: badFetcher3_ = new net::StubURLFetcher(fakeBadData3, true); On 2012/08/09 21:44:29, munjal wrote: > It is better for each individual test to create their own fetchers. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:67: // TODO: use a scoped pointer to release these at dtor time On 2012/08/09 21:44:29, munjal wrote: > You missed doing this TODO. I think it is anyway better to define all of this > localy in individual tests. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:81: ASSERT_TRUE(channelIdOut == "My-channel-id" && ret); On 2012/08/09 21:44:29, munjal wrote: > Split the compound check into multiple asserts so that you get better output > when the test fails. > So do > ASSERT_EQ("My-channel-id", channelIdOut); > ASSERT_FALSE(ret); > > Same with rest of the tests. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:82: channelIdOut.clear(); On 2012/08/09 21:44:29, munjal wrote: > I generally don't link reusing the variables like this. If you forget to clear, > some tests might silently pass when they should fail. It is very hard to catch > such false positivies. > > use a new scope to define fresh variables every time. So something like: > { // Good response string. > std::string channelIdOut; > ASSERT_TRUE(fetcher->ParseResponse(fakeGoodData, &channelIdOut)); > ASSERT_EQ("My-channel-id", channelIdOut); > } > { // Invalid JSON. > std::string channelIdOut; > ASSERT_FALSE(fetcher->ParseResponse(fakeBadData1, &channelIdOut)); > ASSERT_EQ("", channelIdOut); > } Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:83: ret = fetcher->ParseResponse(fakeBadData1, &channelIdOut); On 2012/08/09 21:44:29, munjal wrote: > Either replace fakeData1 with "I am not a JSON string" or name fakeData1 > something better for readability of tests. Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:97: delegate_.Clear(); On 2012/08/09 21:44:29, munjal wrote: > Create fetcher and delegate locally to eliminate the chance of false positives > (and false negatives). Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:106: fetcher->ProcessApiCallSuccess(badFetcher1_); On 2012/08/09 21:44:29, munjal wrote: > Since you tested ParseResponse thoroughly with each different type of bad > response, it is redundant to test all those combinations here also. Just test > with one bad response. > > Also, there are other failure cases - when the API call request fails (i.e. when > the base class fails and calls ProcessApiCallFailure). Done. https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h:16: class StubURLFetcher : public URLFetcher { On 2012/08/09 21:44:29, munjal wrote: > What do you need this class for? If for testing, there are already quite a few > classes like FakeURLFetcher, TestURLFetcher ,etc. that you can use in testing. I took a look at FakeURLFetcher and TestURLFetcher. Both seem to require that you stub out the factory instead of the fetcher, and both seem to require an additional thread to run on. I think that the test code is cleaner with this mock of the URL Fetcher, but I could be convinced otherwise. Are there any issues with threading that I run into if I use FakeURLFetcher? https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/common/net/gaia... File chrome/common/net/gaia/oauth2_api_call_flow.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/common/net/gaia... chrome/common/net/gaia/oauth2_api_call_flow.cc:56: LOG(INFO) << "*** OAuth2ApiCallFlow::BeginApiCall called ***"; On 2012/08/09 21:44:29, munjal wrote: > Remove this (revert this file). Done.
This is now working, and ready for review. It still needs to be integrated with Daniel's changes that are already out for review. I plan to try to add a top level unit test right away while this is going through review (see stub in push_apitests.cc), but hold off on the ProfileKeyedService for caching for a little while, and wait for security analysis before further obfuscating the channel ID. All of Munjal's issues from previous code review are addressed. Unit tests pass. In addition, I have written an extension test to call the API, and it returns the expected result.
There are quite a few style issues. I did not cite all instances, but I tried to cite one instance of each type of issue. Please fix all. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:50: const std::string& refreshToken, refresh_token https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:51: const std::vector<std::string>& scopes); No need to take in scopes as an argument. The fetcher should know the scope. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:55: // that lazily caches the ID I suggest just remove these two methods until we do the ProfileKeyedService. Until then we can call the server sdie API always. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:65: // { return obfuscated_gaia_id_; } I suggest we remove this method. But if you don't, then style issue: { goes on previous line and } goes on a line by itself. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:17: const char nonDictionaryJsonData[] = "{ 0.5 }"; Style issues in naming the constants. Use kGoodData etc. Also make these static const. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:38: } It is best to not reuse instances of TestDelegate. Instead of caling Clear and reusing it, create a new one. Otherwise, the likelihood of false negative, or worse false positive, in a unit test is much more. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:47: } // namespace anonymous Nit: remove "anonymous" https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:51: TEST( ObfuscatedGaiaIdFetcherTest, ParseResponse) { Nit: remove space after ( https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:56: std::string channelIdOut4; style issues in naming varibles. Use underscores. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:68: ret = fetcher.ParseResponse(badJsonData, &channelIdOut2); Do not reuse "ret" variable. Instead add a new scope to separate out tests. That way the chance of false negative and false positive is low. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:110: } // namespace extensions Nit: style issue - two spaces after } https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:63: "https://www.googleapis.com/auth/chromewebstore.notification"); Put this scope in ObfuscatedGaiaIdFetcher class instead of requiring to pass it in. Please use a constant to define the scope. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:99: } // namespace extensions This file has a bunch of LOG statements which are probbly there for debugging purposes. Please remove them.
CR changes addressed, fixes uploaded https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:50: const std::string& refreshToken, On 2012/08/15 20:15:44, munjal wrote: > refresh_token Done. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:51: const std::vector<std::string>& scopes); On 2012/08/15 20:15:44, munjal wrote: > No need to take in scopes as an argument. The fetcher should know the scope. That's a bit difficult in this case because we pass the scope to the base class. If you think it is better, I can contain the base class instead of inheriting from it to make it possible to encapsulate the scope https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:55: // that lazily caches the ID On 2012/08/15 20:15:44, munjal wrote: > I suggest just remove these two methods until we do the ProfileKeyedService. > Until then we can call the server sdie API always. Done. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:65: // { return obfuscated_gaia_id_; } On 2012/08/15 20:15:44, munjal wrote: > I suggest we remove this method. But if you don't, then style issue: { goes on > previous line and } goes on a line by itself. I just removed them, they were only still in the code to make sure you were OK with removing them for now and maybe doing this later. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:17: const char nonDictionaryJsonData[] = "{ 0.5 }"; On 2012/08/15 20:15:44, munjal wrote: > Style issues in naming the constants. Use kGoodData etc. Also make these static > const. Done. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:38: } On 2012/08/15 20:15:44, munjal wrote: > It is best to not reuse instances of TestDelegate. Instead of caling Clear and > reusing it, create a new one. Otherwise, the likelihood of false negative, or > worse false positive, in a unit test is much more. Removed the clear method (it is already not being used) https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:47: } // namespace anonymous On 2012/08/15 20:15:44, munjal wrote: > Nit: remove "anonymous" Done. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:51: TEST( ObfuscatedGaiaIdFetcherTest, ParseResponse) { On 2012/08/15 20:15:44, munjal wrote: > Nit: remove space after ( Done. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:56: std::string channelIdOut4; On 2012/08/15 20:15:44, munjal wrote: > style issues in naming varibles. Use underscores. Done. https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:68: ret = fetcher.ParseResponse(badJsonData, &channelIdOut2); On 2012/08/15 20:15:44, munjal wrote: > Do not reuse "ret" variable. Instead add a new scope to separate out tests. That > way the chance of false negative and false positive is low. Fixed by using 4 separate variables (I think it is more readable than scopes) https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:99: } // namespace extensions On 2012/08/15 20:15:44, munjal wrote: > This file has a bunch of LOG statements which are probbly there for debugging > purposes. Please remove them. Hmm, this looks like an older version of the file. All the log statements are already gone from my version, I'll make sure my cl upload includes the latest.
I haven't taken a detailed look at the entire patch. I'll take a closer look when I'm back tomorrow. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:106: Extra new line. With 80 chars, vertical space is a premium. =) https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:108: // This also holds a reference on this class to keep it alive for a callback. These two lines (and several other places) just describe what the code does. Instead of describing what code does, it is preferable to describe why (if needed).
https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:22: const char kID[] = "id"; static. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:26: namespace { Combine this anonymous namespace with the previous one. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: if (scopes.size() == 0) { Why would the size of the vector not be 0? It is a local variable. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: delete this; We should definitely avoid this. Let us just define a delegate interface and let the extension function do the right thing in terms of lifetime management. here is another "delete this;" in ProcessApiCallFailure. We should remove both. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:133: Nit: remove empty line. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:142: } // namespace extensions Nit: two spaces after } https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:16: #include "chrome/browser/extensions/api/push_messaging/push_messaging_api.h" Include ordering is wrong. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:39: class PushMessagingGetChannelIdFunction; I am not sure I understand the situation here. If we define a delegate interface, and make PushMessagingGetChannelIdFunction implement that delegate, why doesn't it work? We should not try to make the Delegate class ref counted. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:45: Nit: remove this empty line. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:47: Nit: remove this empty line. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:50: delegate, We should try our best to remove this dependency. The fetcher class should not need to know about the extension function that uses it. In other words, any class should be able to use this fetcher by becoming a delegate, not just extension function. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:53: We need a Start() method. It is not a good idea to start fetching in constructor. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:58: Nit: remove this blank line since the next few methods are also template methods in OAuth2ApiCallFlow https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:10: #include "chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h" I think I commented about this before. There are already a few URL fetcher classes for testing purposes. Let us not create our own. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:41: virtual ~TestDelegate() {} Indentation. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:54: std::string channel_id_out4; I generalyl prefer separate scopes since even if you have different variables, you could forget to use the right one. Having a different scope means compiler catches all errors for us and compiler makes sure we start with a clean state. But I leave it to you to decide. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:81: TestDelegate* delegate = new TestDelegate(); Let us not use bare pointers. Use scoped_ptr or scoped_refptr. Here and everywhere. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:90: delegate->AddRef(); Again, don't reuse delegates, or any such objects, across tests. It is very easy to introduce a false positive or other bugs. Just declare a new delegate to use for different tests. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:58: Nit: remove unnecessary empty lines. Here and every where. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:66: void PushMessagingGetChannelIdFunction::RespondOnUIThread( Isn't the ObfuscatedGaiaIdFetcher already running on UI thread (and it takes care of doing the fetch on IO thread)? In fact, IIRC, all fetcher / flow classes call back the delegate on the original thread the fetchers were constructed. So I don't think we need to post task on UI thread to report a result. We should already be caleld back on the UI thread. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:72: std::string channel_id = gaia_id + "." + extension_id(); NIt: Use StringPrintf? https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:91: Profile* profile) { You don't need to pass the profile here since it is an instance method of an extension function. IT can get the profile itself. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:95: const std::string refresh_token = const std::string& refresh_token https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:105: const std::string& gaia_id) { Indentation: 4 spaces https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:115: nit: remove empty line. Same below. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:48: class PushMessagingGetChannelIdFunction : public AsyncExtensionFunction { This class should implement the delegate interface of ObfuscatedGaiaIdFetcher instead of ObfuscatedGaiaIdFetcher being aware of this class. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:49: public: Indentation is off. Should be one space for public:, private: etc. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:53: virtual void OnObfuscatedGaiaIdFetchFailure( Move these methods to private once they are in the delegate class. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:46: // } Please remove commented code. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h:6: #ifndef CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_STUB_URL_FETCHER_H_ No need ot write our own stub URL fetcher. Use TestURLFetcher or FakeURLFetcher.
You probably want to take a look at your rebase locally. There are some odd files (such as Android stuff) in the newest version of your patch. I will keep thinking about proper lifetime management for the ExtensionFunction implementation and ObfuscatedGaiaIdFetcher. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:22: const char kID[] = "id"; This name is pretty undescriptive for a top-level constant. You can rename it... or since this constant is only used in one place, you can just move it into the function where it's used (I would probably do the latter here). http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:54: : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()), std::string() instead of "". http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:54: : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()), I'm not a fan of Scopes(), but I guess there's no great solution here since we always pass in the same thing. Scopes() shouldn't be a member function though. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: if (scopes.size() == 0) { Prefer empty() over size() == 0 http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:71: if (delegate_) Why do we allow null delegates? http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:96: source->GetResponseAsString(&response_body); CHECK(source->GetResponseAsString(&response_body)) perhaps? http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: delete this; On 2012/08/15 23:04:19, munjal wrote: > We should definitely avoid this. Let us just define a delegate interface and let > the extension function do the right thing in terms of lifetime management. > > here is another "delete this;" in ProcessApiCallFailure. We should remove both. The problem is this: The extension function (logically) ought to "own" its corresponding instance of ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to keep a ref count to PushMessagingGetChannelIdFunction to keep it alive (since we reply asynchronously). We need to break the cycle somehow or find another way to fix the ownership model here. I've been thinking about this problem and I have not yet come to a great solution. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:138: DictionaryValue* dict = static_cast<DictionaryValue*>(value.get()); Instead of value->GetType() != base::Value::TYPE_DICTIONARY, just use base::Value::GetAsDictionary. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:19: class ObfuscatedGaiaIdFetchTest; This forward declare is unnecessary. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:55: // Implementation of template methods in OAuth2ApiCallFlow. Nit: These aren't templated, so "template methods" is a little inaccurate. Normally I'd just say "// OAuth2ApiCallFlow implementation." I'd also group all the methods from the same interface together with no new lines as munjal has suggested. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:68: friend class ObfuscatedGaiaIdFetcherTest; This line is unnecessary. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:77: static std::vector<std::string> Scopes(); This method name should probably have a verb in it, if you choose to keep it. I would prefer to pass in scopes as an argument though, since passing it in this way looks clunky (in my opinion). http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:43: bool failed_; DISALLOW_COPY_AND_ASSIGN. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:54: std::string channel_id_out4; I don't care about scoped vs non-scoped. However, please move variables nearer to where they are used. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:55: TestDelegate* delegate = new TestDelegate(); Do we leak this? In general, all "new" allocations should be wrapped in a smart pointer. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:61: ASSERT_EQ(channel_id_out1, "My-channel-id"); Prefer EXPECT instead of ASSERT. ASSERT() should be used for things where a test cannot possibly continue (for example, you call Foo::CreateInstance() and CreateInstance() fails for whatever reason). ASSERT terminates the current test while EXPECT will mark a failure but allow the rest of the test case to run, which is typically what you want. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:71: ASSERT_NE(channel_id_out3, "My-channel-id"); If channel_id_out3 is supposed to be empty, then we should say EXPECT_TRUE(channel_id_out3.empty()); http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:12: #include "chrome/browser/profiles/profile_manager.h" Remove this include since it's not used. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:16: #include "chrome/common/net/gaia/google_service_auth_error.h" Remove this include as it's not used. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:72: std::string channel_id = gaia_id + "." + extension_id(); Shouldn't channel ID be empty if gaia_id is empty? http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: DictionaryValue* result = new DictionaryValue(); scoped_ptr http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:83: // SetResult() takes ownership of result. If you use scoped_ptr, this comment won't be needed. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:126: this, "", error.state())); std::string() instead of "" http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:5: // This file includes the JavaScript interface for Push Messaging Unnecessary comment. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:47: // that we use to build the channel ID. I would probably leave out this comment as it merely describes what a reader can see by looking at line 61 and line 67.
http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: delete this; On 2012/08/16 19:12:43, dcheng wrote: > On 2012/08/15 23:04:19, munjal wrote: > > We should definitely avoid this. Let us just define a delegate interface and > let > > the extension function do the right thing in terms of lifetime management. > > > > here is another "delete this;" in ProcessApiCallFailure. We should remove > both. > > The problem is this: > The extension function (logically) ought to "own" its corresponding instance of > ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to keep a > ref count to PushMessagingGetChannelIdFunction to keep it alive (since we reply > asynchronously). We need to break the cycle somehow or find another way to fix > the ownership model here. I've been thinking about this problem and I have not > yet come to a great solution. The ExtensionFunction should AddRef itself instead of relying on this class to add a reference. That is what all extension functions do.
On 2012/08/16 21:45:12, munjal wrote: > http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... > File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc > (right): > > http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... > chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: > delete this; > On 2012/08/16 19:12:43, dcheng wrote: > > On 2012/08/15 23:04:19, munjal wrote: > > > We should definitely avoid this. Let us just define a delegate interface and > > let > > > the extension function do the right thing in terms of lifetime management. > > > > > > here is another "delete this;" in ProcessApiCallFailure. We should remove > > both. > > > > The problem is this: > > The extension function (logically) ought to "own" its corresponding instance > of > > ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to keep a > > ref count to PushMessagingGetChannelIdFunction to keep it alive (since we > reply > > asynchronously). We need to break the cycle somehow or find another way to fix > > the ownership model here. I've been thinking about this problem and I have not > > yet come to a great solution. > > The ExtensionFunction should AddRef itself instead of relying on this class to > add a reference. That is what all extension functions do. Generally speaking, I feel manual refcounting is error prone and something we should try to avoid if possible. I took a quick look at the places that use AddRef()/Release() pairs--and indeed, a lot of them have no choice, because they use NotificationObserver::Observe. However, for this particular case, we can just pass a callback to the fetcher. When ObfuscatedGaiaIdFetcher has finished it's work, it can call back to the ExtensionFunction implementation with base::ResetAndReturn(&callback_)->Run(status, gaia_id).
On Thu, Aug 16, 2012 at 5:18 PM, <dcheng@chromium.org> wrote: > On 2012/08/16 21:45:12, munjal wrote: > > http://codereview.chromium.**org/10836182/diff/10004/** > chrome/browser/extensions/api/**push_messaging/obfuscated_** > gaia_id_fetcher.cc<http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc> > >> File >> > chrome/browser/extensions/api/**push_messaging/obfuscated_** > gaia_id_fetcher.cc > >> (right): >> > > > http://codereview.chromium.**org/10836182/diff/10004/** > chrome/browser/extensions/api/**push_messaging/obfuscated_** > gaia_id_fetcher.cc#newcode105<http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc#newcode105> > > chrome/browser/extensions/api/**push_messaging/obfuscated_** > gaia_id_fetcher.cc:105: > >> delete this; >> On 2012/08/16 19:12:43, dcheng wrote: >> > On 2012/08/15 23:04:19, munjal wrote: >> > > We should definitely avoid this. Let us just define a delegate >> interface >> > and > >> > let >> > > the extension function do the right thing in terms of lifetime >> management. >> > > >> > > here is another "delete this;" in ProcessApiCallFailure. We should >> remove >> > both. >> > >> > The problem is this: >> > The extension function (logically) ought to "own" its corresponding >> instance >> of >> > ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to >> keep >> > a > >> > ref count to PushMessagingGetChannelIdFunct**ion to keep it alive >> (since we >> reply >> > asynchronously). We need to break the cycle somehow or find another way >> to >> > fix > >> > the ownership model here. I've been thinking about this problem and I >> have >> > not > >> > yet come to a great solution. >> > > The ExtensionFunction should AddRef itself instead of relying on this >> class to >> add a reference. That is what all extension functions do. >> > > Generally speaking, I feel manual refcounting is error prone and something > we > should try to avoid if possible. I took a quick look at the places that use > AddRef()/Release() pairs--and indeed, a lot of them have no choice, > because they > use NotificationObserver::Observe. > > We discussed this in person, repeating here for Pete: Lot of extension functions do AddRef and Release on themselves. This is an extremely common usage across extension functions. So I think one good way to do this (which will also be consistent with other extension functions) is to define a delegate interface in ObfuscatedGaiaIdFetcher, make the ExtensionFunction implement that interface. Then ExtensionFunction can AddRef before it calls Start() on the fetcher and Release() whenever it replies back to the extension. This is fine IMO since ExtensionFunction is in the best position to figure out when it needs to be alive and when it is not needed. > However, for this particular case, we can just pass a callback to the > fetcher. > When ObfuscatedGaiaIdFetcher has finished it's work, it can call back to > the > ExtensionFunction implementation with > base::ResetAndReturn(&**callback_)->Run(status, gaia_id). > > > > http://codereview.chromium.**org/10836182/<http://codereview.chromium.org/108... >
This should address all but one or two of the issues so far, I'll discuss the remaining issues in person. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:22: const char kID[] = "id"; On 2012/08/16 19:12:43, dcheng wrote: > This name is pretty undescriptive for a top-level constant. You can rename it... > or since this constant is only used in one place, you can just move it into the > function where it's used (I would probably do the latter here). Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:26: namespace { On 2012/08/15 23:04:19, munjal wrote: > Combine this anonymous namespace with the previous one. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:54: : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()), On 2012/08/16 19:12:43, dcheng wrote: > std::string() instead of "". Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:54: : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()), On 2012/08/16 19:12:43, dcheng wrote: > I'm not a fan of Scopes(), but I guess there's no great solution here since we > always pass in the same thing. > > Scopes() shouldn't be a member function though. Munjal suggested that it be a member function because it is an important implementation detail of the class, and I agree. I'm happy to change it to be a non-member function if you and Munjal both prefer. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: if (scopes.size() == 0) { On 2012/08/15 23:04:19, munjal wrote: > Why would the size of the vector not be 0? It is a local variable. It is always 0, this is left over from when it was a member variable. Check removed. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: if (scopes.size() == 0) { On 2012/08/16 19:12:43, dcheng wrote: > Prefer empty() over size() == 0 check removed instead. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:71: if (delegate_) On 2012/08/16 19:12:43, dcheng wrote: > Why do we allow null delegates? It is because we are using the delegate to hold it alive, and the delegate needs to be a pointer instead of a ref so that it is deleteable. We'll revisit this when we revisit the addref strategy as a comment below suggests. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:96: source->GetResponseAsString(&response_body); On 2012/08/16 19:12:43, dcheng wrote: > CHECK(source->GetResponseAsString(&response_body)) perhaps? Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: delete this; On 2012/08/16 21:45:12, munjal wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > On 2012/08/15 23:04:19, munjal wrote: > > > We should definitely avoid this. Let us just define a delegate interface and > > let > > > the extension function do the right thing in terms of lifetime management. > > > > > > here is another "delete this;" in ProcessApiCallFailure. We should remove > > both. > > > > The problem is this: > > The extension function (logically) ought to "own" its corresponding instance > of > > ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to keep a > > ref count to PushMessagingGetChannelIdFunction to keep it alive (since we > reply > > asynchronously). We need to break the cycle somehow or find another way to fix > > the ownership model here. I've been thinking about this problem and I have not > > yet come to a great solution. > > The ExtensionFunction should AddRef itself instead of relying on this class to > add a reference. That is what all extension functions do. Daniel explicitly requested that I not call AddRef explicitly, and that my code should never do it. I'm happy with either solution, I'll try to get a 5 minute chat with both of you so we can pick the best way to solve this issue. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:133: On 2012/08/15 23:04:19, munjal wrote: > Nit: remove empty line. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:138: DictionaryValue* dict = static_cast<DictionaryValue*>(value.get()); On 2012/08/16 19:12:43, dcheng wrote: > Instead of value->GetType() != base::Value::TYPE_DICTIONARY, just use > base::Value::GetAsDictionary. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:142: } // namespace extensions On 2012/08/15 23:04:19, munjal wrote: > Nit: two spaces after } Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:16: #include "chrome/browser/extensions/api/push_messaging/push_messaging_api.h" On 2012/08/15 23:04:19, munjal wrote: > Include ordering is wrong. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:19: class ObfuscatedGaiaIdFetchTest; On 2012/08/16 19:12:43, dcheng wrote: > This forward declare is unnecessary. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:45: On 2012/08/15 23:04:19, munjal wrote: > Nit: remove this empty line. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:47: On 2012/08/15 23:04:19, munjal wrote: > Nit: remove this empty line. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:53: On 2012/08/15 23:04:19, munjal wrote: > We need a Start() method. It is not a good idea to start fetching in > constructor. We do have a start method, it is inherited from OAuth2ApiCallFlow http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:55: // Implementation of template methods in OAuth2ApiCallFlow. On 2012/08/16 19:12:43, dcheng wrote: > Nit: These aren't templated, so "template methods" is a little inaccurate. > Normally I'd just say "// OAuth2ApiCallFlow implementation." I'd also group all > the methods from the same interface together with no new lines as munjal has > suggested. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:58: On 2012/08/15 23:04:19, munjal wrote: > Nit: remove this blank line since the next few methods are also template methods > in OAuth2ApiCallFlow Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:68: friend class ObfuscatedGaiaIdFetcherTest; On 2012/08/16 19:12:43, dcheng wrote: > This line is unnecessary. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:10: #include "chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h" On 2012/08/15 23:04:19, munjal wrote: > I think I commented about this before. There are already a few URL fetcher > classes for testing purposes. Let us not create our own. I replied to your comment before with some reasons why I though having the stub_url_fetcher class was a good idea. I'll discuss this in person with you. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:41: virtual ~TestDelegate() {} On 2012/08/15 23:04:19, munjal wrote: > Indentation. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:43: bool failed_; On 2012/08/16 19:12:43, dcheng wrote: > DISALLOW_COPY_AND_ASSIGN. Done, but why? It seems like this class is not in any danger of being copied, and since it is in a private namespace already, it is not in any danger of being used in other files. Does the extra safety really justify the extra code complexity (admittedly minor) of adding this line, or is this a coding standard that I missed reading somewhere? http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:54: std::string channel_id_out4; On 2012/08/15 23:04:19, munjal wrote: > I generalyl prefer separate scopes since even if you have different variables, > you could forget to use the right one. Having a different scope means compiler > catches all errors for us and compiler makes sure we start with a clean state. > > But I leave it to you to decide. I generally prefer not to use scopes unless the code absolutely requires it, since when I read code, every time I see a new scope, it is a speed bump to my understanding the code as I need to wonder why it is there, so I prefer to make the tests independent through other means. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:54: std::string channel_id_out4; On 2012/08/16 19:12:43, dcheng wrote: > I don't care about scoped vs non-scoped. However, please move variables nearer > to where they are used. Done, but why? These unit tests are really fast already, and further micro-optimization won't gain us much. Do you think it makes the code more readable to have variables closer to the point of use instead of at the top? (It's OK if you do, I'm just trying to understand the reason behind the suggestion.) Is it just a matter of chrome coding style to declare variables close to use always? http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:55: TestDelegate* delegate = new TestDelegate(); On 2012/08/16 19:12:43, dcheng wrote: > Do we leak this? In general, all "new" allocations should be wrapped in a smart > pointer. Nope, it deletes itself with the current design. If we keep that design, I'll add a comment here (and in the other tests where we allocate it). http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:61: ASSERT_EQ(channel_id_out1, "My-channel-id"); On 2012/08/16 19:12:43, dcheng wrote: > Prefer EXPECT instead of ASSERT. ASSERT() should be used for things where a test > cannot possibly continue (for example, you call Foo::CreateInstance() and > CreateInstance() fails for whatever reason). > > ASSERT terminates the current test while EXPECT will mark a failure but allow > the rest of the test case to run, which is typically what you want. Done, changed all ASSERTs in this file to EXPECT. FYI - I've actually received the opposite guidance in the past, but I'm happy with either way, and I'll be happy to use EXPECT if that is the way we do it here. (The theory is that unit tests are important, and you shouldn't keep running them if one fails until that one is fixed. The counter-argument goes that you sometimes fix more than one failing test per run. In practice, I find that I normally only fix one anyway, so it doesn't make so much difference to me.) http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:81: TestDelegate* delegate = new TestDelegate(); On 2012/08/15 23:04:19, munjal wrote: > Let us not use bare pointers. Use scoped_ptr or scoped_refptr. Here and > everywhere. As noted earlier, this is an artifact of using a self deleting reference. I'll fix this when you, Daniel, and I agree on the best approach. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:90: delegate->AddRef(); On 2012/08/15 23:04:19, munjal wrote: > Again, don't reuse delegates, or any such objects, across tests. It is very easy > to introduce a false positive or other bugs. Just declare a new delegate to use > for different tests. We aren't re-using the delegate here. This is to keep the delegate alive during the test, due to the current design of self deleting delegates that I will talk to You and Daniel about in person. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:12: #include "chrome/browser/profiles/profile_manager.h" On 2012/08/16 19:12:43, dcheng wrote: > Remove this include since it's not used. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:16: #include "chrome/common/net/gaia/google_service_auth_error.h" On 2012/08/16 19:12:43, dcheng wrote: > Remove this include as it's not used. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:58: On 2012/08/15 23:04:19, munjal wrote: > Nit: remove unnecessary empty lines. Here and every where. Done, but you and I have different ideas of where they are unnecessary. Please let me know if I missed any next time around, since I am still internalizing the coding standards. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:72: std::string channel_id = gaia_id + "." + extension_id(); On 2012/08/16 19:12:43, dcheng wrote: > Shouldn't channel ID be empty if gaia_id is empty? Good catch! Added check for empty http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:72: std::string channel_id = gaia_id + "." + extension_id(); On 2012/08/15 23:04:19, munjal wrote: > NIt: Use StringPrintf? I apologize for the pushback, but why? (While I'm definitely willing to do it, I want to understand the reason first.) The code as is is clear and simple, and it seems that StringPrintf is in a separate library, which complicates code construction. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: DictionaryValue* result = new DictionaryValue(); On 2012/08/16 19:12:43, dcheng wrote: > scoped_ptr I tried originally, and that failed to properly transfer ownership. Reading up on scoped pointers, they are supposed to be used only if the lifetime of the object matches the lifetime of the function - in this case, the result must outlast the lifetime of the function. Perhaps I could use some unique_ptr and a swap idiom, but the ExtensionFunction is not using that so I'd have to change the contract of ExtensionFunction::SetResult everywhere to use it.. I could use auto_ptr with release instead, but auto_ptr seems to be a no-no in our code base, and not a recommended practice any more. I'm definitely open to more suggestions... http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:83: // SetResult() takes ownership of result. On 2012/08/16 19:12:43, dcheng wrote: > If you use scoped_ptr, this comment won't be needed. scoped_ptr doesn't work here because the object needs to outlast the lifetime of this function. I could use uniqe_ptr instead if ExtensionFunction::SetResult used it, but they don't. I could use auto_ptr and do a release, but I've heard that auto_ptr is a no-no in our code base. I clarified the comment instead of changing the code. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:91: Profile* profile) { On 2012/08/15 23:04:19, munjal wrote: > You don't need to pass the profile here since it is an instance method of an > extension function. IT can get the profile itself. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:95: const std::string refresh_token = On 2012/08/15 23:04:19, munjal wrote: > const std::string& refresh_token Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:105: const std::string& gaia_id) { On 2012/08/15 23:04:19, munjal wrote: > Indentation: 4 spaces Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:106: On 2012/08/15 22:20:45, dcheng wrote: > Extra new line. With 80 chars, vertical space is a premium. =) Done, but I'm not sure that I agree in principle. Sure, it is nice to see more code vertically, but if extra whitespace makes the code more readable, I see it as an overall maintanability win, even if you can fit fewer lines onscreen. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:108: // This also holds a reference on this class to keep it alive for a callback. On 2012/08/15 22:20:45, dcheng wrote: > These two lines (and several other places) just describe what the code does. > Instead of describing what code does, it is preferable to describe why (if > needed). Comment changed, see if the new one is better. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:115: On 2012/08/15 23:04:19, munjal wrote: > nit: remove empty line. Same below. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:126: this, "", error.state())); On 2012/08/16 19:12:43, dcheng wrote: > std::string() instead of "" Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:5: // This file includes the JavaScript interface for Push Messaging On 2012/08/16 19:12:43, dcheng wrote: > Unnecessary comment. Done. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:47: // that we use to build the channel ID. On 2012/08/16 19:12:43, dcheng wrote: > I would probably leave out this comment as it merely describes what a reader can > see by looking at line 61 and line 67. Sure, I'll do that, but I have a question on Philosophy - I use comments to quickly skim what a code is supposed to be doing when I do maintenance. Is the way we do things at Google and on Chrome to not comment what the code does? Personally, I think it makes the code better and more readable, but I will absolutely go along with the way we do things here. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:48: class PushMessagingGetChannelIdFunction : public AsyncExtensionFunction { On 2012/08/15 23:04:19, munjal wrote: > This class should implement the delegate interface of ObfuscatedGaiaIdFetcher > instead of ObfuscatedGaiaIdFetcher being aware of this class. My first implementation did that, but after a discussion with Daniel, we refactored the code to the current way so that I would not need to call AddRef. After we discuss this in person, I'll fix this to do whatever we decide is the best way. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:49: public: On 2012/08/15 23:04:19, munjal wrote: > Indentation is off. Should be one space for public:, private: etc. Oops, my bad, I let the editor indent "public:" for me. Fixed. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:53: virtual void OnObfuscatedGaiaIdFetchFailure( On 2012/08/15 23:04:19, munjal wrote: > Move these methods to private once they are in the delegate class. Noted. Will fix when we decide the right approach http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:46: // } On 2012/08/15 23:04:19, munjal wrote: > Please remove commented code. Done.
WRT lifetime, use AddRef()/Release() as suggested by Munjal. I talked with him on Friday, and while I don't completely agree in this instance, I understand his reasoning. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:54: : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()), On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > I'm not a fan of Scopes(), but I guess there's no great solution here since we > > always pass in the same thing. > > > > Scopes() shouldn't be a member function though. > > Munjal suggested that it be a member function because it is an important > implementation detail of the class, and I agree. I'm happy to change it to be a > non-member function if you and Munjal both prefer. It is an implementation detail. In fact, it's so much of one that it can live entirely in the .cc =) http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:43: bool failed_; On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > DISALLOW_COPY_AND_ASSIGN. > > Done, but why? It seems like this class is not in any danger of being copied, > and since it is in a private namespace already, it is not in any danger of being > used in other files. Does the extra safety really justify the extra code > complexity (admittedly minor) of adding this line, or is this a coding standard > that I missed reading somewhere? Unless a class needs to be copyable or assignable, the style convention is to explicitly disallow it. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:54: std::string channel_id_out4; On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > I don't care about scoped vs non-scoped. However, please move variables nearer > > to where they are used. > > Done, but why? These unit tests are really fast already, and further > micro-optimization won't gain us much. Do you think it makes the code more > readable to have variables closer to the point of use instead of at the top? > (It's OK if you do, I'm just trying to understand the reason behind the > suggestion.) Is it just a matter of chrome coding style to declare variables > close to use always? This isn't for optimization reasons, just readability. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:61: ASSERT_EQ(channel_id_out1, "My-channel-id"); On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > Prefer EXPECT instead of ASSERT. ASSERT() should be used for things where a > test > > cannot possibly continue (for example, you call Foo::CreateInstance() and > > CreateInstance() fails for whatever reason). > > > > ASSERT terminates the current test while EXPECT will mark a failure but allow > > the rest of the test case to run, which is typically what you want. > > Done, changed all ASSERTs in this file to EXPECT. FYI - I've actually received > the opposite guidance in the past, but I'm happy with either way, and I'll be > happy to use EXPECT if that is the way we do it here. (The theory is that unit > tests are important, and you shouldn't keep running them if one fails until that > one is fixed. The counter-argument goes that you sometimes fix more than one > failing test per run. In practice, I find that I normally only fix one anyway, > so it doesn't make so much difference to me.) This is just the general convention with googletest: http://googletesting.blogspot.com/2008/07/tott-expect-vs-assert.html http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:72: std::string channel_id = gaia_id + "." + extension_id(); On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/15 23:04:19, munjal wrote: > > NIt: Use StringPrintf? > > I apologize for the pushback, but why? (While I'm definitely willing to do it, > I want to understand the reason first.) The code as is is clear and simple, and > it seems that StringPrintf is in a separate library, which complicates code > construction. Chaining + results in multiple temporaries being created. This results in the contents of strings being copied multiple times between temporaries, and a simple chain of concatenations can be surprisingly expensive. It probably doesn't matter here, but because of the behavior of +, it's generally avoided in favor of alternatives for consistency. If you don't want to use StringPrintf, then use +=. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: DictionaryValue* result = new DictionaryValue(); On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > scoped_ptr > > I tried originally, and that failed to properly transfer ownership. Reading up > on scoped pointers, they are supposed to be used only if the lifetime of the > object matches the lifetime of the function - in this case, the result must > outlast the lifetime of the function. Perhaps I could use some unique_ptr and a > swap idiom, but the ExtensionFunction is not using that so I'd have to change > the contract of ExtensionFunction::SetResult everywhere to use it.. I could use > auto_ptr with release instead, but auto_ptr seems to be a no-no in our code > base, and not a recommended practice any more. I'm definitely open to more > suggestions... Use scoped_ptr<T>::release() to transfer ownership to SetResult(). It makes the code self-documenting. In this instance, however, we should be using the glue provided by the IDL generator instead of rolling our own serialization (for example, how OnMessage uses glue::Message instead of using DictionaryValue* directly). http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:106: On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/15 22:20:45, dcheng wrote: > > Extra new line. With 80 chars, vertical space is a premium. =) > > Done, but I'm not sure that I agree in principle. Sure, it is nice to see more > code vertically, but if extra whitespace makes the code more readable, I see it > as an overall maintanability win, even if you can fit fewer lines onscreen. It's OK to have newlines to break up logical blocks of code, but it's unnecessary at the beginning/end of a function. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:108: // This also holds a reference on this class to keep it alive for a callback. On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/15 22:20:45, dcheng wrote: > > These two lines (and several other places) just describe what the code does. > > Instead of describing what code does, it is preferable to describe why (if > > needed). > > Comment changed, see if the new one is better. You can remove the comment on line 104 entirely. base::Bind() is designed to be safe in the way it handles "this" pointers. If you don't want base::Bind() to take a reference, you must use a WeakPtr<T> or base::Unretained() or base::Owned()). That's just the way it works, and it's not typically commented upon. http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:47: // that we use to build the channel ID. On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 19:12:43, dcheng wrote: > > I would probably leave out this comment as it merely describes what a reader > can > > see by looking at line 61 and line 67. > > Sure, I'll do that, but I have a question on Philosophy - I use comments to > quickly skim what a code is supposed to be doing when I do maintenance. Is the > way we do things at Google and on Chrome to not comment what the code does? > Personally, I think it makes the code better and more readable, but I will > absolutely go along with the way we do things here. It's OK to comment on the 'what' if it's something subtle or tricky. In this case, the class name pretty much encapsulates what the class does--it's the implementation of the GetChannelId extension function. By definition, an extension function is callable from Javascript. Specific comments on details of the implementation probably belong in the .cc. However, something that might belong in this class comment might describe how the channel ID is derived (I would probably avoid specific mention of ObfuscatedGaiaIdFetcher).
http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/10004/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: DictionaryValue* result = new DictionaryValue(); On 2012/08/19 22:38:35, dcheng wrote: > On 2012/08/19 22:05:14, Pete Williamson wrote: > > On 2012/08/16 19:12:43, dcheng wrote: > > > scoped_ptr > > > > I tried originally, and that failed to properly transfer ownership. Reading > up > > on scoped pointers, they are supposed to be used only if the lifetime of the > > object matches the lifetime of the function - in this case, the result must > > outlast the lifetime of the function. Perhaps I could use some unique_ptr and > a > > swap idiom, but the ExtensionFunction is not using that so I'd have to change > > the contract of ExtensionFunction::SetResult everywhere to use it.. I could > use > > auto_ptr with release instead, but auto_ptr seems to be a no-no in our code > > base, and not a recommended practice any more. I'm definitely open to more > > suggestions... > > Use scoped_ptr<T>::release() to transfer ownership to SetResult(). It makes the > code self-documenting. > > In this instance, however, we should be using the glue provided by the IDL > generator instead of rolling our own serialization (for example, how OnMessage > uses glue::Message instead of using DictionaryValue* directly). Some examples of using the IDL glue: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/... http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/... Search for "Results"
Latest patch should fix all issues so far, please take a look when you can. Thanks! https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h:16: class StubURLFetcher : public URLFetcher { On 2012/08/13 21:21:14, Pete Williamson wrote: > On 2012/08/09 21:44:29, munjal wrote: > > What do you need this class for? If for testing, there are already quite a few > > classes like FakeURLFetcher, TestURLFetcher ,etc. that you can use in testing. > > I took a look at FakeURLFetcher and TestURLFetcher. Both seem to require that > you stub out the factory instead of the fetcher, and both seem to require an > additional thread to run on. I think that the test code is cleaner with this > mock of the URL Fetcher, but I could be convinced otherwise. Are there any > issues with threading that I run into if I use FakeURLFetcher? Done. I found out that I could use the FakeURLFetcher by itself without a thread and a factory, so it ended up not being as much trouble as I was expecting it to be. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:54: : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()), On 2012/08/19 22:38:35, dcheng wrote: > On 2012/08/19 22:05:14, Pete Williamson wrote: > > On 2012/08/16 19:12:43, dcheng wrote: > > > I'm not a fan of Scopes(), but I guess there's no great solution here since > we > > > always pass in the same thing. > > > > > > Scopes() shouldn't be a member function though. > > > > Munjal suggested that it be a member function because it is an important > > implementation detail of the class, and I agree. I'm happy to change it to be > a > > non-member function if you and Munjal both prefer. > > It is an implementation detail. In fact, it's so much of one that it can live > entirely in the .cc =) Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: delete this; On 2012/08/19 22:05:14, Pete Williamson wrote: > On 2012/08/16 21:45:12, munjal wrote: > > On 2012/08/16 19:12:43, dcheng wrote: > > > On 2012/08/15 23:04:19, munjal wrote: > > > > We should definitely avoid this. Let us just define a delegate interface > and > > > let > > > > the extension function do the right thing in terms of lifetime management. > > > > > > > > here is another "delete this;" in ProcessApiCallFailure. We should remove > > > both. > > > > > > The problem is this: > > > The extension function (logically) ought to "own" its corresponding instance > > of > > > ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to keep > a > > > ref count to PushMessagingGetChannelIdFunction to keep it alive (since we > > reply > > > asynchronously). We need to break the cycle somehow or find another way to > fix > > > the ownership model here. I've been thinking about this problem and I have > not > > > yet come to a great solution. > > > > The ExtensionFunction should AddRef itself instead of relying on this class to > > add a reference. That is what all extension functions do. > > Daniel explicitly requested that I not call AddRef explicitly, and that my code > should never do it. I'm happy with either solution, I'll try to get a 5 minute > chat with both of you so we can pick the best way to solve this issue. After some clarification from Daniel, I settled on the AddRef method, which is now implemented. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:39: class PushMessagingGetChannelIdFunction; On 2012/08/15 23:04:19, munjal wrote: > I am not sure I understand the situation here. If we define a delegate > interface, and make PushMessagingGetChannelIdFunction implement that delegate, > why doesn't it work? We should not try to make the Delegate class ref counted. Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:50: delegate, On 2012/08/15 23:04:19, munjal wrote: > We should try our best to remove this dependency. The fetcher class should not > need to know about the extension function that uses it. In other words, any > class should be able to use this fetcher by becoming a delegate, not just > extension function. Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:77: static std::vector<std::string> Scopes(); On 2012/08/16 19:12:43, dcheng wrote: > This method name should probably have a verb in it, if you choose to keep it. I > would prefer to pass in scopes as an argument though, since passing it in this > way looks clunky (in my opinion). Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:66: void PushMessagingGetChannelIdFunction::RespondOnUIThread( On 2012/08/15 23:04:19, munjal wrote: > Isn't the ObfuscatedGaiaIdFetcher already running on UI thread (and it takes > care of doing the fetch on IO thread)? > > In fact, IIRC, all fetcher / flow classes call back the delegate on the original > thread the fetchers were constructed. So I don't think we need to post task on > UI thread to report a result. We should already be caleld back on the UI thread. Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:72: std::string channel_id = gaia_id + "." + extension_id(); On 2012/08/19 22:38:35, dcheng wrote: > On 2012/08/19 22:05:14, Pete Williamson wrote: > > On 2012/08/15 23:04:19, munjal wrote: > > > NIt: Use StringPrintf? > > > > I apologize for the pushback, but why? (While I'm definitely willing to do > it, > > I want to understand the reason first.) The code as is is clear and simple, > and > > it seems that StringPrintf is in a separate library, which complicates code > > construction. > > Chaining + results in multiple temporaries being created. This results in the > contents of strings being copied multiple times between temporaries, and a > simple chain of concatenations can be surprisingly expensive. > > It probably doesn't matter here, but because of the behavior of +, it's > generally avoided in favor of alternatives for consistency. If you don't want to > use StringPrintf, then use +=. Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: DictionaryValue* result = new DictionaryValue(); On 2012/08/19 22:38:35, dcheng wrote: > On 2012/08/19 22:05:14, Pete Williamson wrote: > > On 2012/08/16 19:12:43, dcheng wrote: > > > scoped_ptr > > > > I tried originally, and that failed to properly transfer ownership. Reading > up > > on scoped pointers, they are supposed to be used only if the lifetime of the > > object matches the lifetime of the function - in this case, the result must > > outlast the lifetime of the function. Perhaps I could use some unique_ptr and > a > > swap idiom, but the ExtensionFunction is not using that so I'd have to change > > the contract of ExtensionFunction::SetResult everywhere to use it.. I could > use > > auto_ptr with release instead, but auto_ptr seems to be a no-no in our code > > base, and not a recommended practice any more. I'm definitely open to more > > suggestions... > > Use scoped_ptr<T>::release() to transfer ownership to SetResult(). It makes the > code self-documenting. > > In this instance, however, we should be using the glue provided by the IDL > generator instead of rolling our own serialization (for example, how OnMessage > uses glue::Message instead of using DictionaryValue* directly). Done. Ah, I see that we are using chrome's scoped_ptr, not std::scoped_ptr, which has no release method, which is why I thought it wouldn't work. In any case, this has now been replaced with the IDL glue method. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:108: // This also holds a reference on this class to keep it alive for a callback. On 2012/08/19 22:38:35, dcheng wrote: > On 2012/08/19 22:05:14, Pete Williamson wrote: > > On 2012/08/15 22:20:45, dcheng wrote: > > > These two lines (and several other places) just describe what the code does. > > > Instead of describing what code does, it is preferable to describe why (if > > > needed). > > > > Comment changed, see if the new one is better. > > You can remove the comment on line 104 entirely. base::Bind() is designed to be > safe in the way it handles "this" pointers. If you don't want base::Bind() to > take a reference, you must use a WeakPtr<T> or base::Unretained() or > base::Owned()). That's just the way it works, and it's not typically commented > upon. Done. https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... File chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/10004/chrome/browser/ext... chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h:6: #ifndef CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_STUB_URL_FETCHER_H_ On 2012/08/15 23:04:19, munjal wrote: > No need ot write our own stub URL fetcher. Use TestURLFetcher or FakeURLFetcher. Done.
Looking good. I mostly have minor comments at this point, mostly regarding style. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:38: Generally we don't use double newlines to separate methods/namespaces. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:127: if (!rv || NULL == dict) It is unnecessary to check that dict is NULL. So you can just do if (!value->GetAsDictionary(&dict)) return false; https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:20: class DictionaryValue; This is unused. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:20: class DictionaryValue; Unused. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:49: // TODO: Someday let's make a profile keyed service to cache the GAIA ID. Should be TODO(petewil) https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:81: // Unowned, weak pointer to the delegate. Normally the delegate owns Don't use "weak" since we have WeakPtr<T> as well. I'd just say that the delegate must remain valid for the lifetime of this class. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:84: std::string obfuscated_gaia_id_; It seems like this is unused. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:32: TestDelegate() { Prefer initializer syntax. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:37: bool Succeeded() { return succeeded_; } bool succeeded() const { return succeeded_; } https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:38: bool Failed() { return failed_; } Ditto. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:63: bool ret2 = fetcher.ParseResponse(kBadJsonData, &channel_id_out2); Isn't this a static method? We can just do ObfuscatedGaiaIdFetcher::ParseResponse instead of creating instances. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:83: ObfuscatedGaiaIdFetcher* fetcher = scoped_ptr, as I think this now leaks. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:84: new ObfuscatedGaiaIdFetcher(NULL, delegate.get(), refresh_token); Nit: You can probably just say std::string(), as people just tend to use unnamed temporaries for arguments like this. I don't feel particularly strongly about this though. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:99: ObfuscatedGaiaIdFetcher* fetcher = scoped_ptr. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:100: new ObfuscatedGaiaIdFetcher(NULL, delegate.get(), refresh_token); Same as above about refresh_token. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:111: // TODO: add case for when the base class fails and calls TODO(petewil) https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:64: const std::string& gaia_id, const long status) { Just 'long' instead of 'const long' https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:74: // TODO: (after initial checkin) It may be a good idea to further TODO(petewil) https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:95: const std::string& refresh_token = One space. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: AddRef(); My preference would be to move this into RunImpl, above the return statement. The rationale for this is RunImpl is a common interface for ExtensionFunctions to implement so it's easier to find it there if someone is just searching through the code. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:11: #include "base/gtest_prod_util.h" #include "base/memory/scoped_ptr.h" https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:43: : public AsyncExtensionFunction, public ObfuscatedGaiaIdFetcher::Delegate{ Indent two spaces. Missing space before { https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:58: virtual void OnObfuscatedGaiaIdFetchSuccess(const std::string& gaia_id) I'd add a comment here: // ObfuscatedGiaiaIdFetcher::Delegate implementation. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/common/exten... File chrome/common/extensions/api/experimental_push_messaging.idl (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/common/exten... chrome/common/extensions/api/experimental_push_messaging.idl:27: // Async calculate the channel ID for the app to give to its push server These comments will end up in the generated documentation. All extension functions are, by definition, asynchronous. How about a comment like this instead? // Retrieves the channel ID associated with this extension. Typically, an extension will want to send this value to its application server so the server can use it to trigger push messages back to the extension. I used "extension" instead of "app" here but I think that's OK, since it helps disambiguate between "Chrome extension" and the "application server" (not sure what a better term here would be).
Looking much better and simpler. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:40: // We use this static function to encapsulate the scopes needed at ctor time. Nit: may be just say "Returns a vector of scopes needed to call the API to get obfuscated gaia id." https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:44: "https://www.googleapis.com/auth/chromewebstore.notification"); NIt: style issue: indent 4 spaces https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:86: // TODO(munjal): Change error code paths in this method to report an Nit: I am ok with my name in the TODO. But just making sure you did this intentionally vs a copy/paste artifact. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:81: // Unowned, weak pointer to the delegate. Normally the delegate owns On 2012/08/20 19:10:01, dcheng wrote: > Don't use "weak" since we have WeakPtr<T> as well. I'd just say that the > delegate must remain valid for the lifetime of this class. Agree with Daniel. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:63: void PushMessagingGetChannelIdFunction::RespondOnUIThread( I still don't understand fully whether we need to do the thread task positng explicitly here or not. Can you please explain why we need this?
Latest style fixes per Munjal and DCheng https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:38: On 2012/08/20 19:10:01, dcheng wrote: > Generally we don't use double newlines to separate methods/namespaces. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:40: // We use this static function to encapsulate the scopes needed at ctor time. On 2012/08/20 20:01:25, munjal wrote: > Nit: may be just say "Returns a vector of scopes needed to call the API to get > obfuscated gaia id." Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:44: "https://www.googleapis.com/auth/chromewebstore.notification"); On 2012/08/20 20:01:25, munjal wrote: > NIt: style issue: indent 4 spaces Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:86: // TODO(munjal): Change error code paths in this method to report an On 2012/08/20 20:01:25, munjal wrote: > Nit: I am ok with my name in the TODO. But just making sure you did this > intentionally vs a copy/paste artifact. This was a copy/ paste artifact, but I think it is still valid, this should be fixed at the same time as the code I pasted from. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:127: if (!rv || NULL == dict) On 2012/08/20 19:10:01, dcheng wrote: > It is unnecessary to check that dict is NULL. So you can just do if > (!value->GetAsDictionary(&dict)) return false; Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:20: class DictionaryValue; On 2012/08/20 19:10:01, dcheng wrote: > Unused. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:49: // TODO: Someday let's make a profile keyed service to cache the GAIA ID. On 2012/08/20 19:10:01, dcheng wrote: > Should be TODO(petewil) Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:81: // Unowned, weak pointer to the delegate. Normally the delegate owns On 2012/08/20 20:01:25, munjal wrote: > On 2012/08/20 19:10:01, dcheng wrote: > > Don't use "weak" since we have WeakPtr<T> as well. I'd just say that the > > delegate must remain valid for the lifetime of this class. > > Agree with Daniel. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:84: std::string obfuscated_gaia_id_; On 2012/08/20 19:10:01, dcheng wrote: > It seems like this is unused. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:32: TestDelegate() { On 2012/08/20 19:10:01, dcheng wrote: > Prefer initializer syntax. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:37: bool Succeeded() { return succeeded_; } On 2012/08/20 19:10:01, dcheng wrote: > bool succeeded() const { return succeeded_; } Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:38: bool Failed() { return failed_; } On 2012/08/20 19:10:01, dcheng wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:63: bool ret2 = fetcher.ParseResponse(kBadJsonData, &channel_id_out2); On 2012/08/20 19:10:01, dcheng wrote: > Isn't this a static method? We can just do > ObfuscatedGaiaIdFetcher::ParseResponse instead of creating instances. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:83: ObfuscatedGaiaIdFetcher* fetcher = On 2012/08/20 19:10:01, dcheng wrote: > scoped_ptr, as I think this now leaks. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:84: new ObfuscatedGaiaIdFetcher(NULL, delegate.get(), refresh_token); On 2012/08/20 19:10:01, dcheng wrote: > Nit: You can probably just say std::string(), as people just tend to use unnamed > temporaries for arguments like this. I don't feel particularly strongly about > this though. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:99: ObfuscatedGaiaIdFetcher* fetcher = On 2012/08/20 19:10:01, dcheng wrote: > scoped_ptr. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:100: new ObfuscatedGaiaIdFetcher(NULL, delegate.get(), refresh_token); On 2012/08/20 19:10:01, dcheng wrote: > Same as above about refresh_token. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:111: // TODO: add case for when the base class fails and calls On 2012/08/20 19:10:01, dcheng wrote: > TODO(petewil) Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:63: void PushMessagingGetChannelIdFunction::RespondOnUIThread( On 2012/08/20 20:01:25, munjal wrote: > I still don't understand fully whether we need to do the thread task positng > explicitly here or not. Can you please explain why we need this? We aren't doing any thread posting anymore. Changed the name of this function to "Respond" to avoid any potential confusion. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:64: const std::string& gaia_id, const long status) { On 2012/08/20 19:10:01, dcheng wrote: > Just 'long' instead of 'const long' Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:74: // TODO: (after initial checkin) It may be a good idea to further On 2012/08/20 19:10:01, dcheng wrote: > TODO(petewil) Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:95: const std::string& refresh_token = On 2012/08/20 19:10:01, dcheng wrote: > One space. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:101: AddRef(); On 2012/08/20 19:10:01, dcheng wrote: > My preference would be to move this into RunImpl, above the return statement. > The rationale for this is RunImpl is a common interface for ExtensionFunctions > to implement so it's easier to find it there if someone is just searching > through the code. Done. (Actually, I moved the whole function into RunImpl - there wasn't much left when I moved the AddRef and Start https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:11: #include "base/gtest_prod_util.h" On 2012/08/20 19:10:01, dcheng wrote: > #include "base/memory/scoped_ptr.h" Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:43: : public AsyncExtensionFunction, public ObfuscatedGaiaIdFetcher::Delegate{ On 2012/08/20 19:10:01, dcheng wrote: > Indent two spaces. Missing space before { Added spaces https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:58: virtual void OnObfuscatedGaiaIdFetchSuccess(const std::string& gaia_id) On 2012/08/20 19:10:01, dcheng wrote: > I'd add a comment here: > // ObfuscatedGiaiaIdFetcher::Delegate implementation. Done. https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/common/exten... File chrome/common/extensions/api/experimental_push_messaging.idl (right): https://chromiumcodereview.appspot.com/10836182/diff/1014/chrome/common/exten... chrome/common/extensions/api/experimental_push_messaging.idl:27: // Async calculate the channel ID for the app to give to its push server On 2012/08/20 19:10:01, dcheng wrote: > These comments will end up in the generated documentation. All extension > functions are, by definition, asynchronous. How about a comment like this > instead? > // Retrieves the channel ID associated with this extension. Typically, an > extension will want to send this value to its application server so the server > can use it to trigger push messages back to the extension. > > I used "extension" instead of "app" here but I think that's OK, since it helps > disambiguate between "Chrome extension" and the "application server" (not sure > what a better term here would be). Done.
https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:7: #include <string> This is already included in the header, so it's OK to omit it here. https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: if (delegate_ != NULL) Do we need to allow NULL delegates? It doesn't look like anyone uses this functionality, so we should probably just remove this and DCHECK() that delegate is not NULL in the constructor. Also the preferred style for checking null/non-null-ness of a pointer is if (p) and if (!p) https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:94: ReportFailure(GoogleServiceAuthError::FromConnectionError(101)); What does 101 mean? Is this code defined somewhere? https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:11: #include <vector> This #include is unused. https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:13: #include "base/memory/ref_counted.h" Ditto. https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:14: #include "base/string16.h" Ditto. https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:34: bool Succeeded() const { return succeeded_; } Simple getter names are lowercase as well. https://chromiumcodereview.appspot.com/10836182/diff/2014/chrome/browser/exte... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:35: bool Failed() const { return failed_; } Ditto.
LGTM with nits. Thanks for your patience. You might need owners approval for extensions from mihaip@ http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:65: // a response. Nit: Remove this comment in favor of the comment I suggest below (for consistency with code for all extension APIs). http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:66: AddRef(); Nit: add the following comment on the same line as AddRef 9t obe consistent with other extensionds code) // Balanced in Respond() http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: if (!gaia_id.empty()) { shouldn't we bail out if gaia_id is empty? http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:96: // PushMessagingGetChannelIdFunction object to die. Nit: Remove this comment in favor the comment I suggest below. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:97: Release(); Nit: Add the following comment to be consistent with other extensions code. // Balanced in RunImpl() http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:56: void GetChannelIdOnIOThread(); You are not using this method anymore. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:57: void Respond(const std::string& gaiaId, const long status); Nit: May be name "ReportResult" to be consistent with other extension APIs code.
http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:7: #include <string> On 2012/08/20 21:04:13, dcheng wrote: > This is already included in the header, so it's OK to omit it here. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:63: if (delegate_ != NULL) On 2012/08/20 21:04:13, dcheng wrote: > Do we need to allow NULL delegates? It doesn't look like anyone uses this > functionality, so we should probably just remove this and DCHECK() that delegate > is not NULL in the constructor. > > Also the preferred style for checking null/non-null-ness of a pointer is if (p) > and if (!p) Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:94: ReportFailure(GoogleServiceAuthError::FromConnectionError(101)); On 2012/08/20 21:04:13, dcheng wrote: > What does 101 mean? Is this code defined somewhere? This came from code that I copied from oauth2_mint_token_flow. Munjal says it was picked arbitrarily just so that they can add more errors in the future, and that it is good if I also use 101 here. I added a comment explaining this. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:11: #include <vector> On 2012/08/20 21:04:13, dcheng wrote: > This #include is unused. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:13: #include "base/memory/ref_counted.h" On 2012/08/20 21:04:13, dcheng wrote: > Ditto. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:14: #include "base/string16.h" On 2012/08/20 21:04:13, dcheng wrote: > Ditto. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:34: bool Succeeded() const { return succeeded_; } On 2012/08/20 21:04:13, dcheng wrote: > Simple getter names are lowercase as well. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:35: bool Failed() const { return failed_; } On 2012/08/20 21:04:13, dcheng wrote: > Ditto. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:65: // a response. On 2012/08/20 21:16:22, munjal wrote: > Nit: Remove this comment in favor of the comment I suggest below (for > consistency with code for all extension APIs). Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:66: AddRef(); On 2012/08/20 21:16:22, munjal wrote: > Nit: add the following comment on the same line as AddRef 9t obe consistent with > other extensionds code) > // Balanced in Respond() Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: if (!gaia_id.empty()) { On 2012/08/20 21:16:22, munjal wrote: > shouldn't we bail out if gaia_id is empty? Even if the gaia_id is empty, we still need to return an error to the user. Added an else clause to set the error state. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:96: // PushMessagingGetChannelIdFunction object to die. On 2012/08/20 21:16:22, munjal wrote: > Nit: Remove this comment in favor the comment I suggest below. Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:97: Release(); On 2012/08/20 21:16:22, munjal wrote: > Nit: Add the following comment to be consistent with other extensions code. > // Balanced in RunImpl() Done. http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/2014/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:56: void GetChannelIdOnIOThread(); On 2012/08/20 21:16:22, munjal wrote: > You are not using this method anymore. Done.
LGTM as well with the nits addressed. http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:48: Newline =) http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:52: EXPECT_EQ(channel_id_out1, "My-channel-id"); By convention, the "expected" arg should come first since this is the order googletest expects them when printing out test failures. Sorry for missing this in earlier reviews. http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:80: new ObfuscatedGaiaIdFetcher(NULL, delegate.get(), std::string())); Minor nit. You can just stack allocate these. TestDelegate delegate; ObsfucatedGaiaIdFetcher fetcher(NULL, &delegate, std::string()); Sorry for not noticing this in earlier reviews. http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:82: } else if (status != 0) { That means if gaia_id is empty and status is zero we signal success right? It seems like we should either have: if (status == 0) { DCHECK(!gaia_id.empty()); // Set channel ID to non-empty value } else { // Signal failure. } or if (!gaia_id.empty()) { // Do stuff } else { DCHECK(status != 0); }
http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:48: On 2012/08/20 22:52:22, dcheng wrote: > Newline =) Done. http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:52: EXPECT_EQ(channel_id_out1, "My-channel-id"); On 2012/08/20 22:52:22, dcheng wrote: > By convention, the "expected" arg should come first since this is the order > googletest expects them when printing out test failures. > > Sorry for missing this in earlier reviews. Done. http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:80: new ObfuscatedGaiaIdFetcher(NULL, delegate.get(), std::string())); On 2012/08/20 22:52:22, dcheng wrote: > Minor nit. You can just stack allocate these. > TestDelegate delegate; > ObsfucatedGaiaIdFetcher fetcher(NULL, &delegate, std::string()); > > Sorry for not noticing this in earlier reviews. Done. http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right): http://codereview.chromium.org/10836182/diff/9025/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:82: } else if (status != 0) { On 2012/08/20 22:52:22, dcheng wrote: > That means if gaia_id is empty and status is zero we signal success right? > > It seems like we should either have: > if (status == 0) { > DCHECK(!gaia_id.empty()); > // Set channel ID to non-empty value > } else { > // Signal failure. > } > > or > > if (!gaia_id.empty()) { > // Do stuff > } else { > DCHECK(status != 0); > } != was supposed to be ==, changed.
Adding owners for approval + Ben for GYPI + Mihai for extension
LGTM http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:89: std::string channel_id; Should this be called obfuscated_id? http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: // token since access token expire every hour and we expect to get "access tokens expire" is more correct. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:112: // the obfuscated user ID, so report failure to the caller. Instead of "user ID" say "Gaia ID", for consistency with other comments. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:5: // This file has code to fetch the Obfuscated GAIA ID from the server Redundant comment. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:26: // Fetches obfuscated gaia id of the Google Account that is logged in to Chrome. Capitalize Gaia and ID (in other comments too). Also, "Gaia" is more correct than "GAIA" (based on google3 usage). http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:25: const std::string& obfuscated_id) { These should have the OVERRIDE annotation (making the "inherited methods" comment redudant) http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:57: // ObfuscatedGiaiaIdFetcher::Delegate implementation. Add a newline before this comment. http://codereview.chromium.org/10836182/diff/8011/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_push_messaging.idl (right): http://codereview.chromium.org/10836182/diff/8011/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_push_messaging.idl:17: // The subchannel the message was sent on. This comment needs to be updated. Also, APIs generally signal errors via the SetError function (which populates the chrome.runtime.lastError value). http://codereview.chromium.org/10836182/diff/8011/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_push_messaging.idl:20: // The payload associated with the message, if any. This comment needs to be updated.
BTW, you don't need to edit the CL description to ask for OWNERS approvals, that can happen via the "Publish+Mail Comments" link on the codereview site. Also, the BUG= line traditionally goes at the end of the description. (you should clean up the description since it becomes the commit message)
Ben - Need OWNERS approval for two small gypi changes This addresses all of Mihai's concerns http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:89: std::string channel_id; On 2012/08/22 21:40:45, Mihai Parparita wrote: > Should this be called obfuscated_id? Done. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:105: // token since access token expire every hour and we expect to get On 2012/08/22 21:40:45, Mihai Parparita wrote: > "access tokens expire" is more correct. Done. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.cc:112: // the obfuscated user ID, so report failure to the caller. On 2012/08/22 21:40:45, Mihai Parparita wrote: > Instead of "user ID" say "Gaia ID", for consistency with other comments. Done. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:5: // This file has code to fetch the Obfuscated GAIA ID from the server On 2012/08/22 21:40:45, Mihai Parparita wrote: > Redundant comment. Done. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h:26: // Fetches obfuscated gaia id of the Google Account that is logged in to Chrome. On 2012/08/22 21:40:45, Mihai Parparita wrote: > Capitalize Gaia and ID (in other comments too). > > Also, "Gaia" is more correct than "GAIA" (based on google3 usage). Done. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc:25: const std::string& obfuscated_id) { On 2012/08/22 21:40:45, Mihai Parparita wrote: > These should have the OVERRIDE annotation (making the "inherited methods" > comment redudant) Done. http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right): http://codereview.chromium.org/10836182/diff/8011/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_api.h:57: // ObfuscatedGiaiaIdFetcher::Delegate implementation. On 2012/08/22 21:40:45, Mihai Parparita wrote: > Add a newline before this comment. Done. http://codereview.chromium.org/10836182/diff/8011/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_push_messaging.idl (right): http://codereview.chromium.org/10836182/diff/8011/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_push_messaging.idl:17: // The subchannel the message was sent on. On 2012/08/22 21:40:45, Mihai Parparita wrote: > This comment needs to be updated. > > Also, APIs generally signal errors via the SetError function (which populates > the chrome.runtime.lastError value). Done. http://codereview.chromium.org/10836182/diff/8011/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_push_messaging.idl:20: // The payload associated with the message, if any. On 2012/08/22 21:40:45, Mihai Parparita wrote: > This comment needs to be updated. Done.
gyp lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@google.com/10836182/24002
Change committed as 153108 |
