Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505643002:
Concurrent Snippet Fetches. (Closed)
DescriptionConcurrent Snippet Fetches.
Multiple concurrent fetch processes ensure that the UI will always get
a correct response for a |Fetch| request.
In order to bind fetcher and callback to the request, a new |Request|
class provides another layer of abstraction.
(No tests were deleted. The test "ShouldCancelOngoingFetch" is not part of the use case anymore and was replaced with "ShouldProcessConcurrentFetches")
BUG=659931
Committed: https://crrev.com/7530d98e8559cf6f598b57a073d19cf7470ba752
Cr-Commit-Position: refs/heads/master@{#436280}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removing the remote_suggestions_provider from CL. #Patch Set 3 : Moved Request into NTPSnippetsFetcher. #Patch Set 4 : Reordered. #
Total comments: 18
Patch Set 5 : Request/Builder are private class of Fetcher. Request doesn't handle all callbacks. #
Total comments: 78
Patch Set 6 : Rebased. Reworked parts of the JSON handling. #
Total comments: 42
Patch Set 7 : Rebased. Reordered definitions to match declarations. #
Total comments: 19
Patch Set 8 : Renaming the Handler to JSONParser. #
Total comments: 22
Patch Set 9 : Tested and Fixed AuthHeader. #Patch Set 10 : Stored last fetch directly into member. #Patch Set 11 : Used base::Passed for callback parameters. #
Total comments: 8
Patch Set 12 : Request is parsing JSON now. #
Total comments: 8
Patch Set 13 : Removed alias for Callback-Builder-Pair. Fixed leaking test. #
Total comments: 28
Patch Set 14 : Unpaired Builder and Callback where it wasn't needed. #
Total comments: 6
Patch Set 15 : Addressing nits. #
Total comments: 12
Patch Set 16 : Hiding the request's URLFetcher. #
Total comments: 8
Patch Set 17 : Explanatory comments. #
Dependent Patchsets: Messages
Total messages: 60 (26 generated)
Description was changed from ========== Concurrent Snippet Fetches. Multiple concurrent fetch processes ensure that the UI will always get a correct response for a |Fetch| request. In order to bind fetcher and callback to the request, a new |Request| class provides another layer of abstraction. BUG=659931 ========== to ========== Concurrent Snippet Fetches. Multiple concurrent fetch processes ensure that the UI will always get a correct response for a |Fetch| request. In order to bind fetcher and callback to the request, a new |Request| class provides another layer of abstraction. (No tests were deleted. The test "ShouldCancelOngoingFetch" is not part of the use case anymore and was replaced with "ShouldProcessConcurrentFetches") BUG=659931 ==========
Patchset #1 (id:1) has been deleted
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
a few overall comments https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:399: parse_json_callback_.Run(last_fetch_json_, success_callback, why does the snippets-fetcher have to execute the parse_json_callback? Couldn't this be done inside the request? Seems awkward to double-dispatch in-and-out again of the fetcher... Also, storing last_fetch_json_ as a member doesn't really work anymore, no? Why not have this on the request object as well? https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:463: void NTPSnippetsFetcher::OnJsonParsed(std::unique_ptr<Request> request, with the new flow, you should be able to combine OnJSonParsed and OnJsonError into a single OnFetchCompleted() method. If the status is SUCCESS, then the code would turn the parsed value into snippets and deliver it. If the status is an error, then that error gets simply propagated to the client. https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:481: base::WeakPtr<Request> request) { why is this a weak-ptr? The snippets fetcher created the object and should be responsible for deleting it again... https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:216: void OnFetchFinished(Request::OptionalFetchedCategories fetched_categories); let's try to keep this change internal to the snippets fetcher and not have impact on any of it's clients. Those shouldn't depend on implementation details of the fetcher (and the request object is one of those). https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/request.h (right): https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/request.h:39: class Request : public net::URLFetcherDelegate { can we treat the request as an implementation detail of the snippets fetcher? Ideally we could just define it inside the snippet fetcher's .cc file but if necessary we can also pull out a separate library -- just name it a bit more specific. For example the FetchedCategory (and other data structures getting exposed to clients of the snippets fetcher) should not be defined in here.
Patchset #4 (id:80001) has been deleted
(Complete review not necessary yet.) Hi Tim, thanks for the feedback, I have addressed your comments but I am not completely ready yet (See own comments). The CL is now half the size :D Cheers, Friedrich https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:399: parse_json_callback_.Run(last_fetch_json_, success_callback, On 2016/11/15 19:43:50, tschumann wrote: > why does the snippets-fetcher have to execute the parse_json_callback? > Couldn't this be done inside the request? Seems awkward to double-dispatch > in-and-out again of the fetcher... > > Also, storing last_fetch_json_ as a member doesn't really work anymore, no? Why > not have this on the request object as well? I moved the request handling to a new class. The problem is, that the Callback that the Parser uses MUST be a base::Callback. base::Callback doesn't allow to bind a unique_ptr (obviously). I will consider to move the parsing and FetchFinish to the request tomorrow. (I am not pretty sure, that the parsing and error handling should be the responsibility of the NTPSnippetsFetcher) https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:463: void NTPSnippetsFetcher::OnJsonParsed(std::unique_ptr<Request> request, On 2016/11/15 19:43:50, tschumann wrote: > with the new flow, you should be able to combine OnJSonParsed and OnJsonError > into a single OnFetchCompleted() method. > If the status is SUCCESS, then the code would turn the parsed value into > snippets and deliver it. If the status is an error, then that error gets simply > propagated to the client. Done. (Simple, refactoring might follow) https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:481: base::WeakPtr<Request> request) { On 2016/11/15 19:43:50, tschumann wrote: > why is this a weak-ptr? The snippets fetcher created the object and should be > responsible for deleting it again... It used to be unclear which path was chosen: Error or Success. Now, there is one method and this is gone. https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:216: void OnFetchFinished(Request::OptionalFetchedCategories fetched_categories); On 2016/11/15 19:43:50, tschumann wrote: > let's try to keep this change internal to the snippets fetcher and not have > impact on any of it's clients. Those shouldn't depend on implementation details > of the fetcher (and the request object is one of those). Done. https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/request.h (right): https://codereview.chromium.org/2505643002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/request.h:39: class Request : public net::URLFetcherDelegate { On 2016/11/15 19:43:50, tschumann wrote: > can we treat the request as an implementation detail of the snippets fetcher? > Ideally we could just define it inside the snippet fetcher's .cc file but if > necessary we can also pull out a separate library -- just name it a bit more > specific. > > For example the FetchedCategory (and other data structures getting exposed to > clients of the snippets fetcher) should not be defined in here. Done. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:603: NTPSnippetsFetcher::NTPSnippetsFetcher( move up for less confusing review. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:933: void NTPSnippetsFetcher::OnJsonParsingCompleted( Handling based on FetchResult? https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:337: void FetchFinished(Request* request, This can either use std::unique_ptr<Request> now or be moved completely to Request.
the overall design is still not 100% there. left a few comments. When they are addressed feel free to get an lgtm from somebody else to make the BP. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:431: void NTPSnippetsFetcher::Request::AddAuthentication( this use of the builder pattern seems backwards. The request object should get built by the builder and not own one. What you should do instead: The SnippetsFetcher creates the builder; in case that a token request is necessary, puts the builder on a list of builders (pending_requests_). Later, we the snippets fetcher can pick it up from there, set up the authentication data and then call Build() which gives a request. On this request, the snippets fetcher can then call Start(). The request finishes when an error occurs or we successfully got the json. The snippets fetcher takes over from there, builds the FetchedCategory objects and passes them back to the caller. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:728: request->fetcher()->SetRequestContext(url_request_context_getter_.get()); why don't we do this inside Request::Start()? just hand in the url_request_context_getter_, too? https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:809: void NTPSnippetsFetcher::CompleteRequest(std::unique_ptr<Request> request) { What about making the request's responsibility stop after getting the JSON and the snippets-fetcher takes care from here: Parses the json into the result and calls the snippets_available_callback. Actually, i like this best. It's also nice and symmetric: you call Start() on the Request and the callback passed into gets called when the request is done. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:983: if (request) { no need for the if. you DCHECK() before. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:82: // Error details can be retrieved using last_status(). can you add a TODO() to get rid of the last_status() call? it doesn't fit the concurrent usage anymore and SnippetsAvailableCallback should simply take an status code. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:191: class Request : public net::URLFetcherDelegate { can you move this class definition inside the .cc file? forward-declaring it in the header might be sufficient. It's bloating up the header quite a bit and the nesting inside the NTPSnippetsFetcher class makes the names quite unwieldy. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:337: void FetchFinished(Request* request, On 2016/11/16 18:31:17, fhorschig wrote: > This can either use std::unique_ptr<Request> now or be moved completely to > Request. IMO, the request should not call the snippets_availalble callback. Pass the 'request' as const reference. If the request also calls the snippets_available callback the flow is pretty convoluted.
fhorschig@chromium.org changed reviewers: + treib@chromium.org
The responsibilities should now be where they belong. @treib: Could you please take a look? https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:431: void NTPSnippetsFetcher::Request::AddAuthentication( On 2016/11/17 01:07:32, tschumann wrote: > this use of the builder pattern seems backwards. > The request object should get built by the builder and not own one. > > What you should do instead: The SnippetsFetcher creates the builder; in case > that a token request is necessary, puts the builder on a list of builders > (pending_requests_). Later, we the snippets fetcher can pick it up from there, > set up the authentication data and then call Build() which gives a request. On > this request, the snippets fetcher can then call Start(). The request finishes > when an error occurs or we successfully got the json. The snippets fetcher takes > over from there, builds the FetchedCategory objects and passes them back to the > caller. The "builder" the Request owns builds only part of the request. But I agree that a builder for the request makes sense ... and I deleted the BodyBuilder entirely. I probably hadn't thought this through, so thanks for pointing that out! https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:603: NTPSnippetsFetcher::NTPSnippetsFetcher( On 2016/11/16 18:31:17, fhorschig wrote: > move up for less confusing review. Done. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:728: request->fetcher()->SetRequestContext(url_request_context_getter_.get()); On 2016/11/17 01:07:32, tschumann wrote: > why don't we do this inside Request::Start()? > just hand in the url_request_context_getter_, too? Done. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:809: void NTPSnippetsFetcher::CompleteRequest(std::unique_ptr<Request> request) { On 2016/11/17 01:07:32, tschumann wrote: > What about making the request's responsibility stop after getting the JSON and > the snippets-fetcher takes care from here: Parses the json into the result and > calls the snippets_available_callback. Actually, i like this best. > It's also nice and symmetric: you call Start() on the Request and the callback > passed into gets called when the request is done. > Done. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:933: void NTPSnippetsFetcher::OnJsonParsingCompleted( On 2016/11/16 18:31:17, fhorschig wrote: > Handling based on FetchResult? Not in this CL. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:983: if (request) { On 2016/11/17 01:07:32, tschumann wrote: > no need for the if. you DCHECK() before. Done. https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:191: class Request : public net::URLFetcherDelegate { On 2016/11/17 01:07:32, tschumann wrote: > can you move this class definition inside the .cc file? > forward-declaring it in the header might be sufficient. > > It's bloating up the header quite a bit and the nesting inside the > NTPSnippetsFetcher class makes the names quite unwieldy. Done. As far as I understand, the unwieldy names are not avoidable if we try to hide this implementation detail. (If we wouldn't want to hide that, I would prefer the solution in the first Patchset) https://codereview.chromium.org/2505643002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:337: void FetchFinished(Request* request, On 2016/11/17 01:07:32, tschumann wrote: > On 2016/11/16 18:31:17, fhorschig wrote: > > This can either use std::unique_ptr<Request> now or be moved completely to > > Request. > > IMO, the request should not call the snippets_availalble callback. Pass the > 'request' as const reference. > If the request also calls the snippets_available callback the flow is pretty > convoluted. I agree that the request shouldn't call the callback. But the request holds the callback[1], which needs to be consumed on call. Therefore, a const reference is not viable and passing a unique_ptr rather than a raw pointer makes clear that "FetchFinished consumes the request". [1] and important calling parameters until the overfetching is gone
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:257: // categories. If only fetches for a single category were requested, this s/If only fetches for a single category were requested/If only a single category was requested/ https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:276: categories->emplace_back(std::move(category)); nit: This could just be push_back https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:283: if (!callback.is_null()) Don't handle DCHECK failures. If the callback can be null, remove the DCHECK. Otherwise, remove the if. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:292: Request(const base::Optional<Category>& exclusive_category, By value? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:297: using RequestCompletedCallback = base::OnceCallback<void(void)>; OnceClosure https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:301: SnippetsAvailableCallback snippets_available_callback() { Please rename this - it looks like it's a simple getter, but it isn't. ExtractSnippetsAvailableCallback maybe? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:306: base::Optional<Category> exclusive_category() const { This copies - intended? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:312: const URLFetcher* fetcher() const { return url_fetcher_.get(); } nit: url_fetcher for consistency https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:334: // The callback to notify when new snippets get fetched. ...so what's the other callback for? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:345: std::unique_ptr<base::Value>, nit: Add a name? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:354: // unique_ptr of the instance. I don't understand what this comment is telling me..? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:465: std::string personalization = variations::GetVariationParamValue( Err.. pretty sure this shouldn't be in the dtor? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:520: pending_requests_.push_front(std::move(request_builder)); So if we're already waiting for a refresh token, the request will just get dropped? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:636: } Heads-up: Vitalii recently added braces for all single-line ifs/fors. You'll probably have to rebase. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:678: LOG(DFATAL) << "The SnippetsAvailableCallback was never called!"; LOG_IF https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:682: bool NTPSnippetsFetcher::RequestBuilder::UsesAuthentication() const { This is only called from NTPSnippetsFetcher; it's not used at all in the RequestBuilder. So maybe it should stay on NTPSnippetsFetcher? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:699: if (!auth_header.empty()) { Can this ever get called with an empty header? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:828: << headers << std::endl nit: std::endl enforces a flush. Might as well just use "\n" here. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:873: LOG(DFATAL) << "JsonParser called callback without pending request."; Do we expect this to ever actually happen? If not, just DCHECK and don't handle the error case. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:885: .Run(std::move(result), std::string(), std::move(request_)); nit: add a /*name=*/ for the empty string? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:893: std::move(completion_callback_).Run(nullptr, error, std::move(request_)); Similar here: name for the nullptr? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:921: base::Unretained(this), std::move(request))); Are you sure this works? You're moving out request while calling a method on it. I'm not sure in which order this will get executed. Might be safer to first get a raw pointer to the request, and then call Start(..) on that? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:994: DCHECK(request->fetcher()); nit: These two don't really add anything - if either is null, we'll just crash on the next line anyway. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1006: LOG(ERROR) << "It's High Noon!"; :) https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1016: LOG(ERROR) << "Heroes never die!"; ^^ https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1021: void NTPSnippetsFetcher::RunParseJsonCallback( Should this be called just ParseJson? That callbacks are involved is really an implementation detail. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1023: std::string last_fetch_json; Please rename this - fetched_json? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1039: raw_callback_handler->CreateErrorCallback()); This is all quite intricate, and kinda hard to follow... would it be easier to just pass the parse_json_callback_ to the handler? Copying callbacks is cheap. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1122: request->fetcher()->GetResponseAsString(&last_fetch_json_); Is this the right place? Couldn't this stay in RunParseJsonCallback above? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:196: const std::string)>; const std::string& Also, please add names, especially for the string https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:203: RequestBuilder(const Personalization& personalization, nit: This is just an enum, could pass this by value. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:203: RequestBuilder(const Personalization& personalization, nit: This is just an enum, could pass this by value. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:204: const base::TimeTicks& creation_time, Same here, this is essentially an int. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:214: RequestBuilder& AddAuthentication(const std::string& account_id); What's the difference between "Add" and "Set" here? Should they all be "Set"? (If there is actually a difference, please add a comment to explain) https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:217: RequestBuilder& SetFetchAPI(const FetchAPI& fetch_api); Also here: enum -> by value is fine https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:243: std::unique_ptr<net::URLFetcher> BuildUrlFetcher( nit: BuildURLFetcher, to be consistent with URLFetcher? https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:256: FetchAPI fetch_api_; Seems like the 3 above are really mandatory?! https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:271: class RequestBuilderWithMockedFlagsForTesting : public RequestBuilder { Can we move this to the test? If you make the test class a friend, it should be possible to define this in there. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:301: class NTPSnippetsFetcherRequestBuilderTest What's the conceptual difference between this and the base class? Since the base class isn't actually used, should the two just be merged?
Patchset #6 (id:160001) has been deleted
Thanks for your comments! You found some serious bugs. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:257: // categories. If only fetches for a single category were requested, this On 2016/11/22 10:02:00, Marc Treib wrote: > s/If only fetches for a single category were requested/If only a single category > was requested/ Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:276: categories->emplace_back(std::move(category)); On 2016/11/22 10:02:00, Marc Treib wrote: > nit: This could just be push_back Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:283: if (!callback.is_null()) On 2016/11/22 10:02:00, Marc Treib wrote: > Don't handle DCHECK failures. If the callback can be null, remove the DCHECK. > Otherwise, remove the if. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:292: Request(const base::Optional<Category>& exclusive_category, On 2016/11/22 10:02:01, Marc Treib wrote: > By value? Okay. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:297: using RequestCompletedCallback = base::OnceCallback<void(void)>; On 2016/11/22 10:02:01, Marc Treib wrote: > OnceClosure Nice! https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:301: SnippetsAvailableCallback snippets_available_callback() { On 2016/11/22 10:02:01, Marc Treib wrote: > Please rename this - it looks like it's a simple getter, but it isn't. > ExtractSnippetsAvailableCallback maybe? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:306: base::Optional<Category> exclusive_category() const { On 2016/11/22 10:02:01, Marc Treib wrote: > This copies - intended? Done. Not intended. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:312: const URLFetcher* fetcher() const { return url_fetcher_.get(); } On 2016/11/22 10:02:01, Marc Treib wrote: > nit: url_fetcher for consistency Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:334: // The callback to notify when new snippets get fetched. On 2016/11/22 10:02:01, Marc Treib wrote: > ...so what's the other callback for? Changed the heavily deprecated comments. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:345: std::unique_ptr<base::Value>, On 2016/11/22 10:02:01, Marc Treib wrote: > nit: Add a name? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:354: // unique_ptr of the instance. On 2016/11/22 10:02:01, Marc Treib wrote: > I don't understand what this comment is telling me..? The problem used to describe how this function is used. This is not a useful thing to do in a class declaration. I changed it and added a DCHECK that ensures that this function can not simply drop callbacks which hold references. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:465: std::string personalization = variations::GetVariationParamValue( On 2016/11/22 10:02:00, Marc Treib wrote: > Err.. pretty sure this shouldn't be in the dtor? Oh wow. Thank you for noticing that. This could have become very ugly. I added a test for this as this is incredibly hard to debug when missing. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:520: pending_requests_.push_front(std::move(request_builder)); On 2016/11/22 10:02:00, Marc Treib wrote: > So if we're already waiting for a refresh token, the request will just get > dropped? Yes, this was very wrong. Changed. (I am writing some tests for the authenticated workflow, but this might take a while.) https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:636: } On 2016/11/22 10:02:00, Marc Treib wrote: > Heads-up: Vitalii recently added braces for all single-line ifs/fors. You'll > probably have to rebase. Acknowledged. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:678: LOG(DFATAL) << "The SnippetsAvailableCallback was never called!"; On 2016/11/22 10:02:01, Marc Treib wrote: > LOG_IF nice! https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:682: bool NTPSnippetsFetcher::RequestBuilder::UsesAuthentication() const { On 2016/11/22 10:02:00, Marc Treib wrote: > This is only called from NTPSnippetsFetcher; it's not used at all in the > RequestBuilder. So maybe it should stay on NTPSnippetsFetcher? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:699: if (!auth_header.empty()) { On 2016/11/22 10:02:00, Marc Treib wrote: > Can this ever get called with an empty header? Removed if, as it is very unlikely. Whenever it's called, it's probably intentional. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:828: << headers << std::endl On 2016/11/22 10:02:00, Marc Treib wrote: > nit: std::endl enforces a flush. Might as well just use "\n" here. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:873: LOG(DFATAL) << "JsonParser called callback without pending request."; On 2016/11/22 10:02:01, Marc Treib wrote: > Do we expect this to ever actually happen? If not, just DCHECK and don't handle > the error case. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:885: .Run(std::move(result), std::string(), std::move(request_)); On 2016/11/22 10:02:01, Marc Treib wrote: > nit: add a /*name=*/ for the empty string? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:893: std::move(completion_callback_).Run(nullptr, error, std::move(request_)); On 2016/11/22 10:02:00, Marc Treib wrote: > Similar here: name for the nullptr? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:921: base::Unretained(this), std::move(request))); On 2016/11/22 10:02:00, Marc Treib wrote: > Are you sure this works? You're moving out request while calling a method on it. > I'm not sure in which order this will get executed. Might be safer to first get > a raw pointer to the request, and then call Start(..) on that? Absolutely correct. Weird, that it worked. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:994: DCHECK(request->fetcher()); On 2016/11/22 10:02:01, Marc Treib wrote: > nit: These two don't really add anything - if either is null, we'll just crash > on the next line anyway. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1006: LOG(ERROR) << "It's High Noon!"; On 2016/11/22 10:02:01, Marc Treib wrote: > :) Ah, my debug messages. Sorry. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1016: LOG(ERROR) << "Heroes never die!"; On 2016/11/22 10:02:00, Marc Treib wrote: > ^^ Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1021: void NTPSnippetsFetcher::RunParseJsonCallback( On 2016/11/22 10:02:01, Marc Treib wrote: > Should this be called just ParseJson? That callbacks are involved is really an > implementation detail. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1023: std::string last_fetch_json; On 2016/11/22 10:02:01, Marc Treib wrote: > Please rename this - fetched_json? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1039: raw_callback_handler->CreateErrorCallback()); On 2016/11/22 10:02:00, Marc Treib wrote: > This is all quite intricate, and kinda hard to follow... would it be easier to > just pass the parse_json_callback_ to the handler? Copying callbacks is cheap. I did that. It's more explicit now. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1122: request->fetcher()->GetResponseAsString(&last_fetch_json_); On 2016/11/22 10:02:01, Marc Treib wrote: > Is this the right place? Couldn't this stay in RunParseJsonCallback above? Moved this into the handler class which controls the callback now. The way it is done here is not exactly thread safe as a more recent request would set the member variable, which caused the newer error to be printed twice. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:196: const std::string)>; On 2016/11/22 10:02:01, Marc Treib wrote: > const std::string& > > Also, please add names, especially for the string Deleted. It was not even used anymore. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:203: RequestBuilder(const Personalization& personalization, On 2016/11/22 10:02:01, Marc Treib wrote: > nit: This is just an enum, could pass this by value. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:204: const base::TimeTicks& creation_time, On 2016/11/22 10:02:01, Marc Treib wrote: > Same here, this is essentially an int. Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:214: RequestBuilder& AddAuthentication(const std::string& account_id); On 2016/11/22 10:02:01, Marc Treib wrote: > What's the difference between "Add" and "Set" here? Should they all be "Set"? > (If there is actually a difference, please add a comment to explain) Renamed to Set. The "Add" methods were more complex, depend (partly) on flags to do their work and set other internal variables as well. But as they have no other side effects (and the one set internal variable can't even be set otherwise), it is way less confusing to name them all "Set". https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:217: RequestBuilder& SetFetchAPI(const FetchAPI& fetch_api); On 2016/11/22 10:02:01, Marc Treib wrote: > Also here: enum -> by value is fine Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:243: std::unique_ptr<net::URLFetcher> BuildUrlFetcher( On 2016/11/22 10:02:01, Marc Treib wrote: > nit: BuildURLFetcher, to be consistent with URLFetcher? Done. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:256: FetchAPI fetch_api_; On 2016/11/22 10:02:01, Marc Treib wrote: > Seems like the 3 above are really mandatory?! Not always. There is always a request, even if it is not sent to the server. These properties are only necessary, if we actually fetch something. I added comments to clarify exactly that difference and a |SparseBuild|, so |Build| can use DCHECKs to verify the presence of these properties. https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:271: class RequestBuilderWithMockedFlagsForTesting : public RequestBuilder { On 2016/11/22 10:02:01, Marc Treib wrote: > Can we move this to the test? If you make the test class a friend, it should be > possible to define this in there. THANK YOU! (I feel pretty stupid as I never even considered to make it a inner class of the test and struggled a lot trying to implement this class directly in the ntp_snippets namespace.) https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:301: class NTPSnippetsFetcherRequestBuilderTest On 2016/11/22 10:02:01, Marc Treib wrote: > What's the conceptual difference between this and the base class? Since the base > class isn't actually used, should the two just be merged? Merged, thanks for pointing that out. The former main difference is gone.
A bunch more comments, but much smaller ones :) https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1122: request->fetcher()->GetResponseAsString(&last_fetch_json_); On 2016/11/22 16:04:48, fhorschig wrote: > On 2016/11/22 10:02:01, Marc Treib wrote: > > Is this the right place? Couldn't this stay in RunParseJsonCallback above? > > Moved this into the handler class which controls the callback now. The way it is > done here is not exactly thread safe as a more recent request would set the > member variable, which caused the newer error to be printed twice. Thread safety isn't a problem here - all this runs on one thread. So the problem must've been something else..? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:283: DCHECK(!callback.is_null()); Now the DCHECK doesn't really do anything anymore - if the callback is null, it'll just crash in the next line. And in fact, this function isn't really useful anymore IMO - maybe just inline it where it's called? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:332: // The callback to notify when request returned. nit: Which "request"? This is the Request class :) https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:359: void SetCompletionCallback(CompletionCallback callback) { Can we pass this to Run() and remove the setter? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:365: void SetParsingCallback(ParseJSONCallback callback) { Can we pass this into the ctor and remove the setter? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:383: // a JSON parser. Comment doesn't apply https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:549: url_(std::string()), This doesn't do anything useful. Just leave it out, and rely on GURLs default ctor. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:551: obfuscated_gaia_id_(std::string()), Also here: Just leave it out https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:553: user_class_(std::string()), And here https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:554: language_model_(nullptr) {} This one is important though :) https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:688: bool NTPSnippetsFetcher::UsesAuthentication() const { Move this now to match the order in the header. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:712: DCHECK(user_classifier); If the param can't be null, make it a reference instead of a pointer? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:835: request->SetUrlFetcher(std::move(url_fetcher)); nit: Move the BuildURLFetcher call in here and get rid of the url_fetcher variable? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:873: DCHECK(!completion_callback_.is_null()); This isn't really necessary - if the callback is null, we'll just crash on the next line, so the DCHECK doesn't help. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:880: DCHECK(!completion_callback_.is_null()); Same here https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1015: void NTPSnippetsFetcher::ParseJson(std::unique_ptr<Request> request) { I know I'm contradicting myself now :D, but maybe ParseJsonResponse (or just ParseResponse)? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:69: const ErrorCallback& error_callback)>; FTR, here we would've been fine without the names (but it's also fine to leave them in). https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:213: RequestBuilder& SetAuthenticationHeader(const std::string& auth_header); Now that I see these next to each other: SetAuthentication and SetAuthenticationHeader is a bit confusing. Maybe SetAuthentication needs a better name. Or alternatively, does it ever make sense to set one of these without the other? If not, maybe merge them? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:321: std::list<RequestBuilder> pending_requests_; This really stores the requests that are waiting for an access token, right? Add a comment, and/or rename? Also, should this be an std::queue? IMO that'd make it a bit easier to read. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:199: // TODO(fhorschig): As soon as crbug.com/645447, use nit: As soon as the bug what? :) https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:221: : params_manager_(new variations::testing::VariationParamsManager( base::MakeUnique, to make clear what's going on? https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:285: void SetFetchingPersionalizationVariation( typo
Patchset #7 (id:200001) has been deleted
All annotations addressed. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:283: DCHECK(!callback.is_null()); On 2016/11/22 17:46:43, Marc Treib wrote: > Now the DCHECK doesn't really do anything anymore - if the callback is null, > it'll just crash in the next line. > > And in fact, this function isn't really useful anymore IMO - maybe just inline > it where it's called? Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:332: // The callback to notify when request returned. On 2016/11/22 17:46:43, Marc Treib wrote: > nit: Which "request"? This is the Request class :) Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:359: void SetCompletionCallback(CompletionCallback callback) { On 2016/11/22 17:46:43, Marc Treib wrote: > Can we pass this to Run() and remove the setter? Okay. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:365: void SetParsingCallback(ParseJSONCallback callback) { On 2016/11/22 17:46:43, Marc Treib wrote: > Can we pass this into the ctor and remove the setter? Okay. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:383: // a JSON parser. On 2016/11/22 17:46:43, Marc Treib wrote: > Comment doesn't apply Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:549: url_(std::string()), On 2016/11/22 17:46:43, Marc Treib wrote: > This doesn't do anything useful. Just leave it out, and rely on GURLs default > ctor. Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:551: obfuscated_gaia_id_(std::string()), On 2016/11/22 17:46:43, Marc Treib wrote: > Also here: Just leave it out Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:553: user_class_(std::string()), On 2016/11/22 17:46:43, Marc Treib wrote: > And here Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:554: language_model_(nullptr) {} On 2016/11/22 17:46:43, Marc Treib wrote: > This one is important though :) haha, thanks :D https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:688: bool NTPSnippetsFetcher::UsesAuthentication() const { On 2016/11/22 17:46:43, Marc Treib wrote: > Move this now to match the order in the header. Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:712: DCHECK(user_classifier); On 2016/11/22 17:46:43, Marc Treib wrote: > If the param can't be null, make it a reference instead of a pointer? Good idea. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:835: request->SetUrlFetcher(std::move(url_fetcher)); On 2016/11/22 17:46:43, Marc Treib wrote: > nit: Move the BuildURLFetcher call in here and get rid of the url_fetcher > variable? Thanks. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:873: DCHECK(!completion_callback_.is_null()); On 2016/11/22 17:46:43, Marc Treib wrote: > This isn't really necessary - if the callback is null, we'll just crash on the > next line, so the DCHECK doesn't help. Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:880: DCHECK(!completion_callback_.is_null()); On 2016/11/22 17:46:43, Marc Treib wrote: > Same here Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1015: void NTPSnippetsFetcher::ParseJson(std::unique_ptr<Request> request) { On 2016/11/22 17:46:43, Marc Treib wrote: > I know I'm contradicting myself now :D, but maybe ParseJsonResponse (or just > ParseResponse)? Okay. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:69: const ErrorCallback& error_callback)>; On 2016/11/22 17:46:43, Marc Treib wrote: > FTR, here we would've been fine without the names (but it's also fine to leave > them in). Acknowledged. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:213: RequestBuilder& SetAuthenticationHeader(const std::string& auth_header); On 2016/11/22 17:46:43, Marc Treib wrote: > Now that I see these next to each other: SetAuthentication and > SetAuthenticationHeader is a bit confusing. Maybe SetAuthentication needs a > better name. Or alternatively, does it ever make sense to set one of these > without the other? If not, maybe merge them? Merged. (I liked the explicit way of setting one at a time but there I don't know case where you would need only one.) https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:321: std::list<RequestBuilder> pending_requests_; On 2016/11/22 17:46:43, Marc Treib wrote: > This really stores the requests that are waiting for an access token, right? Add > a comment, and/or rename? > > Also, should this be an std::queue? IMO that'd make it a bit easier to read. True. Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:199: // TODO(fhorschig): As soon as crbug.com/645447, use On 2016/11/22 17:46:44, Marc Treib wrote: > nit: As soon as the bug what? :) Done. https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:221: : params_manager_(new variations::testing::VariationParamsManager( On 2016/11/22 17:46:44, Marc Treib wrote: > base::MakeUnique, to make clear what's going on? Done. (For every unique_ptr member) https://codereview.chromium.org/2505643002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:285: void SetFetchingPersionalizationVariation( On 2016/11/22 17:46:44, Marc Treib wrote: > typo Done.
https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:346: ParseJSONCallback callback); const& https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:367: // This callback is called by a Js ? https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:559: NTPSnippetsFetcher::JsonCallbackHandler::JsonCallbackHandler( Should we call this JsonParser or something like that? That's what it does, the whole callback mangling is exactly the implementation detail that this class hides away. WDYT? https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:566: request_->url_fetcher()->GetResponseAsString(&json_string_); Do we need the json_string_ member? Seems like the only place outside of Run() below where it's accessed is NTPSnippetsFetcher::OnJsonParsingCompleted, which might as well just query it itself. https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:586: .Run(std::move(result), /*name=*/std::string(), std::move(request_)); Instead of "name", this should be the actual name of the parameter :) https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1117: // that instead of over-fetching and filtering here. nit: This comment doesn't parse https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:224: // make these an implementation detail in the body and remove the mock. nit: They could probably be protected already https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:264: net::HttpRequestHeaders headers_; Does this need to be a class member? If we store the auth_header string, I think this could be local in BuildHeaders(), right? https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:287: void CompleteRequest(std::unique_ptr<Request> request); Can we find a better name for this? Not quite sure what it should be though.. something like NetworkFetchFinished? Or maybe just add a comment.
https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:346: ParseJSONCallback callback); On 2016/11/23 11:03:44, Marc Treib wrote: > const& Done. https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:367: // This callback is called by a Js On 2016/11/23 11:03:44, Marc Treib wrote: > ? Changed. But why that happened, no idea... https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:559: NTPSnippetsFetcher::JsonCallbackHandler::JsonCallbackHandler( On 2016/11/23 11:03:44, Marc Treib wrote: > Should we call this JsonParser or something like that? That's what it does, the > whole callback mangling is exactly the implementation detail that this class > hides away. WDYT? Okay. https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:566: request_->url_fetcher()->GetResponseAsString(&json_string_); On 2016/11/23 11:03:44, Marc Treib wrote: > Do we need the json_string_ member? Seems like the only place outside of Run() > below where it's accessed is NTPSnippetsFetcher::OnJsonParsingCompleted, which > might as well just query it itself. Done. (BTW: We had that. Maybe there is another way: we could pass it along with the callbacks. But this would make it less readable IMO.) https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:586: .Run(std::move(result), /*name=*/std::string(), std::move(request_)); On 2016/11/23 11:03:44, Marc Treib wrote: > Instead of "name", this should be the actual name of the parameter :) Sorry! I really shouldn't write comments late at night... https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1117: // that instead of over-fetching and filtering here. On 2016/11/23 11:03:44, Marc Treib wrote: > nit: This comment doesn't parse Done. https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:224: // make these an implementation detail in the body and remove the mock. On 2016/11/23 11:03:44, Marc Treib wrote: > nit: They could probably be protected already Done. https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:264: net::HttpRequestHeaders headers_; On 2016/11/23 11:03:44, Marc Treib wrote: > Does this need to be a class member? If we store the auth_header string, I think > this could be local in BuildHeaders(), right? This adds another check for an empty auth header. But okay, building the header really happens in one place, so done. https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:287: void CompleteRequest(std::unique_ptr<Request> request); On 2016/11/23 11:03:44, Marc Treib wrote: > Can we find a better name for this? Not quite sure what it should be though.. > something like NetworkFetchFinished? Or maybe just add a comment. It's |RequestFetchFinished| now. The network is implied by the "Fetch" and I really like to keep the connection to the Request.
LGTM with some last nits, and two actual fixes. We *really* need tests for authenticated fetching... https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:287: void CompleteRequest(std::unique_ptr<Request> request); On 2016/11/23 12:31:54, fhorschig wrote: > On 2016/11/23 11:03:44, Marc Treib wrote: > > Can we find a better name for this? Not quite sure what it should be though.. > > something like NetworkFetchFinished? Or maybe just add a comment. > > It's |RequestFetchFinished| now. The network is implied by the "Fetch" and I > really like to keep the connection to the Request. Now "Request" kinda sounds like a verb... and also the bike shed should be sky blue. I'll shut up now :) (The terms "request" and "fetch" are both quite overloaded, but that's probably not avoidable here.) https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:635: return *this; Need to set auth_header_ now. (And we really need some tests for the authenticated flow...) https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:689: if (auth_header_.empty()) { Should be if(!empty), right? https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:993: std::unique_ptr<JSONParser> json_parser = optional nit: This could be auto json_parser = MakeUnique... (the type is already in the MakeUnique, no need to spell it out again) https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1082: request->url_fetcher()->GetResponseAsString(&json_string); This could read directly into last_fetch_json_, no?
And BTW, thanks for putting up with all my nagging! This is a super important cleanup :)
Tests are included for the bugs you found. Thank you for your thorough reviews! https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:635: return *this; On 2016/11/23 12:42:26, Marc Treib wrote: > Need to set auth_header_ now. (And we really need some tests for the > authenticated flow...) Absolutely. There is a test for building authenticated requests .... it just didn't check the header. Now there is a header test. (Something seems to be wrong with my setup. This line disappeared just as the comments did...) https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:689: if (auth_header_.empty()) { On 2016/11/23 12:42:26, Marc Treib wrote: > Should be if(!empty), right? Of course. The adjusted test caught that. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:993: std::unique_ptr<JSONParser> json_parser = On 2016/11/23 12:42:26, Marc Treib wrote: > optional nit: This could be > auto json_parser = MakeUnique... > (the type is already in the MakeUnique, no need to spell it out again) Done. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1082: request->url_fetcher()->GetResponseAsString(&json_string); On 2016/11/23 12:42:26, Marc Treib wrote: > This could read directly into last_fetch_json_, no? If two requests return at the same time (in cases where fetches are concurrently) and set the message, one requests' error is printed twice, the other never. It's super unlikely and would not affect the user.
https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1082: request->url_fetcher()->GetResponseAsString(&json_string); On 2016/11/23 13:30:23, fhorschig wrote: > On 2016/11/23 12:42:26, Marc Treib wrote: > > This could read directly into last_fetch_json_, no? > > If two requests return at the same time (in cases where fetches are > concurrently) and set the message, one requests' error is printed twice, the > other never. It's super unlikely and would not affect the user. This runs on one thread only, so there's no such thing as "concurrently". If you saw a problem with error message, that must've been caused by something else.
https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1082: request->url_fetcher()->GetResponseAsString(&json_string); On 2016/11/23 13:35:10, Marc Treib wrote: > On 2016/11/23 13:30:23, fhorschig wrote: > > On 2016/11/23 12:42:26, Marc Treib wrote: > > > This could read directly into last_fetch_json_, no? > > > > If two requests return at the same time (in cases where fetches are > > concurrently) and set the message, one requests' error is printed twice, the > > other never. It's super unlikely and would not affect the user. > > This runs on one thread only, so there's no such thing as "concurrently". > If you saw a problem with error message, that must've been caused by something > else. Okay changed. I have never seen this and just assumed the URLFetcher might work concurrently.
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:294: SnippetsAvailableCallback ExtractSnippetsAvailableCallback() { this function seems pretty dangerous. After the call, 'snippets_available_callback_' is in an undefined state. So this function essentially destroys the object. As discussed offline with Mikel, let's add a static function Request::FinishAndDestroy(std::unique_ptr<Request> request, ...) { std::move(request->snippets_available_callback_).Run(...); } https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:485: FetchFinished(request_builder.SparseBuild(), OptionalFetchedCategories(), SparseBuild() has a smell. Could we instead call the function Abort() and enforce in the request that we never call Start() on it? https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:525: } can you add a NOTREACHED() here to make sure and document we don't get here. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:870: RequestBuilder request_builder) { already pass in the built request here? https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:196: // A class that builds authenticated and non-authenticated Requests. possibly in a different CL, but we should also try to unclutter the NTPSnippetsFetcher's interface. The builder would ideally also go into the .cc file, if tests need it, putting it into a nested "internal" namespace would be a good first step. (with that, we should also move the Request, JsonParser, and FetchAPI types into that "internal" namespace). https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:892: void NTPSnippetsFetcher::StartRequest(std::unique_ptr<Request> request) { Let's clarify the exact purpose of the Request object. As it's currently done, it's purpose is to fetch the JSON string. Maybe Rename it to JsonRequest()? Then, we should limit the lifetime of request to precisely this job -- which means that we should not abuse it to carry the snippets_available_ callback. If we follow this approach, I'd propose the following changes: -- StartRequest() takes the builder. The idea is that StartRequest builds the Request object and RequestFetchFinished() destroys it. -- Let's rename StartRequest() -> StartJsonRequest(), and RequestFetchFinished() -> JsonRequestDone(). -- StartJsonRequest could look like this: void NTPSnippetFetcher::StartJsonRequest(RequestBuilder builder) { std::unique_ptr<JsonRequest> request = builder.Build(); JsonRequest raw_request = request.get(); raw_request->Start(base::BindOnce( &NTPSnippetsFetcher::JsonRequestDone, base::Unretained(this), base::Passed(std::move(request)); } JsonRequestDone() would take the error codes from the fetch so that it doesn't need to reach into the request's url-fetcher (feature-envy): void NTPSnippetFetcher::JsonRequestDone( std::unique_ptr<JsonRequest> request, SnippetsAvailableCallback callback, const URLRequestStatus& status, int response_code) { ... // request gets deleted here. } All other function would simply take a plain pointer as ownership is clearly defined. Now the only question is how to pass the snippetscallback around. We can either keep it in the builder and have Build() return a pair, or (probably cleaner), keep them separated from the start, i.e. have pending_requests_ hold a pair<RequestBuilder, SnippetsAvailableCallback>. The trick is again, that we would pass that callback into JsonRequestDone() and clearly separate the two concerns: fetching json and parsing it into our data structure + return to the caller. https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:923: pending_requests_.pop(); might sound pedantic, but during this call, the last element on pending_requests_ is in an unspecific state. I'd vote for: std::unique_ptr<RequestBuilder> = pending_requests.back(); pending_request.pop(); FetchSnippetsAuthenticated(...); https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1156: return (personalization_ == NTPSnippetsFetcher::Personalization::kPersonal || no need for NTPSnippetsFetcher:: qualifier. another nit: Wouldn't the name NeedsAuthentication be better suited?
A rough design for the next iteration... https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:294: SnippetsAvailableCallback ExtractSnippetsAvailableCallback() { On 2016/11/24 09:32:55, tschumann wrote: > this function seems pretty dangerous. After the call, > 'snippets_available_callback_' is in an undefined state. > So this function essentially destroys the object. > > As discussed offline with Mikel, let's add a static function > > Request::FinishAndDestroy(std::unique_ptr<Request> request, ...) { > std::move(request->snippets_available_callback_).Run(...); > } > It's completely gone in the new design. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:485: FetchFinished(request_builder.SparseBuild(), OptionalFetchedCategories(), On 2016/11/24 09:32:55, tschumann wrote: > SparseBuild() has a smell. > Could we instead call the function Abort() and enforce in the request that we > never call Start() on it? It's Abort now. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:525: } On 2016/11/24 09:32:55, tschumann wrote: > can you add a NOTREACHED() here to make sure and document we don't get here. Done. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:870: RequestBuilder request_builder) { On 2016/11/24 09:32:55, tschumann wrote: > already pass in the built request here? I would like the builder more as we still need to set the URL. It would be very weird to set everything using the builder and the URL directly. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:196: // A class that builds authenticated and non-authenticated Requests. On 2016/11/24 09:32:56, tschumann wrote: > possibly in a different CL, but we should also try to unclutter the > NTPSnippetsFetcher's interface. The builder would ideally also go into the .cc > file, if tests need it, putting it into a nested "internal" namespace would be a > good first step. > (with that, we should also move the Request, JsonParser, and FetchAPI types into > that "internal" namespace). Acknowledged. https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:892: void NTPSnippetsFetcher::StartRequest(std::unique_ptr<Request> request) { On 2016/11/24 09:32:56, tschumann wrote: > Let's clarify the exact purpose of the Request object. As it's currently done, > it's purpose is to fetch the JSON string. Maybe Rename it to JsonRequest()? > > Then, we should limit the lifetime of request to precisely this job -- which > means that we should not abuse it to carry the snippets_available_ callback. > If we follow this approach, I'd propose the following changes: > -- StartRequest() takes the builder. The idea is that StartRequest builds the > Request object and RequestFetchFinished() destroys it. > -- Let's rename StartRequest() -> StartJsonRequest(), and RequestFetchFinished() > -> JsonRequestDone(). > -- StartJsonRequest could look like this: > void NTPSnippetFetcher::StartJsonRequest(RequestBuilder builder) { > std::unique_ptr<JsonRequest> request = builder.Build(); > JsonRequest raw_request = request.get(); > raw_request->Start(base::BindOnce( > &NTPSnippetsFetcher::JsonRequestDone, base::Unretained(this), > base::Passed(std::move(request)); > } > > JsonRequestDone() would take the error codes from the fetch so that it doesn't > need to reach into the request's url-fetcher (feature-envy): > void NTPSnippetFetcher::JsonRequestDone( > std::unique_ptr<JsonRequest> request, SnippetsAvailableCallback callback, > const URLRequestStatus& status, int response_code) { > ... > // request gets deleted here. > } > > All other function would simply take a plain pointer as ownership is clearly > defined. > > > Now the only question is how to pass the snippetscallback around. We can either > keep it in the builder and have Build() return a pair, or (probably cleaner), > keep them separated from the start, i.e. have pending_requests_ hold a > pair<RequestBuilder, SnippetsAvailableCallback>. The trick is again, that we > would pass that callback into JsonRequestDone() and clearly separate the two > concerns: fetching json and parsing it into our data structure + return to the > caller. > Done. https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:923: pending_requests_.pop(); On 2016/11/24 09:32:56, tschumann wrote: > might sound pedantic, but during this call, the last element on > pending_requests_ is in an unspecific state. I'd vote for: > std::unique_ptr<RequestBuilder> = pending_requests.back(); > pending_request.pop(); > FetchSnippetsAuthenticated(...); I changed it to an equivalent but there RequestBuilder can not be copied. Therefore the last element will always be in an unspecified state for one instructions length: RequestBuilder request_builder = std::move(pending_requests_.front()); // Unspecified here because the builder needs to be moved. pending_requests_.pop(); FetchSnippetsAuthenticated(std::move(request_builder), std:: containers stay in a valid state even if not all elements are in a specified state [1]. If it is that important, we can drop the OnceCallback for a Callback and use copies/shared pointers ... but I would like to keep this concept of ownership. [1] http://stackoverflow.com/questions/23118391/can-i-stdmove-an-element-out-of-a... https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1156: return (personalization_ == NTPSnippetsFetcher::Personalization::kPersonal || On 2016/11/24 09:32:56, tschumann wrote: > no need for NTPSnippetsFetcher:: qualifier. > > another nit: Wouldn't the name NeedsAuthentication be better suited? Done.
the overall design looks pretty nice now! thanks for all the iterations. I'll have another closer look, but wanted to give you a quick feedback we're heading the right way. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:870: RequestBuilder request_builder) { On 2016/11/24 17:10:43, fhorschig wrote: > On 2016/11/24 09:32:55, tschumann wrote: > > already pass in the built request here? > > I would like the builder more as we still need to set the URL. It would be very > weird to set everything using the builder and the URL directly. Oh, completely missed that. Fine with me. That call is a bit hidden inside the StartRequest() call --mind making it a separate statement? https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:923: pending_requests_.pop(); On 2016/11/24 17:10:44, fhorschig wrote: > On 2016/11/24 09:32:56, tschumann wrote: > > might sound pedantic, but during this call, the last element on > > pending_requests_ is in an unspecific state. I'd vote for: > > std::unique_ptr<RequestBuilder> = pending_requests.back(); > > pending_request.pop(); > > FetchSnippetsAuthenticated(...); > > I changed it to an equivalent but there RequestBuilder can not be copied. > Therefore the last element will always be in an unspecified state for one > instructions length: > RequestBuilder request_builder = std::move(pending_requests_.front()); > // Unspecified here because the builder needs to be moved. > pending_requests_.pop(); > FetchSnippetsAuthenticated(std::move(request_builder), > > std:: containers stay in a valid state even if not all elements are in a > specified state [1]. If it is that important, we can drop the OnceCallback for a > Callback and use copies/shared pointers ... but I would like to keep this > concept of ownership. > > [1] > http://stackoverflow.com/questions/23118391/can-i-stdmove-an-element-out-of-a... oh that's completely fine for the one statement (actually precisely what I meant). We should just not leave it in that state for the duration of FetchsnippetsAuthenticated(). https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:484: NTPSnippetsFetcher::JsonRequest::JsonRequest( if the request is now also doing the json-parsing (which is completely fine), we should probably rename it, to FetchSnippetRequest -- as it now gives the caller parsed snippets opposed to just the json-strings before. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:853: BuilderCallbackPair builder_callback_pair) { yeah, the underlying type itself would be much easier to read. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:958: FetchResult::OAUTH_TOKEN_ERROR, this seems odd. Abort() is a quite generic name and we hard-code that error code here? I'd vote for simply inlining this code wherever we call the method. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:270: using BuilderCallbackPair = this type is a bit hard to read (because it could also be a builder-callback). Why not BuilderAndCallback? Then again, I really wonder if we need that typedef. It might be easier to read if just use the proper types.
I am not exactly sure about renaming the JsonRequest (see comment). Also still looking for the "memory leaks" found by asan. https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:870: RequestBuilder request_builder) { On 2016/11/24 17:57:26, tschumann wrote: > On 2016/11/24 17:10:43, fhorschig wrote: > > On 2016/11/24 09:32:55, tschumann wrote: > > > already pass in the built request here? > > > > I would like the builder more as we still need to set the URL. It would be > very > > weird to set everything using the builder and the URL directly. > > Oh, completely missed that. Fine with me. That call is a bit hidden inside the > StartRequest() call --mind making it a separate statement? Already done ;) https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/290001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:923: pending_requests_.pop(); On 2016/11/24 17:57:26, tschumann wrote: > On 2016/11/24 17:10:44, fhorschig wrote: > > On 2016/11/24 09:32:56, tschumann wrote: > > > might sound pedantic, but during this call, the last element on > > > pending_requests_ is in an unspecific state. I'd vote for: > > > std::unique_ptr<RequestBuilder> = pending_requests.back(); > > > pending_request.pop(); > > > FetchSnippetsAuthenticated(...); > > > > I changed it to an equivalent but there RequestBuilder can not be copied. > > Therefore the last element will always be in an unspecified state for one > > instructions length: > > RequestBuilder request_builder = std::move(pending_requests_.front()); > > // Unspecified here because the builder needs to be moved. > > pending_requests_.pop(); > > FetchSnippetsAuthenticated(std::move(request_builder), > > > > std:: containers stay in a valid state even if not all elements are in a > > specified state [1]. If it is that important, we can drop the OnceCallback for > a > > Callback and use copies/shared pointers ... but I would like to keep this > > concept of ownership. > > > > [1] > > > http://stackoverflow.com/questions/23118391/can-i-stdmove-an-element-out-of-a... > > oh that's completely fine for the one statement (actually precisely what I > meant). We should just not leave it in that state for the duration of > FetchsnippetsAuthenticated(). Ah, now it makes perfect sense. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:484: NTPSnippetsFetcher::JsonRequest::JsonRequest( On 2016/11/24 17:57:26, tschumann wrote: > if the request is now also doing the json-parsing (which is completely fine), we > should probably rename it, to FetchSnippetRequest -- as it now gives the caller > parsed snippets opposed to just the json-strings before. The responsibilties were: Request | Fetcher -------------------------------- json_str | json_value | snippets and are now: Request | Fetcher -------------------------------- json_str | json_value | snippets Request parses any JSON into a base::Value structure which means it is still the JSON, just in a different representation. The Fetcher uses |JsonToSnippets|. which actually builds the categories and constructs the snippets. Therefore, I think this current name is much more fitting. For a FetchSnippetsRequest, I would have to move |JsonToSnippets| into the request, but this feels wrong. It's more related to the Fetcher than to the snippets. If you like, I can move the string-to-value parsing back to the Fetcher. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:853: BuilderCallbackPair builder_callback_pair) { On 2016/11/24 17:57:26, tschumann wrote: > yeah, the underlying type itself would be much easier to read. Done. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:958: FetchResult::OAUTH_TOKEN_ERROR, On 2016/11/24 17:57:26, tschumann wrote: > this seems odd. Abort() is a quite generic name and we hard-code that error code > here? I'd vote for simply inlining this code wherever we call the method. Done. https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/310001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:270: using BuilderCallbackPair = On 2016/11/24 17:57:26, tschumann wrote: > this type is a bit hard to read (because it could also be a builder-callback). > Why not BuilderAndCallback? > Then again, I really wonder if we need that typedef. It might be easier to read > if just use the proper types. Done, it's far easier to understand this way (but we have some very long signatures).
Patchset #14 (id:350001) has been deleted
Patchset #15 (id:390001) has been deleted
Patchset #14 (id:370001) has been deleted
Patchset #13 (id:330001) has been deleted
Patchset #13 (id:410001) has been deleted
The leaking test is fixed and all comments are adressed. I'd like to hear your opinion about my opinion on the class name before submitting it on Monday. Thanks so far!
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/02 10:31:07, fhorschig wrote: > The leaking test is fixed and all comments are adressed. I'd like to hear your > opinion about my opinion on the class name before submitting it on Monday. > > Thanks so far! Just for the record: If you do any major changes to a CL after you got l-g-t-m, you should ask for approval again before landing. I'm looking now!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:435: OptionalFetchedCategories(), std::move(builder_and_callback.second), IMO this is a good signal that this shouldn't be a pair. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:464: pending_requests_.push(std::move(builder_and_callback)); Instead, just make a pair out of the two separate things here. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:534: DCHECK(stores_result_to_string); nit: no empty line before. (maybe after, if anything) https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:543: FetchedCategoriesVector categories; Unused https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:575: std::unique_ptr<JsonRequest> request = base::MakeUnique<JsonRequest>( optional: This could be "auto request = ..." (no need to spell out the type twice in one line) https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:661: return IsBooleanParameterEnabled(kSendTopLanguagesName, false); What's the "false"? Add a "/*name=*/" comment? https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:877: base::Unretained(this), base::Passed(std::move(request)), Are both base::Passed and std::move required here?! Don't they basically both express the same thing? https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:195: class RequestBuilder { nit: This needs to be in the header only because tests use it, right? Might be worth a comment. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:203: std::unique_ptr<JsonRequest> Abort() const; I think this isn't used anymore? https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:272: builder_callback_pair); Maybe this has been discussed before (if so, feel free to ignore), but: Why is this passed as a pair? Seems it would be simpler to just pass the two things separately. IMO if they're so related that they should never be passed separately, then there should be a struct to group them (and if we can't come up with a name for that struct, then that's a signal that maybe they're not so related after all!) https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:292: void JsonRequestDone(std::unique_ptr<JsonRequest> request, Empty line before - this is not related to OAuth2TokenService::Observer. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:435: " \"user_locale\": \"en\"," Why has this changed? https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:470: " \"uiLanguage\": \"en\"," Same here, why has this changed? https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:944: EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1); So this was necessary to make the test not leak? Then that absolutely deserves a comment!
Thanks for the comments. Just FMI: Is there a way to un-LGTM? (I would really like to know when this CL really deserves to be submitted.) https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:435: OptionalFetchedCategories(), std::move(builder_and_callback.second), On 2016/12/02 11:21:52, Marc Treib wrote: > IMO this is a good signal that this shouldn't be a pair. Yes. Un-paired. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:464: pending_requests_.push(std::move(builder_and_callback)); On 2016/12/02 11:21:52, Marc Treib wrote: > Instead, just make a pair out of the two separate things here. Done. You convinced me multiple times now. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:534: DCHECK(stores_result_to_string); On 2016/12/02 11:21:52, Marc Treib wrote: > nit: no empty line before. (maybe after, if anything) Done. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:543: FetchedCategoriesVector categories; On 2016/12/02 11:21:52, Marc Treib wrote: > Unused Done. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:575: std::unique_ptr<JsonRequest> request = base::MakeUnique<JsonRequest>( On 2016/12/02 11:21:52, Marc Treib wrote: > optional: This could be "auto request = ..." (no need to spell out the type > twice in one line) Done. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:661: return IsBooleanParameterEnabled(kSendTopLanguagesName, false); On 2016/12/02 11:21:52, Marc Treib wrote: > What's the "false"? Add a "/*name=*/" comment? Done. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:877: base::Unretained(this), base::Passed(std::move(request)), On 2016/12/02 11:21:52, Marc Treib wrote: > Are both base::Passed and std::move required here?! Don't they basically both > express the same thing? Removed base::Passed. This is not a good thing to do with OnceCallbacks. It was a temporary attempt to define ownership more clearly that I forgot to clean up, so thanks! https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:195: class RequestBuilder { On 2016/12/02 11:21:52, Marc Treib wrote: > nit: This needs to be in the header only because tests use it, right? > Might be worth a comment. Done. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:203: std::unique_ptr<JsonRequest> Abort() const; On 2016/12/02 11:21:52, Marc Treib wrote: > I think this isn't used anymore? Yes. Not bad, I didn't notice that at all! https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:272: builder_callback_pair); On 2016/12/02 11:21:52, Marc Treib wrote: > Maybe this has been discussed before (if so, feel free to ignore), but: Why is > this passed as a pair? Seems it would be simpler to just pass the two things > separately. > IMO if they're so related that they should never be passed separately, then > there should be a struct to group them (and if we can't come up with a name for > that struct, then that's a signal that maybe they're not so related after all!) They don't cohere enough to keep them in a pair everywhere - just as you mentioned (mulitple times in this draft). They are now separated and only paired up in the |pending_requests_| queue. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:292: void JsonRequestDone(std::unique_ptr<JsonRequest> request, On 2016/12/02 11:21:52, Marc Treib wrote: > Empty line before - this is not related to OAuth2TokenService::Observer. Done. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:435: " \"user_locale\": \"en\"," On 2016/12/02 11:21:52, Marc Treib wrote: > Why has this changed? Removed. The |test_params| method introduced a default language. But it is good to have an undefined case as the default, so I reverted that decision. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:470: " \"uiLanguage\": \"en\"," On 2016/12/02 11:21:52, Marc Treib wrote: > Same here, why has this changed? Done as above. https://codereview.chromium.org/2505643002/diff/430001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:944: EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1); On 2016/12/02 11:21:52, Marc Treib wrote: > So this was necessary to make the test not leak? Then that absolutely deserves a > comment! True. Done.
You can't directly remove an l-g-t-m, but you can say not lgtm! (just for demonstration purposes :D) https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:462: pending_requests_.push({std::move(builder), std::move(callback)}); optional: pending_requests_.emplace(std::move(builder), std::move(callback)); (to get rid of the somewhat ugly curly braces) https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:196: // TODO(fhorschig): Move into separate file with snippets::internal namespace. But then it can't be an inner class anymore :) IMO it's okay for this to stay here for testing reasons. https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:939: // Call the delegate callback manually as the TestUrlFetcher deletes any nit: TestURLFetcher nit: I'm not quite sure what to make of "deletes any Start functionality".
That being said: lgtm :)
Nice demonstration. The red comment is pretty eye-catching... https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:462: pending_requests_.push({std::move(builder), std::move(callback)}); On 2016/12/02 13:40:15, Marc Treib wrote: > optional: pending_requests_.emplace(std::move(builder), std::move(callback)); > (to get rid of the somewhat ugly curly braces) Done. https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:196: // TODO(fhorschig): Move into separate file with snippets::internal namespace. On 2016/12/02 13:40:15, Marc Treib wrote: > But then it can't be an inner class anymore :) > IMO it's okay for this to stay here for testing reasons. It's fine if this is not an inner class at some point and moving this is already on the list for quite a while... https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2505643002/diff/450001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:939: // Call the delegate callback manually as the TestUrlFetcher deletes any On 2016/12/02 13:40:15, Marc Treib wrote: > nit: TestURLFetcher > nit: I'm not quite sure what to make of "deletes any Start functionality". Done.
lgtm few more nits on the design. Feel free to submit once addressed. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:277: void Start(CompletedCallback callback); would be nice to explain any constraints about this method. in particular, do you require SetUrlFetcher() to be called before? That said, I wonder if it would make sense to remove SetURLFetcher() from the public interface and make the builder a friend of json request. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:955: request->url_fetcher()->GetResponseAsString(&last_fetch_json_); this has a smell as you're reaching into implementation details to get something quite fundamental. Why not pass the string into the callback and remove the url_fetcher() accessor? Alternatively, you could also directly expose it on the JsonRequest, i.e. have a method GetJsonResponse() on the request. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:958: tick_clock_->NowTicks() - request->creation_time()); I wouldn't expose the details of the request. If you want to get the fetch-time, then why not have a GetFetchTime() method (taking a clock or the ticks) on the request? The more explict you can make an API, the easier it is to use and maintain. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:975: LOG(WARNING) << "Received invalid snippets: " << last_fetch_json_; nit: I'd inverse this to handle the error cases first. if (!JsonToSnippets(...) { LOG()... return; } FilterCategories(...) ... This makes it easy to follow the main flow. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:296: const std::string& extra_message); nit: 'extra_message' doesn't provide a lot of meaning. Rename to 'error_details'? I guess it's only populated in cases of errors, so that would be appropriate.
This is the moment I have been waiting for so long! https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:277: void Start(CompletedCallback callback); On 2016/12/05 10:53:07, tschumann wrote: > would be nice to explain any constraints about this method. in particular, do > you require SetUrlFetcher() to be called before? > That said, I wonder if it would make sense to remove SetURLFetcher() from the > public interface and make the builder a friend of json request. Done. There is a friendly builder now. This will make even more sense when the builder is moved to snippets::internal later and can be a subclass of the request. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:955: request->url_fetcher()->GetResponseAsString(&last_fetch_json_); On 2016/12/05 10:53:07, tschumann wrote: > this has a smell as you're reaching into implementation details to get something > quite fundamental. Why not pass the string into the callback and remove the > url_fetcher() accessor? > Alternatively, you could also directly expose it on the JsonRequest, i.e. have a > method GetJsonResponse() on the request. Done. |GetResponseString| is now exposed and the fetcher hidden. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:958: tick_clock_->NowTicks() - request->creation_time()); On 2016/12/05 10:53:07, tschumann wrote: > I wouldn't expose the details of the request. If you want to get the fetch-time, > then why not have a GetFetchTime() method (taking a clock or the ticks) on the > request? > > The more explict you can make an API, the easier it is to use and maintain. Done. |GetFetchTime| introduced. RequestBuilder and JsonRequest borrow the TickClock from the SnippetsFetcher. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:975: LOG(WARNING) << "Received invalid snippets: " << last_fetch_json_; On 2016/12/05 10:53:07, tschumann wrote: > nit: I'd inverse this to handle the error cases first. > if (!JsonToSnippets(...) { > LOG()... > return; > } > FilterCategories(...) > ... > > This makes it easy to follow the main flow. Done. https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:296: const std::string& extra_message); On 2016/12/05 10:53:07, tschumann wrote: > nit: 'extra_message' doesn't provide a lot of meaning. Rename to > 'error_details'? I guess it's only populated in cases of errors, so that would > be appropriate. Done.
lgtm https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:277: void Start(CompletedCallback callback); On 2016/12/05 12:46:28, fhorschig wrote: > On 2016/12/05 10:53:07, tschumann wrote: > > would be nice to explain any constraints about this method. in particular, do > > you require SetUrlFetcher() to be called before? > > That said, I wonder if it would make sense to remove SetURLFetcher() from the > > public interface and make the builder a friend of json request. > > Done. There is a friendly builder now. This will make even more sense when the > builder is moved to snippets::internal later and can be a subclass of the > request. thanks. Although I'd oppose to the idea of the builder being a request ;-) https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:267: base::TickClock* tick_clock, please specify lifetime requirements of tick_clock. https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:275: const std::string& error_details)>; please specify the conditions under which a client can expect a message in error_details. https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:498: base::TimeDelta NTPSnippetsFetcher::JsonRequest::GetFetchTime() const { nit: it's technically not a time, GetFetchDuration? https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:215: RequestBuilder& SetTickClock(base::TickClock* tick_clock); please specify lifetime requirements. The clock needs to stay alive until when precisely? Same for the passed in LanguageModel.
https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/470001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:277: void Start(CompletedCallback callback); On 2016/12/05 12:54:35, tschumann wrote: > On 2016/12/05 12:46:28, fhorschig wrote: > > On 2016/12/05 10:53:07, tschumann wrote: > > > would be nice to explain any constraints about this method. in particular, > do > > > you require SetUrlFetcher() to be called before? > > > That said, I wonder if it would make sense to remove SetURLFetcher() from > the > > > public interface and make the builder a friend of json request. > > > > Done. There is a friendly builder now. This will make even more sense when the > > builder is moved to snippets::internal later and can be a subclass of the > > request. > > thanks. Although I'd oppose to the idea of the builder being a request ;-) Thanks for noticing the slip of tongue: inner class. Subclass would indeed be pretty horrible. https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:267: base::TickClock* tick_clock, On 2016/12/05 12:54:35, tschumann wrote: > please specify lifetime requirements of tick_clock. Done. https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:275: const std::string& error_details)>; On 2016/12/05 12:54:35, tschumann wrote: > please specify the conditions under which a client can expect a message in > error_details. Done. https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:498: base::TimeDelta NTPSnippetsFetcher::JsonRequest::GetFetchTime() const { On 2016/12/05 12:54:35, tschumann wrote: > nit: it's technically not a time, GetFetchDuration? Correct. Changed. https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2505643002/diff/490001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:215: RequestBuilder& SetTickClock(base::TickClock* tick_clock); On 2016/12/05 12:54:35, tschumann wrote: > please specify lifetime requirements. The clock needs to stay alive until when > precisely? > Same for the passed in LanguageModel. Done.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2505643002/#ps510001 (title: "Explanatory 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": 510001, "attempt_start_ts": 1480943612669830,
"parent_rev": "61ef2c572953a9a4c2cdc8304ed2ec9e7bf6e426", "commit_rev":
"2f3196c857039e2daadc70e9e88f34fa49e31df9"}
Message was sent while issue was closed.
Description was changed from ========== Concurrent Snippet Fetches. Multiple concurrent fetch processes ensure that the UI will always get a correct response for a |Fetch| request. In order to bind fetcher and callback to the request, a new |Request| class provides another layer of abstraction. (No tests were deleted. The test "ShouldCancelOngoingFetch" is not part of the use case anymore and was replaced with "ShouldProcessConcurrentFetches") BUG=659931 ========== to ========== Concurrent Snippet Fetches. Multiple concurrent fetch processes ensure that the UI will always get a correct response for a |Fetch| request. In order to bind fetcher and callback to the request, a new |Request| class provides another layer of abstraction. (No tests were deleted. The test "ShouldCancelOngoingFetch" is not part of the use case anymore and was replaced with "ShouldProcessConcurrentFetches") BUG=659931 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:510001)
Message was sent while issue was closed.
Description was changed from ========== Concurrent Snippet Fetches. Multiple concurrent fetch processes ensure that the UI will always get a correct response for a |Fetch| request. In order to bind fetcher and callback to the request, a new |Request| class provides another layer of abstraction. (No tests were deleted. The test "ShouldCancelOngoingFetch" is not part of the use case anymore and was replaced with "ShouldProcessConcurrentFetches") BUG=659931 ========== to ========== Concurrent Snippet Fetches. Multiple concurrent fetch processes ensure that the UI will always get a correct response for a |Fetch| request. In order to bind fetcher and callback to the request, a new |Request| class provides another layer of abstraction. (No tests were deleted. The test "ShouldCancelOngoingFetch" is not part of the use case anymore and was replaced with "ShouldProcessConcurrentFetches") BUG=659931 Committed: https://crrev.com/7530d98e8559cf6f598b57a073d19cf7470ba752 Cr-Commit-Position: refs/heads/master@{#436280} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/7530d98e8559cf6f598b57a073d19cf7470ba752 Cr-Commit-Position: refs/heads/master@{#436280} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
