Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513393002:
[NTP] Cleanup: offline pages related tests. (Closed)
|
Created:
4 years, 1 month ago by vitaliii Modified:
4 years, 1 month ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP] Cleanup: offline pages related tests.
During the move from old offline pages interface to the new one, a few
tests were noncritically altered, this CL cleans them up.
BUG=None
Committed: https://crrev.com/167268f8c5adff06b1e6fb9994fb53c8aac80792
Cr-Commit-Position: refs/heads/master@{#433809}
Patch Set 1 #
Total comments: 12
Patch Set 2 : treib@ comments. #
Total comments: 2
Patch Set 3 : rebase + treib@ comment. #
Dependent Patchsets: Messages
Total messages: 23 (13 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: + treib@chromium.org
Hi Marc, PTAL.
https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:237: // Even not related to downloads offline pages are queried here, so that Offline pages which are not related to downloads are also queried here, so that... ? https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:238: // they can be returned in case there are problems with dismissing. I don't quite understand this. You're saying, this also returns Recent Tabs, because that might be helpful when debugging? https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:779: *(offline_pages_model()->mutable_items()) = Should the "is added back" comment be here? https://codereview.chromium.org/2513393002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:145: // Even not related to recent tab offline pages are queried here, so that Also here: IMO this is hard to parse, maybe "Offline pages not related to Recent Tabs are also queried here, ..." ?
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/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:237: // Even not related to downloads offline pages are queried here, so that On 2016/11/21 12:27:53, Marc Treib wrote: > Offline pages which are not related to downloads are also queried here, so > that... > ? Done. https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:238: // they can be returned in case there are problems with dismissing. On 2016/11/21 12:27:53, Marc Treib wrote: > I don't quite understand this. You're saying, this also returns Recent Tabs, > because that might be helpful when debugging? Yes, while testing, I would say. My arguments: 1) the function contract does not say that the returned suggestions must be related to downloads, so I do not see why we should add this implicit assumption; 2) this function is used only for testing, so no performance drawbacks. Overall, I do not like the idea of returning |ContentSuggestion|'s (because as you saw in the tests, this forces us to keep all suggestions in the data source), but I do not have a better approach (IDs seem to be an implementation detail). https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:779: *(offline_pages_model()->mutable_items()) = On 2016/11/21 12:27:53, Marc Treib wrote: > Should the "is added back" comment be here? Done. https://codereview.chromium.org/2513393002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:145: // Even not related to recent tab offline pages are queried here, so that On 2016/11/21 12:27:53, Marc Treib wrote: > Also here: IMO this is hard to parse, maybe > "Offline pages not related to Recent Tabs are also queried here, ..." ? Done. I did it similarly to your comment in DownloadsProvider.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM - I'll leave the decision about dismissed suggestions to you. https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:238: // they can be returned in case there are problems with dismissing. On 2016/11/21 14:56:46, vitaliii wrote: > On 2016/11/21 12:27:53, Marc Treib wrote: > > I don't quite understand this. You're saying, this also returns Recent Tabs, > > because that might be helpful when debugging? > > Yes, while testing, I would say. > My arguments: > 1) the function contract does not say that the returned suggestions must be > related to downloads, so I do not see why we should add this implicit > assumption; Well, it's a method on the Provider, so I'd say it is not-so-implicitly assumed that it would only return this provider's suggestions. I can see that it might be useful anyway, given that two providers share a data source. But it could also be confusing to see dismissed downloads, even though you've never dismissed a download... > 2) this function is used only for testing, so no performance drawbacks. Yeah, I'm not worried about that. > Overall, I do not like the idea of returning |ContentSuggestion|'s (because as > you saw in the tests, this forces us to keep all suggestions in the data > source), but I do not have a better approach (IDs seem to be an implementation > detail). Yes, the GetDismissedSuggestionsForDebugging implementation has proven to be quite a burden for several providers. Maybe we should rethink if it's worth all the effort.
Hey Marc, I replied to your comment, could you please take a quick look? https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:238: // they can be returned in case there are problems with dismissing. On 2016/11/21 15:38:10, Marc Treib wrote: > On 2016/11/21 14:56:46, vitaliii wrote: > > On 2016/11/21 12:27:53, Marc Treib wrote: > > > I don't quite understand this. You're saying, this also returns Recent Tabs, > > > because that might be helpful when debugging? > > > > Yes, while testing, I would say. > > My arguments: > > 1) the function contract does not say that the returned suggestions must be > > related to downloads, so I do not see why we should add this implicit > > assumption; > > Well, it's a method on the Provider, so I'd say it is not-so-implicitly assumed > that it would only return this provider's suggestions. > I can see that it might be useful anyway, given that two providers share a data > source. But it could also be confusing to see dismissed downloads, even though > you've never dismissed a download... Hm, it seems like we misunderstood each other. I clearly agree that |GetDismissedSuggestionsForDebugging| should not return unrelated to this provider suggestions. My point is that the main reason for this must be that the dismissed suggestions code works correctly, but not that we explicitly decided not to return "wrong" suggestions even if they are actually dismissed. In other words, my argument is that it must have an ability to return not related to downloads offline pages, if they are actually dismissed. However, by querying only subset of offline pages we take away this ability.
https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:238: // they can be returned in case there are problems with dismissing. On 2016/11/21 15:49:55, vitaliii wrote: > On 2016/11/21 15:38:10, Marc Treib wrote: > > On 2016/11/21 14:56:46, vitaliii wrote: > > > On 2016/11/21 12:27:53, Marc Treib wrote: > > > > I don't quite understand this. You're saying, this also returns Recent > Tabs, > > > > because that might be helpful when debugging? > > > > > > Yes, while testing, I would say. > > > My arguments: > > > 1) the function contract does not say that the returned suggestions must be > > > related to downloads, so I do not see why we should add this implicit > > > assumption; > > > > Well, it's a method on the Provider, so I'd say it is not-so-implicitly > assumed > > that it would only return this provider's suggestions. > > I can see that it might be useful anyway, given that two providers share a > data > > source. But it could also be confusing to see dismissed downloads, even though > > you've never dismissed a download... > > Hm, it seems like we misunderstood each other. > > I clearly agree that |GetDismissedSuggestionsForDebugging| should not return > unrelated to this provider suggestions. My point is that the main reason for > this must be that the dismissed suggestions code works correctly, but not that > we explicitly decided not to return "wrong" suggestions even if they are > actually dismissed. > > In other words, my argument is that it must have an ability to return not > related to downloads offline pages, if they are actually dismissed. However, by > querying only subset of offline pages we take away this ability. Ah, I think I see your point now. No objections then, though the comment could maybe be a bit clearer :) https://codereview.chromium.org/2513393002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:277: suggestions.push_back(ConvertOfflinePage(item)); Random note: Does this need a rebase onto your other CL, or did that miss some "if"s? :)
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/2513393002/#ps40001 (title: "rebase + treib@ comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
No need to look. https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:238: // they can be returned in case there are problems with dismissing. On 2016/11/21 15:56:57, Marc Treib wrote: > On 2016/11/21 15:49:55, vitaliii wrote: > > On 2016/11/21 15:38:10, Marc Treib wrote: > > > On 2016/11/21 14:56:46, vitaliii wrote: > > > > On 2016/11/21 12:27:53, Marc Treib wrote: > > > > > I don't quite understand this. You're saying, this also returns Recent > > Tabs, > > > > > because that might be helpful when debugging? > > > > > > > > Yes, while testing, I would say. > > > > My arguments: > > > > 1) the function contract does not say that the returned suggestions must > be > > > > related to downloads, so I do not see why we should add this implicit > > > > assumption; > > > > > > Well, it's a method on the Provider, so I'd say it is not-so-implicitly > > assumed > > > that it would only return this provider's suggestions. > > > I can see that it might be useful anyway, given that two providers share a > > data > > > source. But it could also be confusing to see dismissed downloads, even > though > > > you've never dismissed a download... > > > > Hm, it seems like we misunderstood each other. > > > > I clearly agree that |GetDismissedSuggestionsForDebugging| should not return > > unrelated to this provider suggestions. My point is that the main reason for > > this must be that the dismissed suggestions code works correctly, but not that > > we explicitly decided not to return "wrong" suggestions even if they are > > actually dismissed. > > > > In other words, my argument is that it must have an ability to return not > > related to downloads offline pages, if they are actually dismissed. However, > by > > querying only subset of offline pages we take away this ability. > > Ah, I think I see your point now. No objections then, though the comment could > maybe be a bit clearer :) Done. https://codereview.chromium.org/2513393002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2513393002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:277: suggestions.push_back(ConvertOfflinePage(item)); On 2016/11/21 15:56:57, Marc Treib wrote: > Random note: Does this need a rebase onto your other CL, or did that miss some > "if"s? :) Done. Rebase.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479796171985930,
"parent_rev": "ea5c32c190ca1835dbbfa55e9b8e721f7d59fa15", "commit_rev":
"e7c0ea7174fa8a86f962258f0adb4142f3e55cc7"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [NTP] Cleanup: offline pages related tests. During the move from old offline pages interface to the new one, a few tests were noncritically altered, this CL cleans them up. BUG=None ========== to ========== [NTP] Cleanup: offline pages related tests. During the move from old offline pages interface to the new one, a few tests were noncritically altered, this CL cleans them up. BUG=None Committed: https://crrev.com/167268f8c5adff06b1e6fb9994fb53c8aac80792 Cr-Commit-Position: refs/heads/master@{#433809} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/167268f8c5adff06b1e6fb9994fb53c8aac80792 Cr-Commit-Position: refs/heads/master@{#433809} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
