Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570783003:
[Popular Sites] Split PopularSites interface and PopularSitesImpl (Closed)
Description[Popular Sites] Split PopularSites interface and PopularSitesImpl
The introduction of an interface allows dependent classes
(MostVisitedSites) to inject doubles in tests.
BUG=619584
Committed: https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2
Cr-Commit-Position: refs/heads/master@{#438536}
Patch Set 1 #Patch Set 2 : Fix build. #Patch Set 3 : Reverted a few changes. #Patch Set 4 : Split PopularSitesImpl. #
Total comments: 6
Patch Set 5 : Move debug methods to PopularSites interface. #Patch Set 6 : Rebased an resolved conflicts. #Patch Set 7 : Rebased and minor naming change. #Patch Set 8 : Rebased and minor naming change. #Messages
Total messages: 67 (43 generated)
The CQ bit was checked by mastiz@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mastiz@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 ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. BUG=619584 ========== to ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. This has the advantage that PopularSitesImpl can expose an extended API for tests and advanced clients (like popular-sites-internals page), while the PopularSites interface remains simple. BUG=619584 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...)
The CQ bit was checked by mastiz@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mastiz@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 ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. This has the advantage that PopularSitesImpl can expose an extended API for tests and advanced clients (like popular-sites-internals page), while the PopularSites interface remains simple. BUG=619584 ========== to ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. The change also has the advantage that PopularSitesImpl can expose an extended API for tests and advanced clients (like popular-sites-internals page), while the PopularSites interface remains simple. BUG=619584 ==========
mastiz@chromium.org changed reviewers: + sfiera@chromium.org
Builds locally, bots running. I believe ready to review, thx.
https://codereview.chromium.org/2570783003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/2570783003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:22: static std::unique_ptr<ntp_tiles::PopularSitesImpl> NewForProfile( Should the factory be promising that it creates a particular implementation? https://codereview.chromium.org/2570783003/diff/60001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2570783003/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites.h:69: virtual ~PopularSites() = default; I think some bots are going to complain about this being in the header—if so, you'll need to put the = default in a .cc file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2570783003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/2570783003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:22: static std::unique_ptr<ntp_tiles::PopularSitesImpl> NewForProfile( On 2016/12/13 10:53:57, sfiera wrote: > Should the factory be promising that it creates a particular implementation? The alternative would be to move the two remaining functions (local_path, LastURL) to the interface. I didn't like this idea before, but I'm not convinced anymore. WDYT? https://codereview.chromium.org/2570783003/diff/60001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2570783003/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites.h:69: virtual ~PopularSites() = default; On 2016/12/13 10:53:57, sfiera wrote: > I think some bots are going to complain about this being in the header—if so, > you'll need to put the = default in a .cc file. Acknowledged, no complaints so far.
The CQ bit was checked by mastiz@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/2570783003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/2570783003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:22: static std::unique_ptr<ntp_tiles::PopularSitesImpl> NewForProfile( On 2016/12/13 12:14:56, mastiz wrote: > On 2016/12/13 10:53:57, sfiera wrote: > > Should the factory be promising that it creates a particular implementation? > > The alternative would be to move the two remaining functions (local_path, > LastURL) to the interface. I didn't like this idea before, but I'm not convinced > anymore. WDYT? Hmm. They do feel like implementation details, but I think they are only used for diagnostics in the webui, right? I think I'm OK with them being in the interface for that purpose. In the future, maybe the interface can have just base::Value DebugInfo().
The CQ bit was checked by mastiz@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/2570783003/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/2570783003/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:22: static std::unique_ptr<ntp_tiles::PopularSitesImpl> NewForProfile( On 2016/12/13 12:14:56, mastiz wrote: > On 2016/12/13 10:53:57, sfiera wrote: > > Should the factory be promising that it creates a particular implementation? > > The alternative would be to move the two remaining functions (local_path, > LastURL) to the interface. I didn't like this idea before, but I'm not convinced > anymore. WDYT? Went ahead and exported a new patchset following this suggestion. I actually think it's cleaner, and it shouldn't bother future tests much.
LGTM Yes, putting them in the interface looks better to me.
mastiz@chromium.org changed reviewers: + bauerb@chromium.org, sdefresne@chromium.org
sdefresne@chromium.org: Please review changes in ios/ bauerb@chromium.org: Please review changes in chrome/browser Thanks!
Description was changed from ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. The change also has the advantage that PopularSitesImpl can expose an extended API for tests and advanced clients (like popular-sites-internals page), while the PopularSites interface remains simple. BUG=619584 ========== to ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. BUG=619584 ==========
ios/ lgtm (though it's a bit strange that both PopularSites & PopularSitesImpl are defined in the same file)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/13 13:07:20, sdefresne wrote: > ios/ lgtm (though it's a bit strange that both PopularSites & PopularSitesImpl > are defined in the same file) Thx! I was keeping the diff small. If other reviewers agree, I'll move PopularSitesImpl to a dedicated file.
I don't mind if that's handled in a later CL, but you will have to chase down approval from sdefresne and bauerb again to change the #includes in the prefs files :)
android/ and prefs/ LGTM.
The CQ bit was checked by mastiz@chromium.org
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
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...)
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, sfiera@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2570783003/#ps100001 (title: "Rebased an resolved conflicts.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, sfiera@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2570783003/#ps140001 (title: "Rebased and minor naming change.")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mastiz@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mastiz@chromium.org
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mastiz@chromium.org
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": 140001, "attempt_start_ts": 1481731299273750,
"parent_rev": "703dbda49828ab612fef8c803fd6668aa4da2ec6", "commit_rev":
"1be486b69c2269f9a735009a9543527c68355d3b"}
Message was sent while issue was closed.
Description was changed from ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. BUG=619584 ========== to ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. BUG=619584 Review-Url: https://codereview.chromium.org/2570783003 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. BUG=619584 Review-Url: https://codereview.chromium.org/2570783003 ========== to ========== [Popular Sites] Split PopularSites interface and PopularSitesImpl The introduction of an interface allows dependent classes (MostVisitedSites) to inject doubles in tests. BUG=619584 Committed: https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2 Cr-Commit-Position: refs/heads/master@{#438536} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2 Cr-Commit-Position: refs/heads/master@{#438536} |
