Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671453003:
[NTP::Cleanup] Avoid nested complex function calls (Closed)
|
Created:
3 years, 10 months ago by vitaliii Modified:
3 years, 10 months ago CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP::Cleanup] Avoid nested complex function calls
The RemoteSuggestionsProviderImpl constructor takes a long list of parameters and previously the RemoteSuggestionFetcher (which also takes a lot of parameters) was constructed in line. This was confusing, because one could not easily determine to which MakeUnique given argument belonged to.
This CL moves the RemoteSuggestionsFetcher into a local variable to improve readability.
BUG=674033
Review-Url: https://codereview.chromium.org/2671453003
Cr-Commit-Position: refs/heads/master@{#448992}
Committed: https://chromium.googlesource.com/chromium/src/+/5a842fb2c97ce7c2e140a2d77d503c1dc6e1a299
Patch Set 1 #Patch Set 2 : resolve iOS compile error. #
Total comments: 3
Patch Set 3 : tschumann@ & treib@ comments. #
Total comments: 1
Patch Set 4 : clean rebase. #Patch Set 5 : tschumann@ comment. #Patch Set 6 : clean rebase. #
Messages
Total messages: 55 (38 generated)
The CQ bit was checked by vitaliii@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...
vitaliii@chromium.org changed reviewers: + jkrcal@chromium.org
Hi jkrcal@, could you have a look at content_suggestions_service_factory.cc?
lgtm, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2671453003/#ps20001 (title: "resolve iOS compile error.")
The CQ bit was unchecked by vitaliii@chromium.org
The CQ bit was checked by vitaliii@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...
vitaliii@chromium.org changed reviewers: + noyau@chromium.org
Hi noyau@, Could you have a look at iOS bit?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2671453003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2671453003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:192: std::move(status_service)); I'm having a hard time following the motivation. Clearly, moving the fetcher into a separate variable makes the code easier to read. But that's completely independent of using MakeUnique. For example, I don't see the benefit of moving the other parameters into variables (as (1) there's another layer of indirection now, (2) they are still in scope after the call but moved out, which is not an easy state to reason about.
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2671453003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2671453003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:192: std::move(status_service)); On 2017/02/01 11:35:37, tschumann wrote: > I'm having a hard time following the motivation. > Clearly, moving the fetcher into a separate variable makes the code easier to > read. But that's completely independent of using MakeUnique. > For example, I don't see the benefit of moving the other parameters into > variables (as (1) there's another layer of indirection now, (2) they are still > in scope after the call but moved out, which is not an easy state to reason > about. Maybe put all of this into a scope, so the variables won't exist after the call?
noyau@chromium.org changed reviewers: - treib@chromium.org
I don't care one way or another. But this method is now becoming very long and would benefit from a little cleanup, or at least a bit more comments. I would suggest that instead of having one service doing everything, and one factory to assemble them, this should be multiple factories, building multiple services dependent on each other.
The CQ bit was checked by vitaliii@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...
Hi tschumann@ and treib@, I addressed your comments, please have a look. noyau@, do you mean iOS factory? If yes, we could register providers in methods (as we do in the other factory). Overall, IMO this feels orthogonal to this CL. https://codereview.chromium.org/2671453003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2671453003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:192: std::move(status_service)); On 2017/02/01 12:33:20, Marc Treib wrote: > On 2017/02/01 11:35:37, tschumann wrote: > > I'm having a hard time following the motivation. > > Clearly, moving the fetcher into a separate variable makes the code easier to > > read. But that's completely independent of using MakeUnique. > > For example, I don't see the benefit of moving the other parameters into > > variables (as (1) there's another layer of indirection now, (2) they are still > > in scope after the call but moved out, which is not an easy state to reason > > about. > > Maybe put all of this into a scope, so the variables won't exist after the call? Done. 1) I inlined MakeUnique calls, which have no arguments; 2) For ArticlesProvider I placed all of them into a scope, so (2) is not applicable anymore. As for (1) I still think that this way it is more readable + this discourages inlining MakeUnique calls with arguments.
treib@chromium.org changed reviewers: + treib@chromium.org
This seems fine to me; I don't feel very strongly either way. rs lgtm if Tim has no further objections.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2671453003/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2671453003/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:174: { sorry for the late response. I'm not a fan of such scopes. They usually only cure a symptom, not the underlying problem. Usually, the better answer for reducing scope is to extract a function but there are a lot of variables to pass. I'd be in favor of the change that only moves 'suggestions_fetcher' into a separate variable and leaves the rest unchanged. The original problem stemmed from the long parameter list of the fetcher's constructor which might got confused with the provider's constructor. So let's just fix that and keep the rest.
On 2017/02/01 13:15:15, vitaliii wrote: > Hi tschumann@ and treib@, > > I addressed your comments, please have a look. > > noyau@, > > do you mean iOS factory? If yes, we could register providers in methods (as we > do in the other factory). Overall, IMO this feels orthogonal to this CL. > I'd be in favor of a change that would move the creation of providers out of this factory. The providers should have their own factories, and the code associating them with the suggestion service should be shared between platforms. One approach would be to make the providers KeyedServices, this would go a long way in making this code simpler. The code smell that I found most problematic however is the fact that the two factories are diverging in their implementation quite a bit now. . They should be quite similar, and right now this is not the case. I see a lot of conditionals that don't exists on iOS. Your little change here is small, but it adds more lines of code to a very long method, doesn't make it more readable, and overall makes the problem worse.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by vitaliii@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...
[jkrcal@, treib@ - feel free to ignore] I addressed tschumann@ comment, could you have a look? tschumann@: at the change in general; noyau@: as iOS OWNER?
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
noyau is OOO for the whole week
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sdefresne@, thanks for notifying me about noyau@'s absence. Would you mind having a look (as an OWNER of ios/chrome)?
ios/ lgtm
The CQ bit was checked by vitaliii@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: This issue passed the CQ dry run.
Description was changed from ========== [NTP::Cleanup] No nested MakeUnique in service factories. Previously there were instances of MakeUnique, which had another MakeUnique as an argument. This was confusing, because one could not easily determine to which MakeUnique given argument belonged to. This CL passes inner MakeUnique instances as variables. BUG=674033 ========== to ========== [NTP::Cleanup] Avoid nested complex function calls The RemoteSuggestionsProviderImpl constructor takes a long list of parameters and previously the RemoteSuggestionFetcher (which also takes a lot of parameters) was constructed in line. This was confusing, because one could not easily determine to which MakeUnique given argument belonged to. This CL moves the RemoteSuggestionsFetcher into a local variable to improve readability. BUG=674033 ==========
lgtm
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, treib@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2671453003/#ps160001 (title: "clean rebase.")
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": 160001, "attempt_start_ts": 1486565197841190,
"parent_rev": "0f6988096c4433317675447c2f58ecf442a6122c", "commit_rev":
"5a842fb2c97ce7c2e140a2d77d503c1dc6e1a299"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Cleanup] Avoid nested complex function calls The RemoteSuggestionsProviderImpl constructor takes a long list of parameters and previously the RemoteSuggestionFetcher (which also takes a lot of parameters) was constructed in line. This was confusing, because one could not easily determine to which MakeUnique given argument belonged to. This CL moves the RemoteSuggestionsFetcher into a local variable to improve readability. BUG=674033 ========== to ========== [NTP::Cleanup] Avoid nested complex function calls The RemoteSuggestionsProviderImpl constructor takes a long list of parameters and previously the RemoteSuggestionFetcher (which also takes a lot of parameters) was constructed in line. This was confusing, because one could not easily determine to which MakeUnique given argument belonged to. This CL moves the RemoteSuggestionsFetcher into a local variable to improve readability. BUG=674033 Review-Url: https://codereview.chromium.org/2671453003 Cr-Commit-Position: refs/heads/master@{#448992} Committed: https://chromium.googlesource.com/chromium/src/+/5a842fb2c97ce7c2e140a2d77d50... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5a842fb2c97ce7c2e140a2d77d50... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
