Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684973014:
Only show Last N Pages in the UI when the corresponding tab is visible. (Closed)
|
Created:
3 years, 10 months ago by dewittj Modified:
3 years, 9 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, noyau+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org, fgorski Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly show Last N Pages in the UI when the corresponding tab is visible.
This patch takes two approaches:
* NTP suggestions now use a DownloadUIAdapter to manage the set of
items on the page. This adapter is notified about tab addition
and removal, and only notifies about pages that correspond to a
tab.
* Deletes pages when a tab is closed.
Patch Set 1 #Patch Set 2 : Add unit tests #
Total comments: 60
Patch Set 3 : Address comments. #
Total comments: 10
Patch Set 4 : rework delegate ownership. #Patch Set 5 : Move impl out #
Total comments: 2
Messages
Total messages: 60 (42 generated)
The CQ bit was checked by dewittj@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dewittj@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 checked by dewittj@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.
The CQ bit was checked by dewittj@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 checked by dewittj@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...
Description was changed from ========== Make recent tabs work. BUG= ========== to ========== Only show Last N Pages in the UI when the corresponding tab is visible. This patch takes two approaches: * NTP suggestions now use a DownloadUIAdapter to manage the set of items on the page. This adapter is notified about tab addition and removal, and only notifies about pages that correspond to a tab. * Deletes pages when a tab is closed. ==========
dewittj@chromium.org changed reviewers: + dimich@chromium.org
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 dewittj@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
dimich: PTAL fgorski/carlosk: FYI Will add bauerb@ and tedchoc@ when you approve.
I think fgorski and carlosk are not on reviewer list, but the comment indicates you wanted to add them. Few comments: https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/downloads/download_ui_adapter.h:122: ItemInfo(Delegate* delegate, const OfflinePageItem& page); It'd be nice to avoid passing a delegate simply to pull the 'temporary hidden' bit. Rather can pass a bit. ItmeInfo is good as a data bag, and it doesn't keep pointers to Adapter nor Delegate, so it'd be nice to keep it this way. https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:16: extra empty line https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:36: RecentTabsUIAdapterDelegate* delegate_ptr = delegate.get(); This looks like a very painful hook up. Since you already add DownloadUIAdapter::delegate(), you don't seem to need delegate_ptr which is used after std::move(delegate)... https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2016 -> 2017 Same for other files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
carlosk@chromium.org changed reviewers: + carlosk@chromium.org
RSLGTM. https://codereview.chromium.org/2684973014/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2684973014/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:775: sTabModelObservers.put(activity, new RecentTabTracker(tabModelSelector)); nit: you can move this put call in place of the get above, keeping the if-block as is. When put replaces a value that old value is returned. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:216: const std::vector<OfflinePageItem>& offline_pages) {} It seems this method could be removed.
https://codereview.chromium.org/2684973014/diff/120001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2684973014/diff/120001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:45: #include "components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h" Shouldn't this be in OS_ANDROID block? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:19: #include "components/offline_pages/core/offline_page_item.h" I guess this won't be needed once you remove GetPagesMatchingQueryCallbackForFetchRecentTabs. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:20: #include "components/offline_pages/core/offline_page_model_query.h" Please remove this line. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:82: recent_tabs_ui_adapter_ = offline_pages::RecentTabsUIAdapterDelegate:: Please do this in the factory and do not propagate offline_page_model and request_coordinator to the provider. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:91: // static Please remove this comment. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:167: recent_tabs_ui_adapter_->GetOfflineIdByGuid(item->guid); Do I understand right that there may be different pages with same GUID, but not at the same time? E.g. when one navigates the tab to a different URL the new offline page will get the same GUID as the old one? I though about dismissing by GUID, but if my understanding above is right, then this is not possible. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:197: void RecentTabSuggestionsProvider::ItemAdded(const DownloadUIItem& ui_item) { Is this called before |ItemsLoaded|? If yes, we should not query RecentTabs before ItemsLoaded. Otherwise, we will prune dismissed IDs on incomplete list of recent tabs. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:210: if (category_status_ != CategoryStatus::NOT_PROVIDED) Please add curly brackets { }. We use curly brackets even for one line bodies. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h:78: // Manually requests all offline pages and updates the suggestions. s/offline pages/ui utems https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h:81: // Converts an OfflinePageItem to a ContentSuggestion for the s/OfflinePageItem/DownloadUIItem https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:48: int current_tab_id = 100; Since tab_id is never checked in the tests explicitly, you could use tab_id = offline_id + 1000 instead without having a global variable. But still please leave the comment. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:112: offline_pages::RecentTabsUIAdapterDelegate* delegate_ = Why does this have trailing underscore? Should we have |delegate_| member and initialize it in the constructor? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:134: RemoveTab(tab_id); Wouldn't removing tab before |OfflinePageDeleted| be closer to reality? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:173: for (OfflinePageItem& recent_tab : recent_tabs_list) Please add curly brackets { }. In NTP we use curly brackets even for one line bodies. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:235: Mock::VerifyAndClearExpectations(observer()); Why is this needed here? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:356: EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0); Removal of the recent tab changes meaning a bit. Currently, one may do the fetch, but not report anything because there is no RecentTabs. Is there any way to add a recent tab in this test without notifying observers? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:363: // The provider is not notified about the first recent tab yet. I am confused. In the test above you removed exactly the same line. Is there any difference between these two tests? Also |AddTabAndOfflinePageToModel| contradicts the comment - it (implicitly) notifies the provider. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:377: TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowPagesWithoutTab) { Could you also test that InvalidateSuggestion is fired on tab removal? Probably in a different test. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:381: AddOfflinePageToModel(tab); I guess this actually notifies the provider and so contradicts the comment. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:395: OfflinePageItem tab2 = CreateDummyRecentTab(2); Could you please use "first_tab" and "second_tab" variable names? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:408: // Remove Tab by itself doesn't cause OnNewSuggestions to be called. |RemoveTab| or "Tab removal" If it does not cause OnNewSuggestions, please move EXPECT_CALL(OnNewSuggestions ...) after it. https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/downloads/download_ui_adapter.cc:311: deleting_item_ = std::move(it->second); This deleting_item_ approach feels a bit weird. Since it is used only for obtaining offline_id from GUID, why not providing offline_id in the ItemDeleted notification?
The CQ bit was checked by dewittj@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...
dewittj@chromium.org changed reviewers: + vitaliii@chromium.org
I added fgorski and carlosk on CC list, since they were not required. Now carlosk has added himself to the r= list :) PTAL! https://codereview.chromium.org/2684973014/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2684973014/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:775: sTabModelObservers.put(activity, new RecentTabTracker(tabModelSelector)); On 2017/02/15 01:20:54, carlosk wrote: > nit: you can move this put call in place of the get above, keeping the if-block > as is. When put replaces a value that old value is returned. Done. https://codereview.chromium.org/2684973014/diff/120001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2684973014/diff/120001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:45: #include "components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h" On 2017/02/15 10:12:15, vitaliii wrote: > Shouldn't this be in OS_ANDROID block? Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:19: #include "components/offline_pages/core/offline_page_item.h" On 2017/02/15 10:12:15, vitaliii wrote: > I guess this won't be needed once you remove > GetPagesMatchingQueryCallbackForFetchRecentTabs. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:20: #include "components/offline_pages/core/offline_page_model_query.h" On 2017/02/15 10:12:15, vitaliii wrote: > Please remove this line. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:82: recent_tabs_ui_adapter_ = offline_pages::RecentTabsUIAdapterDelegate:: On 2017/02/15 10:12:15, vitaliii wrote: > Please do this in the factory and do not propagate offline_page_model and > request_coordinator to the provider. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:91: // static On 2017/02/15 10:12:15, vitaliii wrote: > Please remove this comment. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:167: recent_tabs_ui_adapter_->GetOfflineIdByGuid(item->guid); On 2017/02/15 10:12:15, vitaliii wrote: > Do I understand right that there may be different pages with same GUID, but not > at the same time? E.g. when one navigates the tab to a different URL the new > offline page will get the same GUID as the old one? > > I though about dismissing by GUID, but if my understanding above is right, then > this is not possible. Yes, this is currently a limitation with how we interact with DownloadUIAdapter. In fact GUID is misnamed once we start doing this. I will discuss with Dmitry about changing guid to something else now. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:197: void RecentTabSuggestionsProvider::ItemAdded(const DownloadUIItem& ui_item) { On 2017/02/15 10:12:15, vitaliii wrote: > Is this called before |ItemsLoaded|? > If yes, we should not query RecentTabs before ItemsLoaded. Otherwise, we will > prune dismissed IDs on incomplete list of recent tabs. No, the design of DownloadUIAdapter is that ItemsLoaded is 1) Always called for new observers even if the adapter is already loaded 2) Always called first. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:210: if (category_status_ != CategoryStatus::NOT_PROVIDED) On 2017/02/15 10:12:15, vitaliii wrote: > Please add curly brackets { }. > We use curly brackets even for one line bodies. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc:216: const std::vector<OfflinePageItem>& offline_pages) {} On 2017/02/15 01:20:54, carlosk wrote: > It seems this method could be removed. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h:78: // Manually requests all offline pages and updates the suggestions. On 2017/02/15 10:12:15, vitaliii wrote: > s/offline pages/ui utems Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h:81: // Converts an OfflinePageItem to a ContentSuggestion for the On 2017/02/15 10:12:15, vitaliii wrote: > s/OfflinePageItem/DownloadUIItem Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:48: int current_tab_id = 100; On 2017/02/15 10:12:16, vitaliii wrote: > Since tab_id is never checked in the tests explicitly, you could use tab_id = > offline_id + 1000 instead without having a global variable. But still please > leave the comment. Good idea, done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:112: offline_pages::RecentTabsUIAdapterDelegate* delegate_ = On 2017/02/15 10:12:15, vitaliii wrote: > Why does this have trailing underscore? > Should we have |delegate_| member and initialize it in the constructor? Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:134: RemoveTab(tab_id); On 2017/02/15 10:12:15, vitaliii wrote: > Wouldn't removing tab before |OfflinePageDeleted| be closer to reality? Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:173: for (OfflinePageItem& recent_tab : recent_tabs_list) On 2017/02/15 10:12:15, vitaliii wrote: > Please add curly brackets { }. In NTP we use curly brackets even for one line > bodies. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:235: Mock::VerifyAndClearExpectations(observer()); On 2017/02/15 10:12:15, vitaliii wrote: > Why is this needed here? I think I added it during testing. It separates the concerns of the "setup" phase from the actual testing. Do you know whether having Times(0) from the below EXPECT_CALL resets the expected number of calls, or whether it just adds 0 to the expected number of calls? Will remove for now. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:356: EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0); On 2017/02/15 10:12:16, vitaliii wrote: > Removal of the recent tab changes meaning a bit. Currently, one may do the > fetch, but not report anything because there is no RecentTabs. Is there any way > to add a recent tab in this test without notifying observers? It's not clear what exactly you are trying to test. What behavior are you trying to simulate by adding a recent tab without notifying? https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:363: // The provider is not notified about the first recent tab yet. On 2017/02/15 10:12:15, vitaliii wrote: > I am confused. In the test above you removed exactly the same line. Is there any > difference between these two tests? > > Also |AddTabAndOfflinePageToModel| contradicts the comment - it (implicitly) > notifies the provider. I think the premise of this test doesn't exist any more, you are always notified of recent tabs now. What I didn't test was the behavior on load, so changed this test to reflect that. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:377: TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowPagesWithoutTab) { On 2017/02/15 10:12:15, vitaliii wrote: > Could you also test that InvalidateSuggestion is fired on tab removal? Probably > in a different test. Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:381: AddOfflinePageToModel(tab); On 2017/02/15 10:12:15, vitaliii wrote: > I guess this actually notifies the provider and so contradicts the comment. No, this doesn't add the tab, but the name of the offline page item is misleading. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:395: OfflinePageItem tab2 = CreateDummyRecentTab(2); On 2017/02/15 10:12:16, vitaliii wrote: > Could you please use "first_tab" and "second_tab" variable names? Done. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:408: // Remove Tab by itself doesn't cause OnNewSuggestions to be called. On 2017/02/15 10:12:15, vitaliii wrote: > |RemoveTab| or "Tab removal" > > If it does not cause OnNewSuggestions, please move EXPECT_CALL(OnNewSuggestions > ...) after it. Done. I added another expectation in addition. https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/downloads/download_ui_adapter.cc:311: deleting_item_ = std::move(it->second); On 2017/02/15 10:12:16, vitaliii wrote: > This deleting_item_ approach feels a bit weird. > Since it is used only for obtaining offline_id from GUID, why not providing > offline_id in the ItemDeleted notification? I'm thinking of a future scenario where we stop using tab ID as the GUID and provide a helper for getting the tab ID from a GUID. Then that would also need to be passed to ItemDeleted. Perhaps it should just pass the whole ItemInfo struct back. Dmitry, any thoughts about this? https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/downloads/download_ui_adapter.h (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/downloads/download_ui_adapter.h:122: ItemInfo(Delegate* delegate, const OfflinePageItem& page); On 2017/02/14 23:01:03, Dmitry Titov wrote: > It'd be nice to avoid passing a delegate simply to pull the 'temporary hidden' > bit. Rather can pass a bit. > > ItmeInfo is good as a data bag, and it doesn't keep pointers to Adapter nor > Delegate, so it'd be nice to keep it this way. Done. https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:16: On 2017/02/14 23:01:03, Dmitry Titov wrote: > extra empty line Done. https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:36: RecentTabsUIAdapterDelegate* delegate_ptr = delegate.get(); On 2017/02/14 23:01:03, Dmitry Titov wrote: > This looks like a very painful hook up. > Since you already add DownloadUIAdapter::delegate(), you don't seem to need > delegate_ptr which is used after std::move(delegate)... delegate() is not the right type exactly, so that strategy would require a static_cast. What do you think of this alternate approach? https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h (right): https://codereview.chromium.org/2684973014/diff/120001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/14 23:01:03, Dmitry Titov wrote: > 2016 -> 2017 > Same for other files. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ntp_snippets -> LGTM with nits. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:235: Mock::VerifyAndClearExpectations(observer()); On 2017/02/15 19:49:45, dewittj wrote: > On 2017/02/15 10:12:15, vitaliii wrote: > > Why is this needed here? > > I think I added it during testing. It separates the concerns of the "setup" > phase from the actual testing. Do you know whether having Times(0) from the > below EXPECT_CALL resets the expected number of calls, or whether it just adds 0 > to the expected number of calls? > > Will remove for now. I guess it implicitly resets the expected number of call. In this piece of code: EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3); AddOfflinePageToModel(CreateDummyRecentTab(2)); AddOfflinePageToModel(CreateDummyRecentTab(3)); EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0); AddOfflinePageToModel(CreateDummyRecentTab(4)); Both EXPECT_CALLs are unsatisfied in the end. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:356: EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0); On 2017/02/15 19:49:45, dewittj wrote: > On 2017/02/15 10:12:16, vitaliii wrote: > > Removal of the recent tab changes meaning a bit. Currently, one may do the > > fetch, but not report anything because there is no RecentTabs. Is there any > way > > to add a recent tab in this test without notifying observers? > It's not clear what exactly you are trying to test. What behavior are you > trying to simulate by adding a recent tab without notifying? Previously, if an offline page is added, the provider should query all pages only if it was a recent tab. As you said below, now we are notified only about recent tabs, so please remove this test. https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:363: // The provider is not notified about the first recent tab yet. On 2017/02/15 19:49:45, dewittj wrote: > On 2017/02/15 10:12:15, vitaliii wrote: > > I am confused. In the test above you removed exactly the same line. Is there > any > > difference between these two tests? > > > > Also |AddTabAndOfflinePageToModel| contradicts the comment - it (implicitly) > > notifies the provider. > > I think the premise of this test doesn't exist any more, you are always notified > of recent tabs now. What I didn't test was the behavior on load, so changed this > test to reflect that. ACK. Yes, fair enough! https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:365: TEST_F(RecentTabSuggestionsProviderTestNoLoad, ShouldFetchOnLoad) { Could you please add a comment saying that this test class does not have SetUp (and to bring attention that this is actually a different test class from other tests)? At first, I missed that there are 2 test classes and was confused how this worked at all. https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:366: // We should only fetch once, when the adapter is loaded. s/ We should/ On startup we should https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:375: AddTabAndOfflinePageToModel(CreateDummyRecentTab(1)); Could you please add recent tabs before EXPECT_CALL?
https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2684973014/diff/120001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:235: Mock::VerifyAndClearExpectations(observer()); On 2017/02/16 08:11:04, vitaliii wrote: > On 2017/02/15 19:49:45, dewittj wrote: > > On 2017/02/15 10:12:15, vitaliii wrote: > > > Why is this needed here? > > > > I think I added it during testing. It separates the concerns of the "setup" > > phase from the actual testing. Do you know whether having Times(0) from the > > below EXPECT_CALL resets the expected number of calls, or whether it just adds > 0 > > to the expected number of calls? > > > > Will remove for now. > > I guess it implicitly resets the expected number of call. > > In this piece of code: > > EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3); > AddOfflinePageToModel(CreateDummyRecentTab(2)); > AddOfflinePageToModel(CreateDummyRecentTab(3)); > EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0); > AddOfflinePageToModel(CreateDummyRecentTab(4)); > > Both EXPECT_CALLs are unsatisfied in the end. drive-by: gmock builds up a stack of expectations. When a new call comes, it traverses the stack and picks the first one that matches (argument matches). Because you're using OnNewSuggestions() with the wildcard-matchers ('_'), the last expectation in the stack will always 'win' -- even if it's already saturated. So adding a new .Times(0) expectation will simply shadow the older one. I find this a bit brittle though. If you care about methods not being called, it might be worth considering using a StrictMock(). However, this might require changes to existing tests and hence not be suitable for this CL. In this particular case, I also think the call to VerifyAndClearExpectations is not necessary. (also note, that when dealing with interleaving calls, you can also specify .RetiresOnSaturation() on a EXPECT_CALL() statement which removes an expectation from the expectation-stack once saturated).
https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc (right): https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:38: delegate->set_ui_adapter(recent_tabs_ui_adapter); That is even more hair-raising :) How about instead of passing delegate into adapter's ctor we have adapter::SetDelegate(std::unique_ptr<Delegate>)? Then it would look like: adapter = new Adapter() delegate = new Delegate(..., adapter) adapter->SetDelegate(delegate) WDYT?
https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc (right): https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:38: delegate->set_ui_adapter(recent_tabs_ui_adapter); On 2017/02/16 23:30:41, Dmitry Titov wrote: > That is even more hair-raising :) > > How about instead of passing delegate into adapter's ctor we have > adapter::SetDelegate(std::unique_ptr<Delegate>)? > > Then it would look like: > > adapter = new Adapter() > delegate = new Delegate(..., adapter) > adapter->SetDelegate(delegate) > > WDYT? Drive-by: [started reading this in wrong context, but wanted to share a different in opinion (now that I already looked at it) -- feel free to ignore though)]. The RegisterTab/UnregisterTab functions seems misplaced on this class (and are the reason why you need the delegate needs to know the ui_adapter). My understanding is that the idea of the delegate is to customize the adaptor (see class comment at the Delegate interface). There are 2 smells: -- why does the bridge call into the delegate and not the adaptor itself. Shouldn't the delegate be considered an implementation detail? -- having the delegate initiate an interaction to the adaptor proposal: -- have the bridge access the adaptor -- Why not move the RegisterTab/UnregisterTab functions into the adaptor and let the adaptor notify the delegate about the change? -- no need for the delegate to know the adaptor :-)
The CQ bit was checked by dewittj@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 checked by dewittj@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...
https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... File components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:365: TEST_F(RecentTabSuggestionsProviderTestNoLoad, ShouldFetchOnLoad) { On 2017/02/16 08:11:04, vitaliii wrote: > Could you please add a comment saying that this test class does not have SetUp > (and to bring attention that this is actually a different test class from other > tests)? > > At first, I missed that there are 2 test classes and was confused how this > worked at all. Done. Also moved to the bottom to separate the two types of test fixtures. https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:366: // We should only fetch once, when the adapter is loaded. On 2017/02/16 08:11:04, vitaliii wrote: > s/ We should/ On startup we should Reworded. https://codereview.chromium.org/2684973014/diff/140001/components/ntp_snippet... components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc:375: AddTabAndOfflinePageToModel(CreateDummyRecentTab(1)); On 2017/02/16 08:11:04, vitaliii wrote: > Could you please add recent tabs before EXPECT_CALL? In this top location, it catches improper |OnNewSuggestions| calls made before load. So I prefer the EXPECT at the top. https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc (right): https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:38: delegate->set_ui_adapter(recent_tabs_ui_adapter); On 2017/02/17 11:30:35, tschumann wrote: > On 2017/02/16 23:30:41, Dmitry Titov wrote: > > That is even more hair-raising :) > > > > How about instead of passing delegate into adapter's ctor we have > > adapter::SetDelegate(std::unique_ptr<Delegate>)? > > > > Then it would look like: > > > > adapter = new Adapter() > > delegate = new Delegate(..., adapter) > > adapter->SetDelegate(delegate) > > > > WDYT? > > Drive-by: [started reading this in wrong context, but wanted to share a > different in opinion (now that I already looked at it) -- feel free to ignore > though)]. > > The RegisterTab/UnregisterTab functions seems misplaced on this class (and are > the reason why you need the delegate needs to know the ui_adapter). > My understanding is that the idea of the delegate is to customize the adaptor > (see class comment at the Delegate interface). > There are 2 smells: > -- why does the bridge call into the delegate and not the adaptor itself. > Shouldn't the delegate be considered an implementation detail? > -- having the delegate initiate an interaction to the adaptor > > proposal: > -- have the bridge access the adaptor > -- Why not move the RegisterTab/UnregisterTab functions into the adaptor and let > the adaptor notify the delegate about the change? > -- no need for the delegate to know the adaptor :-) I think it's important to maintain the separation of concerns here: RecentTabsUIAdapterDelegate is the only part of the system that knows about tabs. I rearranged things so that there are no more pointer shenanigans, please let me know what you think.
PTAL, thanks! tedchoc: ChromeActivity.java - This replaces the earlier patch (from https://codereview.chromium.org/2639403003/) and just has a single entry point for Offline Pages. Let me know if this is more like what you'd expect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
offline pages lgtm
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:773: RecentTabTracker previousObserver = I think you can get rid of the weakhashmap (and make it just a hashmap) and just listen for activity changes. When they get destroyed, remove the entry from the map and destroy the observer. You can look at ApplicationStatus to listen for state changes about the activity state changes.
dewittj@ is OOO this week and I'm working on another patch that depends on this one landing. So I will try to take over it and land it. https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:773: RecentTabTracker previousObserver = On 2017/02/22 23:28:04, Ted C wrote: > I think you can get rid of the weakhashmap (and make it just a hashmap) and just > listen for activity changes. When they get destroyed, remove the entry from the > map and destroy the observer. > > You can look at ApplicationStatus to listen for state changes about the activity > state changes. Looking at the related code it seems that TabWindowManager and TabModelSelectorUma both keep strong references to the Activity (the latter, through ApplicationStatus.registerStateListenerForActivity). But also both of them do listen for the activity lifecycle and eliminate that reference when it's destroyed (see [1] and [2], respectively). What might still be a problem though is that RecentTabTracker::destroy would not be called but maybe that could be resolved by calling it from a finalize() implementation? However, even with all comments above, it seems that all other cases where TabModelSelectorTabModelObserver is extended they do listen to the activity being destroyed and to cleanup after themselves "manually". In conclusion: even though it seems it should work as is I am implementing the scheme you suggested to play it safe, but wanted to also share my findings to ask you WDYT? [1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... [2] https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
On 2017/02/23 03:46:47, carlosk wrote: > dewittj@ is OOO this week and I'm working on another patch that depends on this > one landing. So I will try to take over it and land it. > > https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java > (right): > > https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:773: > RecentTabTracker previousObserver = > On 2017/02/22 23:28:04, Ted C wrote: > > I think you can get rid of the weakhashmap (and make it just a hashmap) and > just > > listen for activity changes. When they get destroyed, remove the entry from > the > > map and destroy the observer. > > > > You can look at ApplicationStatus to listen for state changes about the > activity > > state changes. > > Looking at the related code it seems that TabWindowManager and > TabModelSelectorUma both keep strong references to the Activity (the latter, > through ApplicationStatus.registerStateListenerForActivity). But also both of > them do listen for the activity lifecycle and eliminate that reference when it's > destroyed (see [1] and [2], respectively). > > What might still be a problem though is that RecentTabTracker::destroy would not > be called but maybe that could be resolved by calling it from a finalize() > implementation? > > However, even with all comments above, it seems that all other cases where > TabModelSelectorTabModelObserver is extended they do listen to the activity > being destroyed and to cleanup after themselves "manually". > > In conclusion: even though it seems it should work as is I am implementing the > scheme you suggested to play it safe, but wanted to also share my findings to > ask you WDYT? > > > [1] > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > [2] > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... RecentTabTracker holds a strong ref to TabModelSelector, TabModelSelector holds a strong ref to TabCreatorManager: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... ChromeActivity is the implementor of TabCreatorManager: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... So, I still think this is required not a nice to have.
On 2017/02/23 18:36:02, Ted C (OOO till Monday) wrote: > On 2017/02/23 03:46:47, carlosk wrote: > > dewittj@ is OOO this week and I'm working on another patch that depends on > this > > one landing. So I will try to take over it and land it. > > > > > https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java > > (right): > > > > > https://codereview.chromium.org/2684973014/diff/180001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:773: > > RecentTabTracker previousObserver = > > On 2017/02/22 23:28:04, Ted C wrote: > > > I think you can get rid of the weakhashmap (and make it just a hashmap) and > > just > > > listen for activity changes. When they get destroyed, remove the entry from > > the > > > map and destroy the observer. > > > > > > You can look at ApplicationStatus to listen for state changes about the > > activity > > > state changes. > > > > Looking at the related code it seems that TabWindowManager and > > TabModelSelectorUma both keep strong references to the Activity (the latter, > > through ApplicationStatus.registerStateListenerForActivity). But also both of > > them do listen for the activity lifecycle and eliminate that reference when > it's > > destroyed (see [1] and [2], respectively). > > > > What might still be a problem though is that RecentTabTracker::destroy would > not > > be called but maybe that could be resolved by calling it from a finalize() > > implementation? > > > > However, even with all comments above, it seems that all other cases where > > TabModelSelectorTabModelObserver is extended they do listen to the activity > > being destroyed and to cleanup after themselves "manually". > > > > In conclusion: even though it seems it should work as is I am implementing the > > scheme you suggested to play it safe, but wanted to also share my findings to > > ask you WDYT? > > > > > > [1] > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > [2] > > > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > > RecentTabTracker holds a strong ref to TabModelSelector, TabModelSelector holds > a strong ref to TabCreatorManager: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > ChromeActivity is the implementor of TabCreatorManager: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > So, I still think this is required not a nice to have. OK. I created a new CL to address this issue here: https://crrev.com/2706343007.
lgtm https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... File components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc (right): https://codereview.chromium.org/2684973014/diff/140001/components/offline_pag... components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.cc:38: delegate->set_ui_adapter(recent_tabs_ui_adapter); On 2017/02/17 22:33:38, dewittj (OOO 2.20-2.24) wrote: > On 2017/02/17 11:30:35, tschumann wrote: > > On 2017/02/16 23:30:41, Dmitry Titov wrote: > > > That is even more hair-raising :) > > > > > > How about instead of passing delegate into adapter's ctor we have > > > adapter::SetDelegate(std::unique_ptr<Delegate>)? > > > > > > Then it would look like: > > > > > > adapter = new Adapter() > > > delegate = new Delegate(..., adapter) > > > adapter->SetDelegate(delegate) > > > > > > WDYT? > > > > Drive-by: [started reading this in wrong context, but wanted to share a > > different in opinion (now that I already looked at it) -- feel free to ignore > > though)]. > > > > The RegisterTab/UnregisterTab functions seems misplaced on this class (and are > > the reason why you need the delegate needs to know the ui_adapter). > > My understanding is that the idea of the delegate is to customize the adaptor > > (see class comment at the Delegate interface). > > There are 2 smells: > > -- why does the bridge call into the delegate and not the adaptor itself. > > Shouldn't the delegate be considered an implementation detail? > > -- having the delegate initiate an interaction to the adaptor > > > > proposal: > > -- have the bridge access the adaptor > > -- Why not move the RegisterTab/UnregisterTab functions into the adaptor and > let > > the adaptor notify the delegate about the change? > > -- no need for the delegate to know the adaptor :-) > > I think it's important to maintain the separation of concerns here: > RecentTabsUIAdapterDelegate is the only part of the system that knows about > tabs. > > I rearranged things so that there are no more pointer shenanigans, please let me > know what you think. I think it's fine as is -- like I said this was primarily my personal opinion. There's a light smell that you have a customizing class that needs to know about the class it customizes. Maybe that's a case where actual inheritance would be the better option. Or splitting out another class that knows about both (the delegate and the adaptor). But maybe it's just too early (this is the first implementation of the delegate, right?) and it's best to wait and see. At any rate, I'm fine with this solution (and don't think I should have a lot to say in your code base anyways). Hope I didn't slow you down!
Message was sent while issue was closed.
thanks for the feedback! This has been superseded by Carlos' patch. Closing this issue. |
