Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918513002:
[NTP::Push] Add the classes for sending a breaking news subscription request (Closed)
Description[NTP::Push] Add the classes for sending a breaking news subscription request
Add the classes for sending a gcm subscription request to the content
suggestion server. They are required to be able to later deliver
breaking news notifications via GCM.
BUG=728119
Review-Url: https://codereview.chromium.org/2918513002
Cr-Commit-Position: refs/heads/master@{#476647}
Committed: https://chromium.googlesource.com/chromium/src/+/a19c77077f663f771e822ffb025d9cc607e9cf8e
Patch Set 1 #
Total comments: 16
Patch Set 2 : Adding the tests #
Total comments: 73
Patch Set 3 : bauerb@ comments. #
Total comments: 10
Patch Set 4 : fhorschig@ comments. #Patch Set 5 : bauerb@ comments. #
Total comments: 50
Patch Set 6 : bauerb@ fhorschig@ comments. #
Total comments: 24
Patch Set 7 : bauerb@ fhorschig@ comments. #
Total comments: 2
Patch Set 8 : Minor comments. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (12 generated)
Description was changed from ========== Adding the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later delivery breaking news notifications via GCM. BUG=719998 ========== to ========== Adding the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later delivery breaking news notifications via GCM. BUG=719998 ==========
mamir@chromium.org changed reviewers: + fhorschig@chromium.org
Thank you Friedrich for reviewing this.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
Drive-by review! :) https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:54: LOG_IF(DFATAL, !request_completed_callback_.is_null()) It's illegal to destroy a request before it has completed? What if we are shutting down? (See also the comment in line 86) Also, why DFATAL? If this is a condition that is genuinely not supposed to happen, it shouldn't be handled in a release build, which includes logging (plus, no one would notice anyway). https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:86: // request succeeded. So the callback is never called if the request succeeded? https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:176: "Chromium can received breaking news via GCM push messages. " "receive" https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:179: "Suscrpion takes place only once per profile lifetime. " "Subscription" https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:181: "The sunscrption token that identifies this Chromium profile." "subscription" https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.h:76: SubscriptionJsonRequest(const ParseJSONCallback& callback); How is a client supposed to construct an object of this class -- via this constructor or via the builder? https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.h:88: // The fetcher for subscrbing. Only non-null if a subscription process is "subscribing" https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_request_params.h (right): https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_request_params.h:25: // GCM subscribtion token obtain from GCM driver (instanceID::getToken()) "subscription", "obtained"
Description was changed from ========== Adding the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later delivery breaking news notifications via GCM. BUG=719998 ========== to ========== [NTP::Push] Add the classes for sending a gcm subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=719998 ==========
mamir@chromium.org changed reviewers: - bauerb@chromium.org
Description was changed from ========== [NTP::Push] Add the classes for sending a gcm subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=719998 ========== to ========== [NTP::Push] Add the classes for sending a gcm subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=728119 ==========
mamir@chromium.org changed reviewers: + bauerb@chromium.org
Thank you Bernhard for the drive-by reviews. Please, keep them coming. (and sorry for the silly typos, ehm) https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:54: LOG_IF(DFATAL, !request_completed_callback_.is_null()) On 2017/05/31 13:12:45, Bernhard Bauer wrote: > It's illegal to destroy a request before it has completed? What if we are > shutting down? (See also the comment in line 86) > > Also, why DFATAL? If this is a condition that is genuinely not supposed to > happen, it shouldn't be handled in a release build, which includes logging > (plus, no one would notice anyway). Done. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:86: // request succeeded. On 2017/05/31 13:12:44, Bernhard Bauer wrote: > So the callback is never called if the request succeeded? After an offline discussion with fhorschig@ this is going to change. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:176: "Chromium can received breaking news via GCM push messages. " On 2017/05/31 13:12:44, Bernhard Bauer wrote: > "receive" Done. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:179: "Suscrpion takes place only once per profile lifetime. " On 2017/05/31 13:12:44, Bernhard Bauer wrote: > "Subscription" Done. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.cc:181: "The sunscrption token that identifies this Chromium profile." On 2017/05/31 13:12:44, Bernhard Bauer wrote: > "subscription" Done. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.h:76: SubscriptionJsonRequest(const ParseJSONCallback& callback); On 2017/05/31 13:12:45, Bernhard Bauer wrote: > How is a client supposed to construct an object of this class -- via this > constructor or via the builder? Via the builder. Making the constructor private. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_json_request.h:88: // The fetcher for subscrbing. Only non-null if a subscription process is On 2017/05/31 13:12:45, Bernhard Bauer wrote: > "subscribing" Done. https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_request_params.h (right): https://codereview.chromium.org/2918513002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_request_params.h:25: // GCM subscribtion token obtain from GCM driver (instanceID::getToken()) On 2017/05/31 13:12:45, Bernhard Bauer wrote: > "subscription", "obtained" Done.
Thanks! Just some small issues now: https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:57: /*error_details=*/"The CompletionCallback was never called"); Is that the right error? I would return something like "cancelled". Or just not run the callback at all -- presumably the client knows that it just cancelled the request. https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:72: return builder; Why not inline these calls? :) https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:88: builder.SetParams(params).SetUrl(GURL("http://valid-url.test")).Build(); Do you actually need to call Build() here?
First round of comments ... as discussed, you chose a very difficult road here :D https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:54: LOG_IF(DFATAL, !request_completed_callback_.is_null()) As bernhard said, this is weird (sorry). Could you improve that by calling any pending callback with a temporary error? (The urlfetcher is destroyed and the request cancelled here.) https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:79: .Run(/*result=*/nullptr, As discussed, a status code is sufficient if result is always nullptr. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:83: .Run(/*result=*/nullptr, same here. status should suffice. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:86: // request succeeded. here you could use a success status. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:92: default; I really want to know whether we need that. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:143: headers.SetHeader("Authorization", auth_header_); The auth_header_ is always empty for the forseeable future, right? Would it make sense to introduce it later, when we need it? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:148: bool is_signed_in = false; why don't you inline that? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:157: auto request = base::MakeUnique<base::DictionaryValue>(); Can we use a base::DictionaryValue instead of a pointer here? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:209: url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3); Do we want to retry three times? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:30: // A single request to subscribe for breaking news via GCM, s/,/. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:37: const std::string& error_details)>; How are error details used? Could we handle errors with the existing ntp_snippets::StatusCode? (see components/ntp_snippets/status.h) It seems the result is never used (or present in the response). Could you then simplify the callback to base::OnceCallback<void(const ntp_snippets::Status& status)>? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:43: Builder(Builder&&); Please check if the declaration of this move constructor is necessary. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:53: Builder& SetParseJsonCallback(ParseJSONCallback callback); Drop if you don't need to parse JSON. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:81: SubscriptionJsonRequest(const ParseJSONCallback& callback); As discussed, I guess this constructor should be private. Drop the callback if you don't need to parse JSON. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:82: SubscriptionJsonRequest(SubscriptionJsonRequest&&); Do you need this? (it's =default in cpp ... please let me know) https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:87: std::string GetResponseString() const; Will you need that? Usually all the status you need come with the callback. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:99: ParseJSONCallback parse_json_callback_; Drop if you don't need to parse JSON. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:21: #include "components/variations/variations_params_manager.h" There are quite some includes you wont need here: set test_mock_time_task_runner time tick_clock features snippets_contants? pref_service variations_params_manager test_url_fetcher_factory and maybe more ^^ https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:33: using testing::_; never used https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:34: using testing::Eq; never used https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:35: using testing::Not; never used https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:36: using testing::NotNull; never used https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:39: MATCHER_P(EqualsJSON, json, "equals JSON") { Consider creating a test_helper.cc file instead of duplicating all this code. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:76: std::unique_ptr<TestingPrefServiceSimple> pref_service_; You don't need that right now. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:77: scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_; You don't need that right now. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:79: net::TestURLFetcherFactory fetcher_factory_; You never used that anywhere. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_request_params.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.cc:5: #include "components/ntp_snippets/breaking_news/subscription_request_params.h" Looking at this rather sparse implementation ... we could inline the whole class into the json_request class ... likely even into the anonymous namespace. (The original request just published this to test the request param building) https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_request_params.h (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.h:20: struct SubscriptionRequestParams { that is a very, very, very simple class. are you sure we need it at all? It seems like we could inline the parameters as builder setters. (The original class used more difficult param construction and had to handle the outcome based on the result .... which we don't even parse here.) https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.h:28: // TODO(mamir): Additional fields to be added: country, language. All these fields look easy enough to add as a Set{Country,Language,...} into the builder. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.h:31: // Callbacks for JSON parsing to allow injecting platform-dependent code. None of which are actually used. https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:88: builder.SetParams(params).SetUrl(GURL("http://valid-url.test")).Build(); On 2017/05/31 14:45:40, Bernhard Bauer wrote: > Do you actually need to call Build() here? I don't think so, too
Description was changed from ========== [NTP::Push] Add the classes for sending a gcm subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=728119 ========== to ========== [NTP::Push] Add the classes for sending a breaking news subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=728119 ==========
Thanks a lot fhorschig@ for the thorough informative review. Code is indeed much simpler now. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:54: LOG_IF(DFATAL, !request_completed_callback_.is_null()) On 2017/05/31 14:51:12, fhorschig wrote: > As bernhard said, this is weird (sorry). > Could you improve that by calling any pending callback with > a temporary error? > (The urlfetcher is destroyed and the request cancelled here.) Is it OK as of now? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:79: .Run(/*result=*/nullptr, On 2017/05/31 14:51:12, fhorschig wrote: > As discussed, a status code is sufficient if result is always nullptr. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:83: .Run(/*result=*/nullptr, On 2017/05/31 14:51:12, fhorschig wrote: > same here. status should suffice. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:86: // request succeeded. On 2017/05/31 14:51:12, fhorschig wrote: > here you could use a success status. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:92: default; On 2017/05/31 14:51:12, fhorschig wrote: > I really want to know whether we need that. Required the test IIUC https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:143: headers.SetHeader("Authorization", auth_header_); On 2017/05/31 14:51:13, fhorschig wrote: > The auth_header_ is always empty for the forseeable future, right? > Would it make sense to introduce it later, when we need it? Yes it makes sense. Removed. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:148: bool is_signed_in = false; On 2017/05/31 14:51:12, fhorschig wrote: > why don't you inline that? Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:157: auto request = base::MakeUnique<base::DictionaryValue>(); On 2017/05/31 14:51:12, fhorschig wrote: > Can we use a base::DictionaryValue instead of a pointer here? Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:209: url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3); On 2017/05/31 14:51:12, fhorschig wrote: > Do we want to retry three times? What do you think? :-) https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:30: // A single request to subscribe for breaking news via GCM, On 2017/05/31 14:51:13, fhorschig wrote: > s/,/. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:37: const std::string& error_details)>; On 2017/05/31 14:51:13, fhorschig wrote: > How are error details used? > Could we handle errors with the existing ntp_snippets::StatusCode? > (see components/ntp_snippets/status.h) > > It seems the result is never used (or present in the response). > Could you then simplify the callback to > base::OnceCallback<void(const ntp_snippets::Status& status)>? Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:43: Builder(Builder&&); On 2017/05/31 14:51:13, fhorschig wrote: > Please check if the declaration of this move constructor is necessary. Needed in the test IIUC. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:53: Builder& SetParseJsonCallback(ParseJSONCallback callback); On 2017/05/31 14:51:13, fhorschig wrote: > Drop if you don't need to parse JSON. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:81: SubscriptionJsonRequest(const ParseJSONCallback& callback); On 2017/05/31 14:51:13, fhorschig wrote: > As discussed, I guess this constructor should be private. > Drop the callback if you don't need to parse JSON. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:82: SubscriptionJsonRequest(SubscriptionJsonRequest&&); On 2017/05/31 14:51:13, fhorschig wrote: > Do you need this? (it's =default in cpp ... please let me know) Removed! https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:87: std::string GetResponseString() const; On 2017/05/31 14:51:13, fhorschig wrote: > Will you need that? > Usually all the status you need come with the callback. Removed! https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:99: ParseJSONCallback parse_json_callback_; On 2017/05/31 14:51:13, fhorschig wrote: > Drop if you don't need to parse JSON. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:21: #include "components/variations/variations_params_manager.h" On 2017/05/31 14:51:14, fhorschig wrote: > There are quite some includes you wont need here: > > set > test_mock_time_task_runner > time > tick_clock > features > snippets_contants? > pref_service > variations_params_manager > test_url_fetcher_factory > > and maybe more ^^ Well, actually test_mock_time_task_runner is needed ;-) But I have cleaned the imports. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:33: using testing::_; On 2017/05/31 14:51:14, fhorschig wrote: > never used Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:34: using testing::Eq; On 2017/05/31 14:51:14, fhorschig wrote: > never used Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:35: using testing::Not; On 2017/05/31 14:51:14, fhorschig wrote: > never used Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:36: using testing::NotNull; On 2017/05/31 14:51:14, fhorschig wrote: > never used Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:39: MATCHER_P(EqualsJSON, json, "equals JSON") { On 2017/05/31 14:51:14, fhorschig wrote: > Consider creating a test_helper.cc file instead of duplicating all this code. Could we please discuss this offline? https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:76: std::unique_ptr<TestingPrefServiceSimple> pref_service_; On 2017/05/31 14:51:14, fhorschig wrote: > You don't need that right now. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:77: scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_; On 2017/05/31 14:51:14, fhorschig wrote: > You don't need that right now. I do the initialize the TestURLRequestContextGetter https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:79: net::TestURLFetcherFactory fetcher_factory_; On 2017/05/31 14:51:14, fhorschig wrote: > You never used that anywhere. When I remove it the test crashes. I don't know why especially it makes no sense. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_request_params.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.cc:5: #include "components/ntp_snippets/breaking_news/subscription_request_params.h" On 2017/05/31 14:51:14, fhorschig wrote: > Looking at this rather sparse implementation ... we could inline the whole class > into the json_request class ... likely even into the anonymous namespace. > > (The original request just published this to test the request param building) Acknowledged. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_request_params.h (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.h:20: struct SubscriptionRequestParams { On 2017/05/31 14:51:14, fhorschig wrote: > that is a very, very, very simple class. > are you sure we need it at all? > It seems like we could inline the parameters as builder setters. > (The original class used more difficult param construction and had to > handle the outcome based on the result .... which we don't even parse here.) Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.h:28: // TODO(mamir): Additional fields to be added: country, language. On 2017/05/31 14:51:14, fhorschig wrote: > All these fields look easy enough to add as a Set{Country,Language,...} > into the builder. Acknowledged. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_request_params.h:31: // Callbacks for JSON parsing to allow injecting platform-dependent code. On 2017/05/31 14:51:14, fhorschig wrote: > None of which are actually used. Acknowledged.
Please, bauerb@ take another a look on a much simpler version ;-) Thanks! https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:57: /*error_details=*/"The CompletionCallback was never called"); On 2017/05/31 14:45:40, Bernhard Bauer wrote: > Is that the right error? I would return something like "cancelled". Or just not > run the callback at all -- presumably the client knows that it just cancelled > the request. PTAL https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:72: return builder; On 2017/05/31 14:45:40, Bernhard Bauer wrote: > Why not inline these calls? :) because request_context_getter_ is private. No? https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:88: builder.SetParams(params).SetUrl(GURL("http://valid-url.test")).Build(); On 2017/05/31 14:51:14, fhorschig wrote: > On 2017/05/31 14:45:40, Bernhard Bauer wrote: > > Do you actually need to call Build() here? > > I don't think so, too Done.
https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:54: LOG_IF(DFATAL, !request_completed_callback_.is_null()) On 2017/05/31 18:46:02, mamir wrote: > On 2017/05/31 14:51:12, fhorschig wrote: > > As bernhard said, this is weird (sorry). > > Could you improve that by calling any pending callback with > > a temporary error? > > (The urlfetcher is destroyed and the request cancelled here.) > > Is it OK as of now? Yes, sgtm. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:209: url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3); On 2017/05/31 18:46:02, mamir wrote: > On 2017/05/31 14:51:12, fhorschig wrote: > > Do we want to retry three times? > > What do you think? :-) I doubt it. Once should suffice. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:21: #include "components/variations/variations_params_manager.h" On 2017/05/31 18:46:03, mamir wrote: > On 2017/05/31 14:51:14, fhorschig wrote: > > There are quite some includes you wont need here: > > > > set > > test_mock_time_task_runner > > time > > tick_clock > > features > > snippets_contants? > > pref_service > > variations_params_manager > > test_url_fetcher_factory > > > > and maybe more ^^ > > Well, actually test_mock_time_task_runner is needed ;-) > > But I have cleaned the imports. Let's discuss :D https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:39: MATCHER_P(EqualsJSON, json, "equals JSON") { On 2017/05/31 18:46:03, mamir wrote: > On 2017/05/31 14:51:14, fhorschig wrote: > > Consider creating a test_helper.cc file instead of duplicating all this code. > > Could we please discuss this offline? sure https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:77: scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_; On 2017/05/31 18:46:03, mamir wrote: > On 2017/05/31 14:51:14, fhorschig wrote: > > You don't need that right now. > > I do the initialize the TestURLRequestContextGetter This is a very heavy cannon you use there. It's easier to just use the message loop. (the helpful link you look for is in another comment) https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:79: net::TestURLFetcherFactory fetcher_factory_; On 2017/05/31 18:46:03, mamir wrote: > On 2017/05/31 14:51:14, fhorschig wrote: > > You never used that anywhere. > > When I remove it the test crashes. I don't know why especially it makes no > sense. Oh, right. It replaces the factory for URLFetchers. You actually called "Build()" (even if unnecessary) which instantiates a fetcher. Would be nice to have tests to actually use that (see later PS) https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:72: return builder; On 2017/05/31 19:00:54, mamir wrote: > On 2017/05/31 14:45:40, Bernhard Bauer wrote: > > Why not inline these calls? :) > > because request_context_getter_ is private. No? If you write a getter ... GetRequestContext() { return request_context_getter.get(); } you could inline it. This makes more explicit what happens and you would have discovered, that you used ".SetUrl(GURL("http://valid-url.test"))" twice ;D https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:9: #include <vector> unused import https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:11: #include "base/command_line.h" unused import https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:15: #include "base/metrics/sparse_histogram.h" Metrics includes unused https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:17: #include "base/time/clock.h" unused https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:25: #include "components/signin/core/browser/signin_manager_base.h" unused: components/ntp_snippets/* components/signin/* https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:28: #include "components/variations/variations_associated_data.h" unused? https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:30: #include "net/http/http_response_headers.h" unused? https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:37: #include "ui/base/l10n/l10n_util.h" The last three imports seem unused as well ... please check all imports whether they are truly needed. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:39: using net::URLFetcher; either remove this or remove the net:: before every following URLFetcher. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:40: using net::URLRequestContextGetter; either remove this or remove the net:: before every following URLRequestContextGetter. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:41: using net::HttpRequestHeaders; either remove this or remove the net:: before every following HttpRequestHeaders. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:58: void SubscriptionJsonRequest::Start(CompletedCallback callback) { What happens if you call this method twice? If I didn't miss anything: - existing (not called) callbacks are dropped and overridden by the new one. - a second request is fired (which shouldn't make any sense) You can should check if a request is still running (e.g. if the callback wasn't called?) There are several things you could do here: a) queue the new callback and call it when the running request is done b) throw a temporary error ("request ongoing") c) something else I didn't think of (e.g. create a new request) I think a) would be the cleanest solution. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:74: base::StringPrintf(" %d", status.error()))); This produces really hard to debug Status states. by just printing " %d" in both failure cases, the user always gets a TEMPORARY_ERROR and a number that may or may not be HTTP error code. (why the space in " %d" btw?) https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:77: .Run(ntp_snippets::Status(ntp_snippets::StatusCode::TEMPORARY_ERROR, nit: Is TEMPORARY_ERROR what we want to return? I could imagine this being wrong for 4XX. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:80: // request succeeded. nit: This obvious comment might be obvious. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:145: request.SetString("token", token_); You access a private member (namely url_fetcher_) above. Why use a setter here? https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:149: request, base::JSONWriter::OPTIONS_PRETTY_PRINT, &request_json); Can we drop the option? (in tests, you compare by JSON anyway) https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:28: // A client can expect error_details only, if there was any error during the please update the comment https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:33: // Builds authenticated and non-authenticated SubscriptionJsonRequests. outdated comment? https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:83: // currently ongoing. That comment is a lie :D Your builder always adds a fetcher and there is no point where it is deleted. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:7: #include <set> unused https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:8: #include <utility> unused? https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:53: new net::TestURLRequestContextGetter(mock_task_runner_.get())) {} A message loop in a test should be easier: https://cs.chromium.org/chromium/src/components/doodle/doodle_fetcher_impl_un... https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:65: net::TestURLFetcherFactory fetcher_factory_; It would be nice if you used it. e.g. Test what happens if a request fails Test what happens if a request succeeds Test what happens if you start a request twice Test that an aborted request calls the callback ... (For reference, how about this: https://cs.chromium.org/chromium/src/components/doodle/doodle_fetcher_impl_un...)
https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:79: net::TestURLFetcherFactory fetcher_factory_; On 2017/06/01 09:23:55, fhorschig wrote: > On 2017/05/31 18:46:03, mamir wrote: > > On 2017/05/31 14:51:14, fhorschig wrote: > > > You never used that anywhere. > > > > When I remove it the test crashes. I don't know why especially it makes no > > sense. > > Oh, right. It replaces the factory for URLFetchers. > You actually called "Build()" (even if unnecessary) which > instantiates a fetcher. > Would be nice to have tests to actually use that (see later PS) Yeah, if you're mocking out the URLFetchers anyway, you could just send off the "network requests" and verify the request data. Then once you have more sophisticated response parsing, you could exercise that as well. https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:72: return builder; On 2017/05/31 19:00:54, mamir wrote: > On 2017/05/31 14:45:40, Bernhard Bauer wrote: > > Why not inline these calls? :) > > because request_context_getter_ is private. No? I meant `return SubscriptionJsonRequest::Builder().SetUrl().SetUrlRequestContextGetter);` – sorry, maybe "chain these calls" would have been more accurate. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:82: .Run(ntp_snippets::Status(ntp_snippets::StatusCode::SUCCESS, "")); Nit: Use an empty std::string() constructor instead of a literal to save some instructions.
Thank you for the informative and useful comments. Much appreciated! https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:54: LOG_IF(DFATAL, !request_completed_callback_.is_null()) On 2017/06/01 09:23:55, fhorschig wrote: > On 2017/05/31 18:46:02, mamir wrote: > > On 2017/05/31 14:51:12, fhorschig wrote: > > > As bernhard said, this is weird (sorry). > > > Could you improve that by calling any pending callback with > > > a temporary error? > > > (The urlfetcher is destroyed and the request cancelled here.) > > > > Is it OK as of now? > > Yes, sgtm. Acknowledged. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:209: url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3); On 2017/06/01 09:23:55, fhorschig wrote: > On 2017/05/31 18:46:02, mamir wrote: > > On 2017/05/31 14:51:12, fhorschig wrote: > > > Do we want to retry three times? > > > > What do you think? :-) > > I doubt it. Once should suffice. Done. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:21: #include "components/variations/variations_params_manager.h" On 2017/06/01 09:23:55, fhorschig wrote: > On 2017/05/31 18:46:03, mamir wrote: > > On 2017/05/31 14:51:14, fhorschig wrote: > > > There are quite some includes you wont need here: > > > > > > set > > > test_mock_time_task_runner > > > time > > > tick_clock > > > features > > > snippets_contants? > > > pref_service > > > variations_params_manager > > > test_url_fetcher_factory > > > > > > and maybe more ^^ > > > > Well, actually test_mock_time_task_runner is needed ;-) > > > > But I have cleaned the imports. > > Let's discuss :D Acknowledged. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:39: MATCHER_P(EqualsJSON, json, "equals JSON") { On 2017/06/01 09:23:55, fhorschig wrote: > On 2017/05/31 18:46:03, mamir wrote: > > On 2017/05/31 14:51:14, fhorschig wrote: > > > Consider creating a test_helper.cc file instead of duplicating all this > code. > > > > Could we please discuss this offline? > > sure Adding a TODO as per our offline discussion. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:77: scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_; On 2017/06/01 09:23:55, fhorschig wrote: > On 2017/05/31 18:46:03, mamir wrote: > > On 2017/05/31 14:51:14, fhorschig wrote: > > > You don't need that right now. > > > > I do the initialize the TestURLRequestContextGetter > > This is a very heavy cannon you use there. > It's easier to just use the message loop. > (the helpful link you look for is in another comment) Acknowledged. https://codereview.chromium.org/2918513002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:79: net::TestURLFetcherFactory fetcher_factory_; On 2017/06/01 09:31:23, Bernhard Bauer wrote: > On 2017/06/01 09:23:55, fhorschig wrote: > > On 2017/05/31 18:46:03, mamir wrote: > > > On 2017/05/31 14:51:14, fhorschig wrote: > > > > You never used that anywhere. > > > > > > When I remove it the test crashes. I don't know why especially it makes no > > > sense. > > > > Oh, right. It replaces the factory for URLFetchers. > > You actually called "Build()" (even if unnecessary) which > > instantiates a fetcher. > > Would be nice to have tests to actually use that (see later PS) > > Yeah, if you're mocking out the URLFetchers anyway, you could just send off the > "network requests" and verify the request data. Then once you have more > sophisticated response parsing, you could exercise that as well. Done. https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:72: return builder; On 2017/06/01 09:31:23, Bernhard Bauer wrote: > On 2017/05/31 19:00:54, mamir wrote: > > On 2017/05/31 14:45:40, Bernhard Bauer wrote: > > > Why not inline these calls? :) > > > > because request_context_getter_ is private. No? > > I meant `return > SubscriptionJsonRequest::Builder().SetUrl().SetUrlRequestContextGetter);` – > sorry, maybe "chain these calls" would have been more accurate. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:9: #include <vector> On 2017/06/01 09:23:56, fhorschig wrote: > unused import Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:11: #include "base/command_line.h" On 2017/06/01 09:23:55, fhorschig wrote: > unused import Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:15: #include "base/metrics/sparse_histogram.h" On 2017/06/01 09:23:56, fhorschig wrote: > Metrics includes unused Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:17: #include "base/time/clock.h" On 2017/06/01 09:23:55, fhorschig wrote: > unused Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:25: #include "components/signin/core/browser/signin_manager_base.h" On 2017/06/01 09:23:56, fhorschig wrote: > unused: > components/ntp_snippets/* > components/signin/* Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:28: #include "components/variations/variations_associated_data.h" On 2017/06/01 09:23:55, fhorschig wrote: > unused? Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:30: #include "net/http/http_response_headers.h" On 2017/06/01 09:23:56, fhorschig wrote: > unused? Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:37: #include "ui/base/l10n/l10n_util.h" On 2017/06/01 09:23:55, fhorschig wrote: > The last three imports seem unused as well ... please check all imports whether > they are truly needed. Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:39: using net::URLFetcher; On 2017/06/01 09:23:56, fhorschig wrote: > either remove this or remove the net:: before every following URLFetcher. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:40: using net::URLRequestContextGetter; On 2017/06/01 09:23:55, fhorschig wrote: > either remove this or remove the net:: before every following > URLRequestContextGetter. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:41: using net::HttpRequestHeaders; On 2017/06/01 09:23:55, fhorschig wrote: > either remove this or remove the net:: before every following > HttpRequestHeaders. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:58: void SubscriptionJsonRequest::Start(CompletedCallback callback) { On 2017/06/01 09:23:55, fhorschig wrote: > What happens if you call this method twice? > If I didn't miss anything: > - existing (not called) callbacks are dropped and overridden by the new one. > - a second request is fired (which shouldn't make any sense) > > You can should check if a request is still running (e.g. if the callback wasn't > called?) > There are several things you could do here: > a) queue the new callback and call it when the running request is done > b) throw a temporary error ("request ongoing") > c) something else I didn't think of (e.g. create a new request) > > I think a) would be the cleanest solution. Implemented option 2 after our offline discussion https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:74: base::StringPrintf(" %d", status.error()))); On 2017/06/01 09:23:56, fhorschig wrote: > This produces really hard to debug Status states. > by just printing " %d" in both failure cases, > the user always gets a TEMPORARY_ERROR and a number > that may or may not be HTTP error code. > > > (why the space in " %d" btw?) Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:77: .Run(ntp_snippets::Status(ntp_snippets::StatusCode::TEMPORARY_ERROR, On 2017/06/01 09:23:56, fhorschig wrote: > nit: Is TEMPORARY_ERROR what we want to return? > I could imagine this being wrong for 4XX. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:80: // request succeeded. On 2017/06/01 09:23:56, fhorschig wrote: > nit: This obvious comment might be obvious. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:82: .Run(ntp_snippets::Status(ntp_snippets::StatusCode::SUCCESS, "")); On 2017/06/01 09:31:23, Bernhard Bauer wrote: > Nit: Use an empty std::string() constructor instead of a literal to save some > instructions. Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:145: request.SetString("token", token_); On 2017/06/01 09:23:55, fhorschig wrote: > You access a private member (namely url_fetcher_) above. Why use a setter here? Ignore as per our offline discussion. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.cc:149: request, base::JSONWriter::OPTIONS_PRETTY_PRINT, &request_json); On 2017/06/01 09:23:55, fhorschig wrote: > Can we drop the option? > (in tests, you compare by JSON anyway) Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:28: // A client can expect error_details only, if there was any error during the On 2017/06/01 09:23:56, fhorschig wrote: > please update the comment Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:33: // Builds authenticated and non-authenticated SubscriptionJsonRequests. On 2017/06/01 09:23:56, fhorschig wrote: > outdated comment? Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request.h:83: // currently ongoing. On 2017/06/01 09:23:56, fhorschig wrote: > That comment is a lie :D > Your builder always adds a fetcher and there is no point where it is deleted. Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:7: #include <set> On 2017/06/01 09:23:56, fhorschig wrote: > unused Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:8: #include <utility> On 2017/06/01 09:23:56, fhorschig wrote: > unused? Acknowledged. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:53: new net::TestURLRequestContextGetter(mock_task_runner_.get())) {} On 2017/06/01 09:23:56, fhorschig wrote: > A message loop in a test should be easier: > https://cs.chromium.org/chromium/src/components/doodle/doodle_fetcher_impl_un... Done. https://codereview.chromium.org/2918513002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:65: net::TestURLFetcherFactory fetcher_factory_; On 2017/06/01 09:23:56, fhorschig wrote: > It would be nice if you used it. > e.g. > Test what happens if a request fails > Test what happens if a request succeeds > Test what happens if you start a request twice > Test that an aborted request calls the callback > ... > > (For reference, how about this: > https://cs.chromium.org/chromium/src/components/doodle/doodle_fetcher_impl_un...) Done.
lgtm with comments! https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.cc:27: SubscriptionJsonRequest::SubscriptionJsonRequest() : weak_ptr_factory_(this) {} Does =default work here? https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.cc:67: SubscriptionJsonRequest::Builder::Builder() {} Does =default work here, too? https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:25: // A single request to subscribe for breaking news via GCM. nit: Please mention that the Request has to stay alive in order to be successfully completed. Consider renaming it to fetcher. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:64: std::string obfuscated_gaia_id_; This property is very optional as it's never used. Remove? https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:69: void Start(CompletedCallback callback); Could you comment this please? (Something like: Starts a async request. Invokes the callback when succeeded, failed or destroyed.) https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:80: // The callback to notify when URLFetcher finished and results are available. Nit: When the request is finished/aborted/destroyed. You call in in the dtor! https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:83: base::WeakPtrFactory<SubscriptionJsonRequest> weak_ptr_factory_; unused https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:93: TEST_F(SubscriptionJsonRequestTest, BuildRequest) { Nit: It would be nice if you split this test. Besides "BuildsRequest", this also tests "InvokesCallbackWhenCancelled". https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:191: EXPECT_DCHECK_DEATH(request->Start(callback.Get())); Cool!
https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:80: url_fetcher->SetResponseString(""); Use std::string() instead of a literal. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:191: EXPECT_DCHECK_DEATH(request->Start(callback.Get())); On 2017/06/01 16:25:21, fhorschig wrote: > Cool! I feel like I missed a tech debt meeting ;-) I have to admit, I'm not a massive fan of death tests: 1) The Liskov substitution principle says you can weaken preconditions in subclasses, so the test could be more brittle than necessary. 2) I like to think that the way a class deals with violated preconditions is an implementation detail -- which to some extent is evidenced by the fact that in the test you need to do something different based on whether or not DCHECKs are enabled. 3) In a build without DCHECKs, the test is a no-op, but that's not necessarily clear to a reader, so it might feign more test coverage than actually exists. 4) Chromium uses DCHECKs to express "this can never happen", with a very strong "never". Writing code that makes it happen seems to violate that principle (i.e. it would be considered a bug).
https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.cc:27: SubscriptionJsonRequest::SubscriptionJsonRequest() : weak_ptr_factory_(this) {} On 2017/06/01 16:25:20, fhorschig wrote: > Does =default work here? Yes https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.cc:67: SubscriptionJsonRequest::Builder::Builder() {} On 2017/06/01 16:25:21, fhorschig wrote: > Does =default work here, too? Yes https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:25: // A single request to subscribe for breaking news via GCM. On 2017/06/01 16:25:21, fhorschig wrote: > nit: Please mention that the Request has to stay alive in order to > be successfully completed. Consider renaming it to fetcher. Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:64: std::string obfuscated_gaia_id_; On 2017/06/01 16:25:21, fhorschig wrote: > This property is very optional as it's never used. Remove? Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:69: void Start(CompletedCallback callback); On 2017/06/01 16:25:21, fhorschig wrote: > Could you comment this please? > (Something like: > Starts a async request. > Invokes the callback when succeeded, failed or destroyed.) Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:80: // The callback to notify when URLFetcher finished and results are available. On 2017/06/01 16:25:21, fhorschig wrote: > Nit: When the request is finished/aborted/destroyed. You call in in the dtor! Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:83: base::WeakPtrFactory<SubscriptionJsonRequest> weak_ptr_factory_; On 2017/06/01 16:25:21, fhorschig wrote: > unused Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:80: url_fetcher->SetResponseString(""); On 2017/06/01 16:54:46, Bernhard Bauer wrote: > Use std::string() instead of a literal. Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:93: TEST_F(SubscriptionJsonRequestTest, BuildRequest) { On 2017/06/01 16:25:22, fhorschig wrote: > Nit: It would be nice if you split this test. > Besides "BuildsRequest", this also tests "InvokesCallbackWhenCancelled". Done. https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:191: EXPECT_DCHECK_DEATH(request->Start(callback.Get())); On 2017/06/01 16:54:46, Bernhard Bauer wrote: > On 2017/06/01 16:25:21, fhorschig wrote: > > Cool! > > I feel like I missed a tech debt meeting ;-) > > I have to admit, I'm not a massive fan of death tests: > 1) The Liskov substitution principle says you can weaken preconditions in > subclasses, so the test could be more brittle than necessary. > 2) I like to think that the way a class deals with violated preconditions is an > implementation detail -- which to some extent is evidenced by the fact that in > the test you need to do something different based on whether or not DCHECKs are > enabled. > 3) In a build without DCHECKs, the test is a no-op, but that's not necessarily > clear to a reader, so it might feign more test coverage than actually exists. > 4) Chromium uses DCHECKs to express "this can never happen", with a very strong > "never". Writing code that makes it happen seems to violate that principle (i.e. > it would be considered a bug). Well, I heard similar feedback about using DCHECK. So Bernhard, what do you suggest to use instead of the DCHECK?
https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:191: EXPECT_DCHECK_DEATH(request->Start(callback.Get())); On 2017/06/01 17:13:01, mamir wrote: > On 2017/06/01 16:54:46, Bernhard Bauer wrote: > > On 2017/06/01 16:25:21, fhorschig wrote: > > > Cool! > > > > I feel like I missed a tech debt meeting ;-) > > > > I have to admit, I'm not a massive fan of death tests: > > 1) The Liskov substitution principle says you can weaken preconditions in > > subclasses, so the test could be more brittle than necessary. > > 2) I like to think that the way a class deals with violated preconditions is > an > > implementation detail -- which to some extent is evidenced by the fact that in > > the test you need to do something different based on whether or not DCHECKs > are > > enabled. > > 3) In a build without DCHECKs, the test is a no-op, but that's not necessarily > > clear to a reader, so it might feign more test coverage than actually exists. > > 4) Chromium uses DCHECKs to express "this can never happen", with a very > strong > > "never". Writing code that makes it happen seems to violate that principle > (i.e. > > it would be considered a bug). > > Well, I heard similar feedback about using DCHECK. > So Bernhard, what do you suggest to use instead of the DCHECK? As discussed off-review, I would leave out this test completely (if necessary, test that _clients_ don't start a request more than once), and keep the DCHECK in the implementation. https://codereview.chromium.org/2918513002/diff/120001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/120001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:67: // Starts a async request. The callback is invoked when the request succeeds, Tiny nit: "an async request" :)
https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:191: EXPECT_DCHECK_DEATH(request->Start(callback.Get())); On 2017/06/01 16:54:46, Bernhard Bauer wrote: > On 2017/06/01 16:25:21, fhorschig wrote: > > Cool! > > I feel like I missed a tech debt meeting ;-) Actually, death tests are on next week's meeting agenda and I didn't even know them before. > I have to admit, I'm not a massive fan of death tests: > 1) The Liskov substitution principle says you can weaken preconditions in > subclasses, so the test could be more brittle than necessary. > 2) I like to think that the way a class deals with violated preconditions is an > implementation detail -- which to some extent is evidenced by the fact that in > the test you need to do something different based on whether or not DCHECKs are > enabled. > 3) In a build without DCHECKs, the test is a no-op, but that's not necessarily > clear to a reader, so it might feign more test coverage than actually exists. > 4) Chromium uses DCHECKs to express "this can never happen", with a very strong > "never". Writing code that makes it happen seems to violate that principle (i.e. > it would be considered a bug). So I am just going to link/steal your comment, because these arguments are pretty compelling.
https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc (right): https://codereview.chromium.org/2918513002/diff/100001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc:191: EXPECT_DCHECK_DEATH(request->Start(callback.Get())); On 2017/06/02 08:13:40, fhorschig wrote: > On 2017/06/01 16:54:46, Bernhard Bauer wrote: > > On 2017/06/01 16:25:21, fhorschig wrote: > > > Cool! > > > > I feel like I missed a tech debt meeting ;-) > > Actually, death tests are on next week's meeting agenda and I didn't > even know them before. > > > I have to admit, I'm not a massive fan of death tests: > > 1) The Liskov substitution principle says you can weaken preconditions in > > subclasses, so the test could be more brittle than necessary. > > 2) I like to think that the way a class deals with violated preconditions is > an > > implementation detail -- which to some extent is evidenced by the fact that in > > the test you need to do something different based on whether or not DCHECKs > are > > enabled. > > 3) In a build without DCHECKs, the test is a no-op, but that's not necessarily > > clear to a reader, so it might feign more test coverage than actually exists. > > 4) Chromium uses DCHECKs to express "this can never happen", with a very > strong > > "never". Writing code that makes it happen seems to violate that principle > (i.e. > > it would be considered a bug). > > So I am just going to link/steal your comment, because these arguments > are pretty compelling. Done. https://codereview.chromium.org/2918513002/diff/120001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_json_request.h (right): https://codereview.chromium.org/2918513002/diff/120001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_json_request.h:67: // Starts a async request. The callback is invoked when the request succeeds, On 2017/06/01 20:34:51, Bernhard Bauer wrote: > Tiny nit: "an async request" :) Done.
Thanks! LGTM.
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fhorschig@chromium.org Link to the patchset: https://codereview.chromium.org/2918513002/#ps140001 (title: "Minor comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496409372253540,
"parent_rev": "26f0196082493c3d5109f196828a8c5baf16005a", "commit_rev":
"a19c77077f663f771e822ffb025d9cc607e9cf8e"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Push] Add the classes for sending a breaking news subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=728119 ========== to ========== [NTP::Push] Add the classes for sending a breaking news subscription request Add the classes for sending a gcm subscription request to the content suggestion server. They are required to be able to later deliver breaking news notifications via GCM. BUG=728119 Review-Url: https://codereview.chromium.org/2918513002 Cr-Commit-Position: refs/heads/master@{#476647} Committed: https://chromium.googlesource.com/chromium/src/+/a19c77077f663f771e822ffb025d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a19c77077f663f771e822ffb025d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
