Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553643003:
[NTP::PhysicalWeb] Implement Fetch for "More" button. (Closed)
|
Created:
4 years ago by vitaliii Modified:
4 years ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP::PhysicalWeb] Implement Fetch for "More" button.
According to the PRD, Physical Web section must support "More" button.
This CL implements it (i.e. Fetch method of the provider) + a test.
BUG=667759
Committed: https://crrev.com/741a461097ff13bae577dfc2545493e80926ae1b
Cr-Commit-Position: refs/heads/master@{#436582}
Patch Set 1 #
Total comments: 34
Patch Set 2 : treib@ comments. #
Total comments: 6
Patch Set 3 : treib@ comments. #
Total comments: 2
Patch Set 4 : treib@ nit. #
Dependent Patchsets: Messages
Total messages: 28 (17 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, could you have a look?
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, could you have a look?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:32: std::string GetPageId(const DictionaryValue* page_dictionary) { If it can't be null, reference rather than pointer. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:36: LOG(DFATAL) << "kResolvedUrlKey field is missing."; Use the actual value rather than "kResolvedUrlKey"? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:38: return GURL(raw_resolved_url).spec(); This converts to GURL and back to string. That does *some* normalization (e.g. add a trailing slash in some cases). If that's intended, please add a comment. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:71: // TODO(vitaliii): Implement More action. (crbug.com/667759) Remove this comment now :) https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:145: NotifyStatusChanged(CategoryStatus::AVAILABLE); Not related to this CL, but isn't the status always AVAILABLE anyway? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:187: if (static_cast<int>(suggestions.size()) == max_quantity) { This assumes max_quantity >= 1. Calling this with 0 isn't totally unreasonable, so maybe handle this case, or DCHECK? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:67: int max_quantity, max_count? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:36: using ::testing::get; Why does this have the leading "::", but none of the other "using"s do? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:48: MATCHER(HasUrlPointwise, "") { I have no idea what "pointwise" means :) https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:50: << ", but has URL: " << get<0>(arg).url().spec(); nit: For EXPECT_EQ etc, the expected value comes first. Would be nice to stay consistent. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:133: std::unique_ptr<base::RunLoop> run_loop_; This shouldn't be a member. Instantiate it locally where you need it, and pass it around as necessary (probably you only need to pass around its QuitClosure(), as a "done_callback"). https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:176: const int number_of_suggestions_in_section = 10; kNumSuggestionsInSection? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:204: base::IntToString(i)); TBH, I'd find it easier to read if this just spelled out the 10 IDs that are expected.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Marc, I addressed your comments. PTAL. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:32: std::string GetPageId(const DictionaryValue* page_dictionary) { On 2016/12/05 15:55:19, Marc Treib wrote: > If it can't be null, reference rather than pointer. Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:36: LOG(DFATAL) << "kResolvedUrlKey field is missing."; On 2016/12/05 15:55:19, Marc Treib wrote: > Use the actual value rather than "kResolvedUrlKey"? Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:38: return GURL(raw_resolved_url).spec(); On 2016/12/05 15:55:19, Marc Treib wrote: > This converts to GURL and back to string. That does *some* normalization (e.g. > add a trailing slash in some cases). If that's intended, please add a comment. Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:71: // TODO(vitaliii): Implement More action. (crbug.com/667759) On 2016/12/05 15:55:19, Marc Treib wrote: > Remove this comment now :) Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:145: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/12/05 15:55:19, Marc Treib wrote: > Not related to this CL, but isn't the status always AVAILABLE anyway? It is, but we may start setting it to something else. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:187: if (static_cast<int>(suggestions.size()) == max_quantity) { On 2016/12/05 15:55:19, Marc Treib wrote: > This assumes max_quantity >= 1. Calling this with 0 isn't totally unreasonable, > so maybe handle this case, or DCHECK? Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:67: int max_quantity, On 2016/12/05 15:55:19, Marc Treib wrote: > max_count? Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:36: using ::testing::get; On 2016/12/05 15:55:19, Marc Treib wrote: > Why does this have the leading "::", but none of the other "using"s do? Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:48: MATCHER(HasUrlPointwise, "") { On 2016/12/05 15:55:19, Marc Treib wrote: > I have no idea what "pointwise" means :) This a matcher for Pointwise. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:50: << ", but has URL: " << get<0>(arg).url().spec(); On 2016/12/05 15:55:19, Marc Treib wrote: > nit: For EXPECT_EQ etc, the expected value comes first. Would be nice to stay > consistent. I am not sure what you mean, but I changed order of parameters in return statement below. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:133: std::unique_ptr<base::RunLoop> run_loop_; On 2016/12/05 15:55:19, Marc Treib wrote: > This shouldn't be a member. Instantiate it locally where you need it, and pass > it around as necessary (probably you only need to pass around its QuitClosure(), > as a "done_callback"). Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:176: const int number_of_suggestions_in_section = 10; On 2016/12/05 15:55:19, Marc Treib wrote: > kNumSuggestionsInSection? Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:204: base::IntToString(i)); On 2016/12/05 15:55:19, Marc Treib wrote: > TBH, I'd find it easier to read if this just spelled out the 10 IDs that are > expected. Rewrote |for| loop to make it easier.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:145: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/12/06 07:30:20, vitaliii wrote: > On 2016/12/05 15:55:19, Marc Treib wrote: > > Not related to this CL, but isn't the status always AVAILABLE anyway? > > It is, but we may start setting it to something else. That's not a good reason to keep unnecessary code there. DCHECK instead? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:48: MATCHER(HasUrlPointwise, "") { On 2016/12/06 07:30:21, vitaliii wrote: > On 2016/12/05 15:55:19, Marc Treib wrote: > > I have no idea what "pointwise" means :) > > This a matcher for Pointwise. And now I even looked up what Pointwise does :) It's a bit ugly that this needs a different matcher which does basically the exact same thing. Could this be written in terms of ElementsAre (or ElementsAreArray)? (But not a big deal) https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:50: << ", but has URL: " << get<0>(arg).url().spec(); On 2016/12/06 07:30:21, vitaliii wrote: > On 2016/12/05 15:55:19, Marc Treib wrote: > > nit: For EXPECT_EQ etc, the expected value comes first. Would be nice to stay > > consistent. > > I am not sure what you mean, but I changed order of parameters in return > statement below. I meant, when using EXPECT_EQ, you write EXPECT_EQ(expected_value, actual_value); But here, the first argument is "actual" and the second is "expected". But apparently that's just how Pointwise works :( https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:204: base::IntToString(i)); On 2016/12/06 07:30:21, vitaliii wrote: > On 2016/12/05 15:55:19, Marc Treib wrote: > > TBH, I'd find it easier to read if this just spelled out the 10 IDs that are > > expected. > > Rewrote |for| loop to make it easier. Alright, fair enough. https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:36: LOG(DFATAL) << "resolvedUrl field is missing."; I'd write this as LOG(DFATAL) << physical_web::kResolvedUrlKey << " field is missing."; https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:164: if (!excluded_ids.count(GetPageId(*page_dictionary))) { page_dictionary can be null or uninitialized. LOG(DFATAL) is not a DCHECK, you need to handle failures. https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:58: std::vector<ContentSuggestion> actual_suggestions) { Should the vectors be const& ?
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Hi Marc, could you have a look? https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:145: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/12/06 10:01:34, Marc Treib wrote: > On 2016/12/06 07:30:20, vitaliii wrote: > > On 2016/12/05 15:55:19, Marc Treib wrote: > > > Not related to this CL, but isn't the status always AVAILABLE anyway? > > > > It is, but we may start setting it to something else. > > That's not a good reason to keep unnecessary code there. > DCHECK instead? Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:48: MATCHER(HasUrlPointwise, "") { On 2016/12/06 10:01:34, Marc Treib wrote: > On 2016/12/06 07:30:21, vitaliii wrote: > > On 2016/12/05 15:55:19, Marc Treib wrote: > > > I have no idea what "pointwise" means :) > > > > This a matcher for Pointwise. > > And now I even looked up what Pointwise does :) > It's a bit ugly that this needs a different matcher which does basically the > exact same thing. Could this be written in terms of ElementsAre (or > ElementsAreArray)? (But not a big deal) Done. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:50: << ", but has URL: " << get<0>(arg).url().spec(); On 2016/12/06 10:01:34, Marc Treib wrote: > On 2016/12/06 07:30:21, vitaliii wrote: > > On 2016/12/05 15:55:19, Marc Treib wrote: > > > nit: For EXPECT_EQ etc, the expected value comes first. Would be nice to > stay > > > consistent. > > > > I am not sure what you mean, but I changed order of parameters in return > > statement below. > > I meant, when using EXPECT_EQ, you write > EXPECT_EQ(expected_value, actual_value); > But here, the first argument is "actual" and the second is "expected". > But apparently that's just how Pointwise works :( Done. Removed the matcher, rewrote without Pairwise. https://codereview.chromium.org/2553643003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:204: base::IntToString(i)); On 2016/12/06 10:01:34, Marc Treib wrote: > On 2016/12/06 07:30:21, vitaliii wrote: > > On 2016/12/05 15:55:19, Marc Treib wrote: > > > TBH, I'd find it easier to read if this just spelled out the 10 IDs that are > > > expected. > > > > Rewrote |for| loop to make it easier. > > Alright, fair enough. Acknowledged. https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:36: LOG(DFATAL) << "resolvedUrl field is missing."; On 2016/12/06 10:01:34, Marc Treib wrote: > I'd write this as > LOG(DFATAL) << physical_web::kResolvedUrlKey << " field is missing."; Done. https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:164: if (!excluded_ids.count(GetPageId(*page_dictionary))) { On 2016/12/06 10:01:34, Marc Treib wrote: > page_dictionary can be null or uninitialized. LOG(DFATAL) is not a DCHECK, you > need to handle failures. Done. https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2553643003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:58: std::vector<ContentSuggestion> actual_suggestions) { On 2016/12/06 10:01:34, Marc Treib wrote: > Should the vectors be const& ? Done. The last vector cannot be const, because the callback requires so.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, LGTM! https://codereview.chromium.org/2553643003/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:144: DCHECK_EQ(category_status_, CategoryStatus::AVAILABLE); nit: Expected value comes first (the error message will be more useful/accurate that way)
addressed your nit, no need to look. https://codereview.chromium.org/2553643003/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2553643003/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:144: DCHECK_EQ(category_status_, CategoryStatus::AVAILABLE); On 2016/12/06 12:50:41, Marc Treib wrote: > nit: Expected value comes first (the error message will be more useful/accurate > that way) Done.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2553643003/#ps80001 (title: "treib@ nit.")
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": 80001, "attempt_start_ts": 1481028916850240,
"parent_rev": "9a919821a01a97d24a5189dece9bacd2f096986b", "commit_rev":
"120f7b0b248c7a2eb5a43949b7c33169c1675e5c"}
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [NTP::PhysicalWeb] Implement Fetch for "More" button. According to the PRD, Physical Web section must support "More" button. This CL implements it (i.e. Fetch method of the provider) + a test. BUG=667759 ========== to ========== [NTP::PhysicalWeb] Implement Fetch for "More" button. According to the PRD, Physical Web section must support "More" button. This CL implements it (i.e. Fetch method of the provider) + a test. BUG=667759 Committed: https://crrev.com/741a461097ff13bae577dfc2545493e80926ae1b Cr-Commit-Position: refs/heads/master@{#436582} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/741a461097ff13bae577dfc2545493e80926ae1b Cr-Commit-Position: refs/heads/master@{#436582} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
