Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1844753002:
Histogram the scheme of an origin on the 1st navigation to it. (Closed)
|
Created:
4 years, 8 months ago by palmer Modified:
4 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, asvitkine+watch_chromium.org, ajwong+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHistogram the scheme of an origin on the 1st navigation to it.
In Off The Record and in 'normal' profiles, per session.
BUG=598899
Committed: https://crrev.com/9433e99e44d9f8ded2d9562188491f0b1e234102
Cr-Commit-Position: refs/heads/master@{#388030}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Respond to comments. #
Total comments: 9
Patch Set 3 : Respond to comments; correct MFS logic. #Patch Set 4 : Describe |HaveAlreadySeen|'s side-effect. #
Total comments: 9
Patch Set 5 : use MRUCache instead of std::set. #Patch Set 6 : Move the origin storage from ProfileIOData to Profile. (It's RAM-only.) #Patch Set 7 : Must #include "url/origin.h" in profile.h to use MRUCache. #Patch Set 8 : Use an OriginsSeenService[Factory]. #Patch Set 9 : Trim and move the Service[Factory]. #Patch Set 10 : Remove now-extraneous `git cl format`-ism. #
Total comments: 12
Patch Set 11 : Simplifications thanks to avi. #
Total comments: 12
Patch Set 12 : Get it working in Incognito. Thanks dcheng! #
Total comments: 4
Patch Set 13 : Remove commented-out line. #
Total comments: 6
Patch Set 14 : Respond to comments. #Messages
Total messages: 71 (20 generated)
palmer@chromium.org changed reviewers: + felt@chromium.org
felt: Please see if you think this is on the right track before we bug other reviewers. Thanks!
https://codereview.chromium.org/1844753002/diff/1/chrome/browser/tab_contents... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/1/chrome/browser/tab_contents... chrome/browser/tab_contents/navigation_metrics_recorder.cc:42: bool is_otr = web_contents()->GetBrowserContext()->IsOffTheRecord(); IsOffTheRecord only returns true for Incognito, right? the comment at its definition says so, but I remember pkasting suggesting otherwise (something to do with guest mode?). https://codereview.chromium.org/1844753002/diff/1/components/navigation_metri... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/1844753002/diff/1/components/navigation_metri... components/navigation_metrics/navigation_metrics.cc:67: UMA_HISTOGRAM_ENUMERATION(enumeration_mfs, scheme, SCHEME_MAX); Does this actually work? I thought the histogram macros require string literals or else they barfed, or has that been fixed? https://codereview.chromium.org/1844753002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1844753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23196: + Incognito session. It seems a bit misleading to call it MFS, when it is once per visit. Could you rename it? Also, would it make sense to have the similar policy in place for Non-Incognito for comparison? https://codereview.chromium.org/1844753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23200: +<histogram name="Navigation.IncognitoMainFrameSchemeDifferentPage" Why do we need both of these, if it's already limited to once per visit?
General approach seems OK, assuming privacy team blessing. For posterity: I asked Chris whether he could use tab stack history (meaning, the data structure that backs the "back" button) instead of recording a new set of origins. Chris looked into it and said that approach doesn't work because tab state is lost when the tab is closed. Since Chris wants to track per-session visits, that doesn't work. https://codereview.chromium.org/1844753002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1844753002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:864: || profile_type() == Profile::GUEST_PROFILE; Ah yes, here... this version includes both INCOGNITO and GUEST profiles in the definition of OTR.
New patchset coming up. https://codereview.chromium.org/1844753002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1844753002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:864: || profile_type() == Profile::GUEST_PROFILE; On 2016/04/01 00:04:50, felt wrote: > Ah yes, here... this version includes both INCOGNITO and GUEST profiles in the > definition of OTR. Acknowledged. https://codereview.chromium.org/1844753002/diff/1/chrome/browser/tab_contents... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/1/chrome/browser/tab_contents... chrome/browser/tab_contents/navigation_metrics_recorder.cc:42: bool is_otr = web_contents()->GetBrowserContext()->IsOffTheRecord(); > IsOffTheRecord only returns true for Incognito, right? the comment at its > definition says so, but I remember pkasting suggesting otherwise (something to > do with guest mode?). OTR is for Incognito and Guest Mode. Rather than try to replicate the internals of BrowserContext::IsOffTheRecord, I'll just record Navigation.SchemePerUniqueOrigin and Navigation.SchemePerUniqueOriginOTR, and use the "OTR" nomenclature consistently in this CL. Sound good? https://codereview.chromium.org/1844753002/diff/1/components/navigation_metri... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/1844753002/diff/1/components/navigation_metri... components/navigation_metrics/navigation_metrics.cc:67: UMA_HISTOGRAM_ENUMERATION(enumeration_mfs, scheme, SCHEME_MAX); On 2016/04/01 00:01:59, felt wrote: > Does this actually work? I thought the histogram macros require string literals > or else they barfed, or has that been fixed? It compiles and runs, but the comments about lazy initialization in base/metrics/histogram_macros.h have me wondering, now that you mention it. I've changed it to use literals. https://codereview.chromium.org/1844753002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1844753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23196: + Incognito session. > It seems a bit misleading to call it MFS, when it is once per visit. Could you > rename it? Also, would it make sense to have the similar policy in place for > Non-Incognito for comparison? Sure, will do. Proposed name: Navigation.SchemePerUniqueOrigin[OTR]. https://codereview.chromium.org/1844753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23200: +<histogram name="Navigation.IncognitoMainFrameSchemeDifferentPage" > Why do we need both of these, if it's already limited to once per visit? Hmm, yeah. I'll remove it.
Description was changed from ========== Histogram the scheme of an origin on the 1st navigation to it. In Incognito/Off The Record, per session. BUG=598899 ========== to ========== Histogram the scheme of an origin on the 1st navigation to it. In Off The Record and in 'normal' profiles, per session. BUG=598899 ==========
https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_io_data.h:17: #include "url/origin.h" still needed? https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:868: return origins_seen_.insert(origin).second == false; seems unexpected to me (based on method name & description) that this would also insert the origin in addition to returning true/false https://codereview.chromium.org/1844753002/diff/20001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/1844753002/diff/20001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:67: } else { nit: could you simplify this by returning early? https://codereview.chromium.org/1844753002/diff/20001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:72: UMA_HISTOGRAM_ENUMERATION("Navigation.MainFrameScheme", scheme, SCHEME_MAX); do we still want to calculate MFS like normal for OTR pages?
https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_io_data.h:17: #include "url/origin.h" > still needed? Nope! https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:868: return origins_seen_.insert(origin).second == false; > seems unexpected to me (based on method name & description) that this would also > insert the origin in addition to returning true/false What would you rather do? I want to let the caller not care about the details of keeping track of how we remember origins we've seen and not seen, and I want to let the caller not have to adhere to a multi-call protocol such as (hypothetical): if (!io_data->HaveAlreadySeen(origin)) { // Record UMA... io_data->NoteOriginSeen(origin); } https://codereview.chromium.org/1844753002/diff/20001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/1844753002/diff/20001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:67: } else { On 2016/04/07 02:28:04, felt wrote: > nit: could you simplify this by returning early? Simplified in a different way. https://codereview.chromium.org/1844753002/diff/20001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:72: UMA_HISTOGRAM_ENUMERATION("Navigation.MainFrameScheme", scheme, SCHEME_MAX); > do we still want to calculate MFS like normal for OTR pages? Oh, right. Yep!
https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1844753002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:868: return origins_seen_.insert(origin).second == false; On 2016/04/07 23:45:43, palmer wrote: > > seems unexpected to me (based on method name & description) that this would > also > > insert the origin in addition to returning true/false > > What would you rather do? > > I want to let the caller not care about the details of keeping track of how we > remember origins we've seen and not seen, and I want to let the caller not have > to adhere to a multi-call protocol such as (hypothetical): > > if (!io_data->HaveAlreadySeen(origin)) { > // Record UMA... > io_data->NoteOriginSeen(origin); > } I agree it makes sense to keep it a single method. I was suggesting naming it something like CalculateRepeatVisit, InsertAndLookupOrigin, or adjusting the comment to mention it isn't side effect free.
> I agree it makes sense to keep it a single method. I was suggesting naming it > something like CalculateRepeatVisit, InsertAndLookupOrigin, or adjusting the > comment to mention it isn't side effect free. I updated the comment (see chrome/browser/profiles/profile_io_data.h).
palmer@chromium.org changed reviewers: + avi@chromium.org, cbentzel@chromium.org, mmenke@chromium.org, mpearson@chromium.org
PTAL, thank you! mpearson: tools/metrics cbentzel: components/navigation_metrics avi: chrome/browser/tab_contents mmenke: chrome/browser/profiles
https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:609: std::set<url::Origin> origins_seen_; Not a huge fan of unbounded memory use - yes, it shouldn't be a huge amount, but it all adds up, particularly if we're going to keep this around. I'd be fine with an LRUCache, or something. Admittedly, that adds a bit of complexity. Open to other ideas. I assume that just tracking [non-back/forward] cross-process navigations isn't good enough for what you want. https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/tab_cont... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/tab_cont... chrome/browser/tab_contents/navigation_metrics_recorder.cc:45: ProfileIOData::FromResourceContext(context->GetResourceContext()); BUG: We're dereferencing an IOThread object on the UI thread.
https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:609: std::set<url::Origin> origins_seen_; On 2016/04/08 20:42:29, mmenke wrote: > Not a huge fan of unbounded memory use - yes, it shouldn't be a huge amount, but > it all adds up, particularly if we're going to keep this around. I'd be fine > with an LRUCache, or something. Admittedly, that adds a bit of complexity. > Open to other ideas. I assume that just tracking [non-back/forward] > cross-process navigations isn't good enough for what you want. Also wondering, is this really want you want? I'm curious if number of origins is really better than portion of navigations to HTTP pages, for your purposes.
histograms.xml lgtm
https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:609: std::set<url::Origin> origins_seen_; > Not a huge fan of unbounded memory use - yes, it shouldn't be a huge amount, but > it all adds up, particularly if we're going to keep this around. I'd be fine > with an LRUCache, or something. Admittedly, that adds a bit of complexity. > Open to other ideas. I feel that unbounded memory concern, but it's correct for our goals. An LRU cache might suffice if the cache size were set high enough; I'll defer to felt for a judgement on that. Seems to me like we should be able to pick a decent size. Do we have an LRU set class on hand? If not, I'd kind of enjoy writing one... > I assume that just tracking [non-back/forward] > cross-process navigations isn't good enough for what you want. Right. https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:609: std::set<url::Origin> origins_seen_; > Also wondering, is this really want you want? I'm curious if number of origins > is really better than portion of navigations to HTTP pages, for your purposes. Yes, it is. Our goal is to show a verbose security state (e.g. "foo.com NOT SECURE", "gmail.com SECURE") on a profile's first visit to the origin (potentially, first visit within time interval T), and on subsequent visits show non-verbose. The purpose of this CL is to estimate how often people would experience the verbosity, and to see if that measurement differs in OTR vs. normal. For portion of portion of navigations to HTTP pages, indeed, Navigation.MainFrameScheme[DifferentPage] already does suffice, and we use it for that. But, here we aim to measure something slightly different. https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/tab_cont... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/tab_cont... chrome/browser/tab_contents/navigation_metrics_recorder.cc:45: ProfileIOData::FromResourceContext(context->GetResourceContext()); > BUG: We're dereferencing an IOThread object on the UI thread. Hmm. The ways I can think of to solve this are: * Put the set<Origin> in Profile, rather than ProfileIOData (since it's not really I/O anyway) * Post an asynchronous task to the IO thread? * ? Are there other ways? I'm not sure what's best, and will take any advice.
https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:609: std::set<url::Origin> origins_seen_; On 2016/04/08 21:52:02, palmer wrote: > > Not a huge fan of unbounded memory use - yes, it shouldn't be a huge amount, > but > > it all adds up, particularly if we're going to keep this around. I'd be fine > > with an LRUCache, or something. Admittedly, that adds a bit of complexity. > > Open to other ideas. > > I feel that unbounded memory concern, but it's correct for our goals. An LRU > cache might suffice if the cache size were set high enough; I'll defer to felt > for a judgement on that. Seems to me like we should be able to pick a decent > size. Are we thinking the feature itself would work similarly, or are we just talking histograms? Also, if this histogram is going to only stay around for a couple versions of Chrome, I'm less concerned about the design, but if it's going to be a long term fixture, think we should spend some time thinking about it. > Do we have an LRU set class on hand? If not, I'd kind of enjoy writing one... I thought we did when I made that comment, I'm less sure now. > > I assume that just tracking [non-back/forward] > > cross-process navigations isn't good enough for what you want. > > Right. https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/tab_cont... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/tab_cont... chrome/browser/tab_contents/navigation_metrics_recorder.cc:45: ProfileIOData::FromResourceContext(context->GetResourceContext()); On 2016/04/08 21:52:02, palmer wrote: > > BUG: We're dereferencing an IOThread object on the UI thread. > > Hmm. The ways I can think of to solve this are: > > * Put the set<Origin> in Profile, rather than ProfileIOData (since it's not > really I/O anyway) > * Post an asynchronous task to the IO thread? > * ? > > Are there other ways? I'm not sure what's best, and will take any advice. I'll think about this a bit on Monday. Profile may be the way to go.
> > I feel that unbounded memory concern, but it's correct for our goals. An LRU > > cache might suffice if the cache size were set high enough; I'll defer to felt > > for a judgement on that. Seems to me like we should be able to pick a decent > > size. > > Are we thinking the feature itself would work similarly, or are we just talking > histograms? Also, if this histogram is going to only stay around for a couple > versions of Chrome, I'm less concerned about the design, but if it's going to be > a long term fixture, think we should spend some time thinking about it. I think it kind of depends on the outcome of the measurements we get from UMA. But, it is likely to be a long-term fixture, I think. > > Do we have an LRU set class on hand? If not, I'd kind of enjoy writing one... > > I thought we did when I made that comment, I'm less sure now. See https://codereview.chromium.org/1868353003/. We have 2, both of which are close but not quite what we need here. The new one could replace one of the old ones. (See comments in that CL.)
https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1844753002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:609: std::set<url::Origin> origins_seen_; On 2016/04/08 22:56:29, mmenke wrote: > On 2016/04/08 21:52:02, palmer wrote: > > > Not a huge fan of unbounded memory use - yes, it shouldn't be a huge amount, > > but > > > it all adds up, particularly if we're going to keep this around. I'd be > fine > > > with an LRUCache, or something. Admittedly, that adds a bit of complexity. > > > Open to other ideas. > > > > I feel that unbounded memory concern, but it's correct for our goals. An LRU > > cache might suffice if the cache size were set high enough; I'll defer to felt > > for a judgement on that. Seems to me like we should be able to pick a decent > > size. > > Are we thinking the feature itself would work similarly, or are we just talking > histograms? Also, if this histogram is going to only stay around for a couple > versions of Chrome, I'm less concerned about the design, but if it's going to be > a long term fixture, think we should spend some time thinking about it. > > > Do we have an LRU set class on hand? If not, I'd kind of enjoy writing one... > > I thought we did when I made that comment, I'm less sure now. > > > > I assume that just tracking [non-back/forward] > > > cross-process navigations isn't good enough for what you want. > > > > Right. > Would base/containers/mru_cache.h suffice (not a set, but...)
OK, here's a version that uses MRUCache, instead of set. Thoughts on the thread problem? Shall we move the cache to Profile, mmenke?
On 2016/04/12 00:10:20, palmer wrote: > OK, here's a version that uses MRUCache, instead of set. > > Thoughts on the thread problem? Shall we move the cache to Profile, mmenke? Sorry, forgot about that part - yea, I think Profile is the way to go, or at least a much better choice than ProfileIOData. Note that in profiles/, I only really own ProfileIOData, and related code - I wouldn't consider adding this to Profile to fall under my purview. Also, I don't believe this catches redirects...Do we care about redirects through HTTP domains? I guess not?
> Sorry, forgot about that part - yea, I think Profile is the way to go, or at > least a much better choice than ProfileIOData. Note that in profiles/, I only > really own ProfileIOData, and related code - I wouldn't consider adding this to > Profile to fall under my purview. OK, done. Thanks for all your help! new patchset coming up, and I'll find a reviewer for it. > Also, I don't believe this catches redirects...Do we care about redirects > through HTTP domains? I guess not? No, I don't think so; we only care about what we show/would have shown at the completion of navigation.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844753002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844753002/100001
palmer@chromium.org changed reviewers: + anthonyvd@chromium.org - mmenke@chromium.org
anthonyvd: PTAl as a profiles OWNER? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
anthonyvd@chromium.org changed reviewers: + mlerman@chromium.org
+mlerman I think the accepted wisdom is that we should avoid adding more stuff to Profile and instead favor things like PKSs. I'll defer to Mike, who has more profiles experience than I do, for this. Mike, wdyt?
On 2016/04/12 20:29:14, anthonyvd wrote: > +mlerman > > I think the accepted wisdom is that we should avoid adding more stuff to Profile > and instead favor things like PKSs. I'll defer to Mike, who has more profiles > experience than I do, for this. > > Mike, wdyt? I agree with Anthony, this isn't a piece of data for the Profile to store or manage.
> > I think the accepted wisdom is that we should avoid adding more stuff to > Profile > > and instead favor things like PKSs. I'll defer to Mike, who has more profiles > > experience than I do, for this. > > > > Mike, wdyt? > > I agree with Anthony, this isn't a piece of data for the Profile to store or > manage. OK, thanks. Pardon my ignorance, but, what does PKS stand for? I am finding things like components/keyed_service/core/refcounted_keyed_service.h and chrome/browser/apps/drive/drive_service_bridge.h but I'd like to make sure those are things like what you mean before I start hacking. Thanks!
On 2016/04/12 20:53:44, palmer wrote: > > > I think the accepted wisdom is that we should avoid adding more stuff to > > Profile > > > and instead favor things like PKSs. I'll defer to Mike, who has more > profiles > > > experience than I do, for this. > > > > > > Mike, wdyt? > > > > I agree with Anthony, this isn't a piece of data for the Profile to store or > > manage. > > OK, thanks. Pardon my ignorance, but, what does PKS stand for? I am finding > things like components/keyed_service/core/refcounted_keyed_service.h and > chrome/browser/apps/drive/drive_service_bridge.h but I'd like to make sure those > are things like what you mean before I start hacking. Thanks! He's referring to a ProfileKeyedService, which nowadays is a KeyedService with an associated BrowserContextKeyedServiceFactory. You'll probably want to copy the design of an existing one (chrome/browser/supervised_user/supervised_user_service.h); do we have docs explaining this?
> He's referring to a ProfileKeyedService, which nowadays is a KeyedService with > an associated BrowserContextKeyedServiceFactory. > > You'll probably want to copy the design of an existing one > (chrome/browser/supervised_user/supervised_user_service.h); do we have docs > explaining this? OK, I have now done that. However, an "is element in set" construction this complicated (Patch Set 8 is net +180 lines) gives me pause. Is this really the right approach?
On 2016/04/13 00:49:32, palmer wrote: > > He's referring to a ProfileKeyedService, which nowadays is a KeyedService with > > an associated BrowserContextKeyedServiceFactory. > > > > You'll probably want to copy the design of an existing one > > (chrome/browser/supervised_user/supervised_user_service.h); do we have docs > > explaining this? > > OK, I have now done that. > > However, an "is element in set" construction this complicated (Patch Set 8 is > net +180 lines) gives me pause. Is this really the right approach? Brief comments: 1) You don't need GetBrowserContextToUse() implemented. 2) This service shouldn't be implemented in chrome/browser/profiles. chrome/browser/tab_contents would probably be a better fit. 3) Your service doesn't need to keep a Profile*. Also overall, you might be able to get away with a map that lives in navigation_metrics_recorder, keyed off of either Profile* or Profile*->profile_path(). Less extensible, but lighter weight.
> Also overall, you might be able to get away with a map that lives in > navigation_metrics_recorder, keyed off of either Profile* or > Profile*->profile_path(). Less extensible, but lighter weight. That doesn't seem like it's going to work; a new NavigationMetricsRecorder seems to be created for each WebContents, so the map ends up not being a single one per profile. When I try to make the map global to the navigation_metrics_recorder compilation unit, I get error: declaration requires an exit-time destructor [-Werror,-Wexit-time-destructors] I think I'll just slim down and move the PKS.
palmer@chromium.org changed reviewers: - anthonyvd@chromium.org, mlerman@chromium.org
avi: PTAL as chrome/browser/tab_contents/OWNERS cbentzel: components/navigation_metrics/OWNERS Thanks, all!
You make this key from the Profile when you should key it off of the BrowserContext. You'll save a lot of casts if you do. Sorry, just realized that extensions/browser/event_router.h and extensions/browser/event_router_factory.h are better samples for you to have worked from.... :( https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/navigation_metrics_recorder.cc:48: OriginsSeenServiceFactory::GetForProfile(profile); You shouldn't need to pass in a Profile here, just a BrowserContext. See my comments below. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.cc (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.cc:5: #include "chrome/browser/profiles/profile.h" You do not need this include; remove it. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.h (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.h:12: class Profile; You don't need this forward declaration; remove it. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.h (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:12: class Profile; remove this because... https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:17: static OriginsSeenService* GetForProfile(Profile* profile); You should make this be: static OriginsSeenService* GetForBrowserContext(content::BrowserContext* context); https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:22: static KeyedService* BuildInstanceFor(Profile* profile); and static KeyedService* BuildInstanceFor(content::BrowserContext* context);
https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/navigation_metrics_recorder.cc:48: OriginsSeenServiceFactory::GetForProfile(profile); On 2016/04/14 02:43:26, Avi wrote: > You shouldn't need to pass in a Profile here, just a BrowserContext. See my > comments below. Done. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.cc (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.cc:5: #include "chrome/browser/profiles/profile.h" On 2016/04/14 02:43:26, Avi wrote: > You do not need this include; remove it. Done. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.h (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.h:12: class Profile; On 2016/04/14 02:43:26, Avi wrote: > You don't need this forward declaration; remove it. Done. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.h (right): https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:12: class Profile; On 2016/04/14 02:43:26, Avi wrote: > remove this because... Done. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:17: static OriginsSeenService* GetForProfile(Profile* profile); On 2016/04/14 02:43:26, Avi wrote: > You should make this be: > > static OriginsSeenService* GetForBrowserContext(content::BrowserContext* > context); Done. https://codereview.chromium.org/1844753002/diff/180001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:22: static KeyedService* BuildInstanceFor(Profile* profile); On 2016/04/14 02:43:26, Avi wrote: > and > > static KeyedService* BuildInstanceFor(content::BrowserContext* context); Done.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844753002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844753002/200001
Simplify it even more! https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/navigation_metrics_recorder.cc:10: #include "chrome/browser/profiles/profile.h" Do you still need this, as you don't use profiles here any more. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.cc (right): https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:7: #include "chrome/browser/profiles/incognito_helpers.h" Do you need this? https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:8: #include "chrome/browser/profiles/profile.h" You don't need this line... https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:37: content::BrowserContext* profile) const { |context|, not |profile|. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:38: return BuildInstanceFor(static_cast<Profile*>(profile)); ... because you don't need this cast, as BuildInstanceFor takes a BrowserContext. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.h (right): https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:32: content::BrowserContext* profile) const override; |context|, not |profile|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset coming up, but first I have to figure out why NavigationMetricsRecorder::DidNavigateMainFrame is getting NULL for OriginsSeenService* service = OriginsSeenServiceFactory::GetForBrowserContext(context); in Incognito mode (only; |context| is not NULL). Gotta head out to a meeting in a moment, but if anyone has any clues... https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right): https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/navigation_metrics_recorder.cc:10: #include "chrome/browser/profiles/profile.h" On 2016/04/14 19:08:02, Avi wrote: > Do you still need this, as you don't use profiles here any more. Done. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.cc (right): https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:7: #include "chrome/browser/profiles/incognito_helpers.h" On 2016/04/14 19:08:02, Avi wrote: > Do you need this? Done. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:8: #include "chrome/browser/profiles/profile.h" On 2016/04/14 19:08:02, Avi wrote: > You don't need this line... Done. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:37: content::BrowserContext* profile) const { On 2016/04/14 19:08:02, Avi wrote: > |context|, not |profile|. Done. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:38: return BuildInstanceFor(static_cast<Profile*>(profile)); On 2016/04/14 19:08:02, Avi wrote: > ... because you don't need this cast, as BuildInstanceFor takes a > BrowserContext. Done. https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.h (right): https://codereview.chromium.org/1844753002/diff/200001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.h:32: content::BrowserContext* profile) const override; On 2016/04/14 19:08:02, Avi wrote: > |context|, not |profile|. Done.
Yeah, OriginsSeenServiceFactory::BuildServiceInstanceFor doesn't get called in Incognito mode. It's not yet obvious to me why...
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844753002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844753002/220001
LGTM with fix. https://codereview.chromium.org/1844753002/diff/220001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.cc (right): https://codereview.chromium.org/1844753002/diff/220001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:37: // return chrome::GetBrowserContextRedirectedInIncognito(context); Remove this commented line. https://codereview.chromium.org/1844753002/diff/220001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:38: return chrome::GetBrowserContextOwnInstanceInIncognito(context); Yep, this is what you want. Given that you have an OTR metric, you want to keep stats for OTR on a separate service.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1844753002/diff/220001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service_factory.cc (right): https://codereview.chromium.org/1844753002/diff/220001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:37: // return chrome::GetBrowserContextRedirectedInIncognito(context); On 2016/04/15 00:47:03, Avi wrote: > Remove this commented line. Done. https://codereview.chromium.org/1844753002/diff/220001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service_factory.cc:38: return chrome::GetBrowserContextOwnInstanceInIncognito(context); On 2016/04/15 00:47:03, Avi wrote: > Yep, this is what you want. Given that you have an OTR metric, you want to keep > stats for OTR on a separate service. Acknowledged.
components/navigation_metrics LGTM
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1844753002/#ps240001 (title: "Remove commented-out line.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844753002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844753002/240001
The CQ bit was unchecked by palmer@chromium.org
palmer@chromium.org changed reviewers: + battre@chromium.org
battre: Can you or another privacy team person take a look? Thanks!
lgtm https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.cc (right): https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.cc:7: #include <iostream> delete this line? https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.h (right): https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.h:21: bool HaveAlreadySeenOrigin(const url::Origin& origin); Nit (up to you): The function name suggests that this is a getter and the comment explains the opposite. How about bool InsertSeenOrigin(const url::Origin& origin); or just bool Insert(const url::Origin& origin); The call could be still quite readable: bool already_seen = origins_seen_service->Insert(original); https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.h:25: base::MRUCache<url::Origin, bool> origins_seen_; Can you add a comment that this is intentionally in volatile storage because the OriginsSeenService is also instantiated for OTR profiles.
Thank you! https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.cc (right): https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.cc:7: #include <iostream> On 2016/04/18 15:20:51, battre wrote: > delete this line? Done. https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... File chrome/browser/tab_contents/origins_seen_service.h (right): https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.h:21: bool HaveAlreadySeenOrigin(const url::Origin& origin); On 2016/04/18 15:20:51, battre wrote: > Nit (up to you): The function name suggests that this is a getter and the > comment explains the opposite. How about > > bool InsertSeenOrigin(const url::Origin& origin); > > or just > > bool Insert(const url::Origin& origin); > > The call could be still quite readable: > > bool already_seen = origins_seen_service->Insert(original); Done. https://codereview.chromium.org/1844753002/diff/240001/chrome/browser/tab_con... chrome/browser/tab_contents/origins_seen_service.h:25: base::MRUCache<url::Origin, bool> origins_seen_; On 2016/04/18 15:20:51, battre wrote: > Can you add a comment that this is intentionally in volatile storage because the > OriginsSeenService is also instantiated for OTR profiles. Done.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbentzel@chromium.org, avi@chromium.org, mpearson@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/1844753002/#ps260001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844753002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844753002/260001
Message was sent while issue was closed.
Description was changed from ========== Histogram the scheme of an origin on the 1st navigation to it. In Off The Record and in 'normal' profiles, per session. BUG=598899 ========== to ========== Histogram the scheme of an origin on the 1st navigation to it. In Off The Record and in 'normal' profiles, per session. BUG=598899 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Histogram the scheme of an origin on the 1st navigation to it. In Off The Record and in 'normal' profiles, per session. BUG=598899 ========== to ========== Histogram the scheme of an origin on the 1st navigation to it. In Off The Record and in 'normal' profiles, per session. BUG=598899 Committed: https://crrev.com/9433e99e44d9f8ded2d9562188491f0b1e234102 Cr-Commit-Position: refs/heads/master@{#388030} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9433e99e44d9f8ded2d9562188491f0b1e234102 Cr-Commit-Position: refs/heads/master@{#388030}
Message was sent while issue was closed.
On 2016/04/18 at 21:22:19, commit-bot wrote: > Patchset 14 (id:??) landed as https://crrev.com/9433e99e44d9f8ded2d9562188491f0b1e234102 > Cr-Commit-Position: refs/heads/master@{#388030} It looks like iOS downstream code calls RecordMainFrameNavigation. It looks like the OriginsSeenService does not depends on anything from //chrome and should be easily shareable with iOS. Do you have any plan to componentize this code?
Message was sent while issue was closed.
> It looks like iOS downstream code calls RecordMainFrameNavigation. > > It looks like the OriginsSeenService does not depends on anything from //chrome > and should be easily shareable with iOS. Do you have any plan to componentize > this code? I hadn't considered it. I think it would be OK, for the purposes of our experiment, if we #ifdef'd it out for iOS. We could componentize too, I suppose. |
