Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919383005:
Small cleanup of SiteEngagementService. (Closed)
DescriptionSmall cleanup of SiteEngagementService.
This CL moves pref management into SiteEngagementScore so that
SiteEngagementService has a cleaner flow for operating on
SiteEngagementScores.
BUG=604305
Committed: https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf
Cr-Commit-Position: refs/heads/master@{#396789}
Patch Set 1 : #Patch Set 2 : #
Total comments: 3
Patch Set 3 : start from scratch #Patch Set 4 : const some thangs #
Total comments: 28
Patch Set 5 : address comments #
Total comments: 2
Patch Set 6 : address nit + rebase #
Messages
Total messages: 22 (12 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
dominickn@chromium.org changed reviewers: + dominickn@chromium.org
Meta comment: I need to fix up issues with the score reset not being based on DecayedScore(), and ensuring last shortcut launch time isn't brought forward in time. https://codereview.chromium.org/1919383005/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1919383005/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:626: score->get()->AddPoints(points); Nit: if you're not going to null check, then GetEngagementScore(url, true)->get()->AddPoints(points); https://codereview.chromium.org/1919383005/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1919383005/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:313: bool update); I don't really like this ScopedEngagementScore. - bool in the interface is sad. It's not obvious that |update| means "update the content setting when this goes out of scope" - everything in here could just be moved into SiteEngagementScore, with the destructor stuff in an explicit Save() method - we should also make GetSiteEngagementScore() as as private method in SiteEngagementService, with tests blessed to access it. Then we can test properties of SiteEngagementScore when testing SiteEngagementService (e.g. last shortcut launch time) https://codereview.chromium.org/1919383005/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:349: const base::Time* last_shortcut_launch_time); Having one argument be the raw time and a second argument being a pointer is kind of awful and I'm desperately trying to think of a way we can avoid it.
Description was changed from ========== Small cleanup of SiteEngagementService. BUG=604305 ========== to ========== Small cleanup of SiteEngagementService. This CL moves pref management into SiteEngagementScore so that SiteEngagementService has a cleaner flow for operating on SiteEngagementScores. BUG=604305 ==========
I think this should address all your concerns as discussed.
This is so much cleaner than before! Mostly nits. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:7: #include <memory> for std::unique_ptr https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:144: explicit SiteEngagementScore( Nit: remove explicit and the unit tests comment https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:159: score_map[origin] = score.GetScore(); score_map[origin] = GetScore(origin); Delete the line above. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:262: if (score.GetScore() != 0) if (GetScore(origin) != 0) Delete the line above. Actually, this could just use GetScoreMap(). I don't really mind either way. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:395: if (score.MaxPointsPerDayAdded()) if (GetEngagementScore(origin).MaxPointsPerDayAdded()) https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:431: return; continue? https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:474: engagement_score.set_last_shortcut_launch_time(last_visit); if (!engagement_score.last_shortcut_launch_time().is_null() && engagement_score.last_shortcut_launch_time() > last_visit) https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:96: bool IsBootstrapped(); const https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:102: void ResetScoreForURL(const GURL& url, double score); const https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:106: void SetLastShortcutLaunchTime(const GURL& url); const https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:132: void AddPoints(const GURL& url, double points); const https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:135: SiteEngagementScore GetEngagementScore(const GURL& origin) const; This could be in the anonymous namespace in site_engagement_sevice.cc, taking a clock pointer and origin argument. Then you don't need the forward declaration of SiteEngagementScore. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:177: const history::OriginCountAndLastVisitMap& remaining_origin_counts); Can this method be const now?
Patchset #5 (id:180001) has been deleted
https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:7: On 2016/05/25 06:52:43, dominickn wrote: > #include <memory> for std::unique_ptr Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:144: explicit SiteEngagementScore( On 2016/05/25 06:52:43, dominickn wrote: > Nit: remove explicit and the unit tests comment The comment is still true. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:159: score_map[origin] = score.GetScore(); On 2016/05/25 06:52:43, dominickn wrote: > score_map[origin] = GetScore(origin); > > Delete the line above. Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:262: if (score.GetScore() != 0) On 2016/05/25 06:52:43, dominickn wrote: > if (GetScore(origin) != 0) > > Delete the line above. > > Actually, this could just use GetScoreMap(). I don't really mind either way. Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:395: if (score.MaxPointsPerDayAdded()) On 2016/05/25 06:52:43, dominickn wrote: > if (GetEngagementScore(origin).MaxPointsPerDayAdded()) Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:431: return; On 2016/05/25 06:52:43, dominickn wrote: > continue? Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:474: engagement_score.set_last_shortcut_launch_time(last_visit); On 2016/05/25 06:52:43, dominickn wrote: > if (!engagement_score.last_shortcut_launch_time().is_null() && > engagement_score.last_shortcut_launch_time() > last_visit) Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:96: bool IsBootstrapped(); On 2016/05/25 06:52:43, dominickn wrote: > const Done. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:102: void ResetScoreForURL(const GURL& url, double score); On 2016/05/25 06:52:43, dominickn wrote: > const So.... I don't want this to be const because it's bananas for setters to be const. So I added a const override for GetEngagementScore so that this is no longer const-able. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:135: SiteEngagementScore GetEngagementScore(const GURL& origin) const; On 2016/05/25 06:52:43, dominickn wrote: > This could be in the anonymous namespace in site_engagement_sevice.cc, taking a > clock pointer and origin argument. Then you don't need the forward declaration > of SiteEngagementScore. I'd rather not do that for two reasons: - I'd have to pass clock_ and profile_ at every call which is a bit verbose for no reason. - It would allow const setters again. Blech. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:177: const history::OriginCountAndLastVisitMap& remaining_origin_counts); On 2016/05/25 06:52:43, dominickn wrote: > Can this method be const now? Not anymore ;)
lgtm % optional nit (combine if statement) https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:144: explicit SiteEngagementScore( On 2016/05/26 08:26:20, calamity wrote: > On 2016/05/25 06:52:43, dominickn wrote: > > Nit: remove explicit and the unit tests comment > > The comment is still true. Acknowledged. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:102: void ResetScoreForURL(const GURL& url, double score); On 2016/05/26 08:26:20, calamity wrote: > On 2016/05/25 06:52:43, dominickn wrote: > > const > > So.... I don't want this to be const because it's bananas for setters to be > const. So I added a const override for GetEngagementScore so that this is no > longer const-able. Acknowledged. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:135: SiteEngagementScore GetEngagementScore(const GURL& origin) const; On 2016/05/26 08:26:20, calamity wrote: > On 2016/05/25 06:52:43, dominickn wrote: > > This could be in the anonymous namespace in site_engagement_sevice.cc, taking > a > > clock pointer and origin argument. Then you don't need the forward declaration > > of SiteEngagementScore. > > I'd rather not do that for two reasons: > - I'd have to pass clock_ and profile_ at every call which is a bit verbose for > no reason. > - It would allow const setters again. Blech. Acknowledged. https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:177: const history::OriginCountAndLastVisitMap& remaining_origin_counts); On 2016/05/26 08:26:20, calamity wrote: > On 2016/05/25 06:52:43, dominickn wrote: > > Can this method be const now? > > Not anymore ;) Acknowledged. https://codereview.chromium.org/1919383005/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1919383005/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:259: if (origin.is_valid()) { Strictly speaking this could be one if statement, but oh well.
On 2016/05/26 09:33:20, dominickn wrote: > lgtm % optional nit (combine if statement) > > https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_score.h (right): > > https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_score.h:144: explicit > SiteEngagementScore( > On 2016/05/26 08:26:20, calamity wrote: > > On 2016/05/25 06:52:43, dominickn wrote: > > > Nit: remove explicit and the unit tests comment > > > > The comment is still true. > > Acknowledged. > > https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_service.h (right): > > https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.h:102: void > ResetScoreForURL(const GURL& url, double score); > On 2016/05/26 08:26:20, calamity wrote: > > On 2016/05/25 06:52:43, dominickn wrote: > > > const > > > > So.... I don't want this to be const because it's bananas for setters to be > > const. So I added a const override for GetEngagementScore so that this is no > > longer const-able. > > Acknowledged. > > https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.h:135: SiteEngagementScore > GetEngagementScore(const GURL& origin) const; > On 2016/05/26 08:26:20, calamity wrote: > > On 2016/05/25 06:52:43, dominickn wrote: > > > This could be in the anonymous namespace in site_engagement_sevice.cc, > taking > > a > > > clock pointer and origin argument. Then you don't need the forward > declaration > > > of SiteEngagementScore. > > > > I'd rather not do that for two reasons: > > - I'd have to pass clock_ and profile_ at every call which is a bit verbose > for > > no reason. > > - It would allow const setters again. Blech. > > Acknowledged. > > https://codereview.chromium.org/1919383005/diff/160001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.h:177: const > history::OriginCountAndLastVisitMap& remaining_origin_counts); > On 2016/05/26 08:26:20, calamity wrote: > > On 2016/05/25 06:52:43, dominickn wrote: > > > Can this method be const now? > > > > Not anymore ;) > > Acknowledged. > > https://codereview.chromium.org/1919383005/diff/200001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_service.cc (right): > > https://codereview.chromium.org/1919383005/diff/200001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.cc:259: if (origin.is_valid()) > { > Strictly speaking this could be one if statement, but oh well. Ping: is this going to land?
https://codereview.chromium.org/1919383005/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1919383005/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:259: if (origin.is_valid()) { On 2016/05/26 09:33:20, dominickn wrote: > Strictly speaking this could be one if statement, but oh well. Done.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/1919383005/#ps220001 (title: "address nit + rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919383005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919383005/220001
Message was sent while issue was closed.
Description was changed from ========== Small cleanup of SiteEngagementService. This CL moves pref management into SiteEngagementScore so that SiteEngagementService has a cleaner flow for operating on SiteEngagementScores. BUG=604305 ========== to ========== Small cleanup of SiteEngagementService. This CL moves pref management into SiteEngagementScore so that SiteEngagementService has a cleaner flow for operating on SiteEngagementScores. BUG=604305 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Small cleanup of SiteEngagementService. This CL moves pref management into SiteEngagementScore so that SiteEngagementService has a cleaner flow for operating on SiteEngagementScores. BUG=604305 ========== to ========== Small cleanup of SiteEngagementService. This CL moves pref management into SiteEngagementScore so that SiteEngagementService has a cleaner flow for operating on SiteEngagementScores. BUG=604305 Committed: https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf Cr-Commit-Position: refs/heads/master@{#396789} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf Cr-Commit-Position: refs/heads/master@{#396789} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
