Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2952343004:
Adding previews information to PLM UKM (Closed)
DescriptionAdding previews information to PLM UKM
This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview;
"server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is
shown, no metrics are added to the UKM.
Similarly, when the user does not opt out, nothing is added for opt_out.
This does not track offline previews yet, but may in the future.
The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page.
BUG=701514, 723711, 728707
Review-Url: https://codereview.chromium.org/2952343004
Cr-Commit-Position: refs/heads/master@{#487238}
Committed: https://chromium.googlesource.com/chromium/src/+/1806f8e181472bb2fb3d32ce575c4b2ec8a36bf2
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase test failures #Patch Set 4 : stop observing on background #
Total comments: 15
Patch Set 5 : change over tests + adding booleans instead of bitmask #
Total comments: 2
Patch Set 6 : ukm.xml #Patch Set 7 : comment on BroadcastEventToObservers #
Total comments: 19
Patch Set 8 : rkaplow + tbansal comments #Patch Set 9 : rebase #Messages
Total messages: 67 (44 generated)
The CQ bit was checked by ryansturm@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 ryansturm@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: linux_chromium_chromeos_ozone_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 ryansturm@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 ryansturm@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.
ryansturm@chromium.org changed reviewers: + bmcquade@chromium.org, rkaplow@chromium.org
bmcquade: PTAL at PLM related code rkaplow: PTAL at UKM usage
just a first pass - can you also update the ukm.xml. Bryan - do we think this is covered enough under the original Pageload privacy review or should these additional metrics be reviewed separately? https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.h (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer.h:21: enum PreviewsUKMValues { it's not clear to me why you want to encode this in a bitmask. I would think it would be better to encode as seperate boolean metrics https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:73: void RecordUKMOptOut(); I might rename these method - it sounds like it is related to opting out of UKM
Thanks! High level looks good - just a few questions/suggestions about the source id lookup APIs. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc:82: ukm::UkmRecorder* ukm_recorder = g_browser_process->ukm_recorder(); i think you may need to check that ukm_recorder is non-null here, though rkaplow can say for sure. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:102: metrics_observer->GetUKMSourceIdForNavigationHandle(navigation_handle); one risk with this approach is that PLM explicitly stops tracking some loads at commit, and you won't retain that behavior in your code. For example, we stop tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering logic but it'd be better to not have to do that. This is an interesting/challenging case, because as you note the previews decision is made during DidFinishNavigation, and the ordering of invocations between MWCO and the previews observer appears to be not well defined. Are we sure ordering is not well defined? Is it the case that tab helper construction order guarantees WCO callback order? If that's the case, it may be sensible to rely on the ordering and always only use GetUKMSourceForCommittedLoad. We could add some tests to make sure this ordering is guaranteed, either at the WCO level or just in our specific observer implementations.
Just answering comments -- will address other issues later. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.h (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer.h:21: enum PreviewsUKMValues { On 2017/06/27 22:17:09, rkaplow wrote: > it's not clear to me why you want to encode this in a bitmask. I would think it > would be better to encode as seperate boolean metrics That approach is what I initially proposed, since it seems simpler. There was pushback on the size of the data for k previews types, where k keeps growing, but you are more of an authority on this than anyone who wanted the bitmask. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:102: metrics_observer->GetUKMSourceIdForNavigationHandle(navigation_handle); On 2017/06/28 13:13:40, Bryan McQuade wrote: > one risk with this approach is that PLM explicitly stops tracking some loads at > commit, and you won't retain that behavior in your code. For example, we stop > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering logic > but it'd be better to not have to do that. > I think that is fine as committed_load_ will be empty, so source_id_ will be empty after this in that case. > This is an interesting/challenging case, because as you note the previews > decision is made during DidFinishNavigation, and the ordering of invocations > between MWCO and the previews observer appears to be not well defined. Are we > sure ordering is not well defined? Is it the case that tab helper construction > order guarantees WCO callback order? If that's the case, it may be sensible to > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We could > add some tests to make sure this ordering is guaranteed, either at the WCO level > or just in our specific observer implementations. The way that they are ordered is based on compiler generated addresses . See https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... This approach as far as I understand it, has the possibility to create different ordering on different compilers for the same code, so it's not something we can rely on I believe. In general, I think the approach goes like this: previews then metrics: It will pull it out of pending_loads_ using NavigationHandle. The only cases where pending_loads_ won't have the navigation are when subframes and ShouldTrackNavigation returns false. Since PLM supports all previews types in general, source_id_ will be valid for any previews navigations. metrics then previews: pending_loads_ won't have the navigation_handle, so it will use committed_load_, and if committed_load_ is non-null, then the source_id_ will be for this navigation_handle_. In general, I think the first case is hairier, because the fallback is somewhat frightening, as it will attribute it to the previous URL. That being said: a) I could store committed_navigation_handle in MWCO and reduce this to GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are reasonable assurances, with the risk of having a dangling reference to a deleted object -- this could be addressed by casting to void* or some other method. b) Get the GlobalRequestID in this class's ReadyToCommit method store it as committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO instead, but I think that carries risk for transfer navigations, as I don't fully understand them. c) csharrison@ was opposed to this, but Posting a task here to this tab helper to ask MWCO the currently committed SourceId after both DidFinishNavigations run.
Thanks! A couple thoughts. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.h (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer.h:21: enum PreviewsUKMValues { On 2017/06/28 at 16:19:35, RyanSturm wrote: > On 2017/06/27 22:17:09, rkaplow wrote: > > it's not clear to me why you want to encode this in a bitmask. I would think it > > would be better to encode as seperate boolean metrics > > That approach is what I initially proposed, since it seems simpler. There was pushback on the size of the data for k previews types, where k keeps growing, but you are more of an authority on this than anyone who wanted the bitmask. Yes, I'm totally supportive of switching to separate booleans if that's recommended. Apologies for my incorrect interpretation here! https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:102: metrics_observer->GetUKMSourceIdForNavigationHandle(navigation_handle); On 2017/06/28 at 16:19:35, RyanSturm wrote: > On 2017/06/28 13:13:40, Bryan McQuade wrote: > > one risk with this approach is that PLM explicitly stops tracking some loads at > > commit, and you won't retain that behavior in your code. For example, we stop > > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering logic > > but it'd be better to not have to do that. > > > > I think that is fine as committed_load_ will be empty, so source_id_ will be empty after this in that case. > > > This is an interesting/challenging case, because as you note the previews > > decision is made during DidFinishNavigation, and the ordering of invocations > > between MWCO and the previews observer appears to be not well defined. Are we > > sure ordering is not well defined? Is it the case that tab helper construction > > order guarantees WCO callback order? If that's the case, it may be sensible to > > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We could > > add some tests to make sure this ordering is guaranteed, either at the WCO level > > or just in our specific observer implementations. > > The way that they are ordered is based on compiler generated addresses . See https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... > > This approach as far as I understand it, has the possibility to create different ordering on different compilers for the same code, so it's not something we can rely on I believe. > > > In general, I think the approach goes like this: > > previews then metrics: > It will pull it out of pending_loads_ using NavigationHandle. The only cases where pending_loads_ won't have the navigation are when subframes and ShouldTrackNavigation returns false. Since PLM supports all previews types in general, source_id_ will be valid for any previews navigations. > > metrics then previews: > pending_loads_ won't have the navigation_handle, so it will use committed_load_, and if committed_load_ is non-null, then the source_id_ will be for this navigation_handle_. > > In general, I think the first case is hairier, because the fallback is somewhat frightening, as it will attribute it to the previous URL. > > That being said: > a) I could store committed_navigation_handle in MWCO and reduce this to GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are reasonable assurances, with the risk of having a dangling reference to a deleted object -- this could be addressed by casting to void* or some other method. > > b) Get the GlobalRequestID in this class's ReadyToCommit method store it as committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO instead, but I think that carries risk for transfer navigations, as I don't fully understand them. > > c) csharrison@ was opposed to this, but Posting a task here to this tab helper to ask MWCO the currently committed SourceId after both DidFinishNavigations run. Thanks! It sounds like, given the order can vary depending on compiler, we need to safely handle both orderings as you've done. I'm a little bit concerned about allowing external code to get the source id of uncommitted page loads given that we might filter them out, but I think it's the most straightforward way to address this issue in the near term. We could potentially allow other components in chrome to observe commits at the time they happen in MWCO. We have something like this for tests already: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics... - I would not want to provide the actual PageLoadTracker to non-test code but we could provide perhaps a PageLoadExtraInfo which would contain the source id. That said, this would make your consumer code more complex since either order is possible. We could define a new object that only 'activates' once both commits have been observed - this would address the ordering issues nicely but is more complex and it's also not always clear that both PLM and previews activate on the same set of page loads. So in the near term can we add a comment on the new GetUKMSourceIdForNavigationHandle and GetUKMSourceForCommittedLoad noting that these are not intended for wide use yet, and consumers should ping code owners before using these APIs? If we get lots of pings asking to call these it suggests we should probably come up with a cleaner way to solve this class of problem; if this new code is the only code that uses it then we may be ok to leave as is. How does this sound? RE: retaining committed_navigation_handle, once an object is free, we're also free to re-use the same address for some other object, so this committed_navigation_handle could collide with a newly allocated navigation handle. So I think we want to avoid using pointers as identifiers after the memory being pointed to has been freed.
https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:102: metrics_observer->GetUKMSourceIdForNavigationHandle(navigation_handle); On 2017/06/28 16:48:18, Bryan McQuade wrote: > On 2017/06/28 at 16:19:35, RyanSturm wrote: > > On 2017/06/28 13:13:40, Bryan McQuade wrote: > > > one risk with this approach is that PLM explicitly stops tracking some loads > at > > > commit, and you won't retain that behavior in your code. For example, we > stop > > > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering > logic > > > but it'd be better to not have to do that. > > > > > > > I think that is fine as committed_load_ will be empty, so source_id_ will be > empty after this in that case. > > > > > This is an interesting/challenging case, because as you note the previews > > > decision is made during DidFinishNavigation, and the ordering of invocations > > > between MWCO and the previews observer appears to be not well defined. Are > we > > > sure ordering is not well defined? Is it the case that tab helper > construction > > > order guarantees WCO callback order? If that's the case, it may be sensible > to > > > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We > could > > > add some tests to make sure this ordering is guaranteed, either at the WCO > level > > > or just in our specific observer implementations. > > > > The way that they are ordered is based on compiler generated addresses . See > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... > > > > > This approach as far as I understand it, has the possibility to create > different ordering on different compilers for the same code, so it's not > something we can rely on I believe. > > > > > > In general, I think the approach goes like this: > > > > previews then metrics: > > It will pull it out of pending_loads_ using NavigationHandle. The only cases > where pending_loads_ won't have the navigation are when subframes and > ShouldTrackNavigation returns false. Since PLM supports all previews types in > general, source_id_ will be valid for any previews navigations. > > > > metrics then previews: > > pending_loads_ won't have the navigation_handle, so it will use > committed_load_, and if committed_load_ is non-null, then the source_id_ will be > for this navigation_handle_. > > > > In general, I think the first case is hairier, because the fallback is > somewhat frightening, as it will attribute it to the previous URL. > > > > That being said: > > a) I could store committed_navigation_handle in MWCO and reduce this to > GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are > reasonable assurances, with the risk of having a dangling reference to a deleted > object -- this could be addressed by casting to void* or some other method. > > > > b) Get the GlobalRequestID in this class's ReadyToCommit method store it as > committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO > instead, but I think that carries risk for transfer navigations, as I don't > fully understand them. > > > > c) csharrison@ was opposed to this, but Posting a task here to this tab helper > to ask MWCO the currently committed SourceId after both DidFinishNavigations > run. > > Thanks! It sounds like, given the order can vary depending on compiler, we need > to safely handle both orderings as you've done. > > I'm a little bit concerned about allowing external code to get the source id of > uncommitted page loads given that we might filter them out, but I think it's the > most straightforward way to address this issue in the near term. > > We could potentially allow other components in chrome to observe commits at the > time they happen in MWCO. We have something like this for tests already: > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics... > - I would not want to provide the actual PageLoadTracker to non-test code but we > could provide perhaps a PageLoadExtraInfo which would contain the source id. > > That said, this would make your consumer code more complex since either order is > possible. We could define a new object that only 'activates' once both commits > have been observed - this would address the ordering issues nicely but is more > complex and it's also not always clear that both PLM and previews activate on > the same set of page loads. > > So in the near term can we add a comment on the new > GetUKMSourceIdForNavigationHandle and GetUKMSourceForCommittedLoad noting that > these are not intended for wide use yet, and consumers should ping code owners > before using these APIs? If we get lots of pings asking to call these it > suggests we should probably come up with a cleaner way to solve this class of > problem; if this new code is the only code that uses it then we may be ok to > leave as is. > > How does this sound? > > RE: retaining committed_navigation_handle, once an object is free, we're also > free to re-use the same address for some other object, so this > committed_navigation_handle could collide with a newly allocated navigation > handle. So I think we want to avoid using pointers as identifiers after the > memory being pointed to has been freed. I'll go ahead and add the comments as suggested because that approach SGTM. Agreed about the dangling reference for a lot of reasons.
https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc:98: size_t ukm_source_count() { return test_ukm_recorder_.sources_count(); } I have a change in flight: https://chromium-review.googlesource.com/c/558945/ which generalizes our PLM UKM test infra. I'll try to land this later this week. LMK if any test functionality you need is missing. Thanks!
On 2017/07/03 at 16:56:52, Bryan McQuade wrote: > https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc (right): > > https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc:98: size_t ukm_source_count() { return test_ukm_recorder_.sources_count(); } > I have a change in flight: https://chromium-review.googlesource.com/c/558945/ which generalizes our PLM UKM test infra. I'll try to land this later this week. LMK if any test functionality you need is missing. Thanks! The UkmTester class just landed so you should be able to use it now. I think it makes UKM tests a bit cleaner & shares testing code across observer tests. Can you see if this test utility allows you to test what you need to test? Feel free to add to it if anything is missing.
The CQ bit was checked by ryansturm@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 ryansturm@chromium.org to run a CQ dry run
Patchset #5 (id:80001) 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...
Some major changes since the last version. bmcquade,rkaplow: PTAL https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc:82: ukm::UkmRecorder* ukm_recorder = g_browser_process->ukm_recorder(); On 2017/06/28 13:13:40, Bryan McQuade wrote: > i think you may need to check that ukm_recorder is non-null here, though rkaplow > can say for sure. Done. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.h (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer.h:21: enum PreviewsUKMValues { On 2017/06/28 16:48:18, Bryan McQuade wrote: > On 2017/06/28 at 16:19:35, RyanSturm wrote: > > On 2017/06/27 22:17:09, rkaplow wrote: > > > it's not clear to me why you want to encode this in a bitmask. I would think > it > > > would be better to encode as seperate boolean metrics > > > > That approach is what I initially proposed, since it seems simpler. There was > pushback on the size of the data for k previews types, where k keeps growing, > but you are more of an authority on this than anyone who wanted the bitmask. > > Yes, I'm totally supportive of switching to separate booleans if that's > recommended. Apologies for my incorrect interpretation here! Done. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc:98: size_t ukm_source_count() { return test_ukm_recorder_.sources_count(); } On 2017/07/03 16:56:52, Bryan McQuade wrote: > I have a change in flight: https://chromium-review.googlesource.com/c/558945/ > which generalizes our PLM UKM test infra. I'll try to land this later this week. > LMK if any test functionality you need is missing. Thanks! Done. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:73: void RecordUKMOptOut(); On 2017/06/27 22:17:09, rkaplow wrote: > I might rename these method - it sounds like it is related to opting out of UKM Done. https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://codereview.chromium.org/2952343004/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:102: metrics_observer->GetUKMSourceIdForNavigationHandle(navigation_handle); On 2017/06/28 16:56:56, RyanSturm wrote: > On 2017/06/28 16:48:18, Bryan McQuade wrote: > > On 2017/06/28 at 16:19:35, RyanSturm wrote: > > > On 2017/06/28 13:13:40, Bryan McQuade wrote: > > > > one risk with this approach is that PLM explicitly stops tracking some > loads > > at > > > > commit, and you won't retain that behavior in your code. For example, we > > stop > > > > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering > > logic > > > > but it'd be better to not have to do that. > > > > > > > > > > I think that is fine as committed_load_ will be empty, so source_id_ will be > > empty after this in that case. > > > > > > > This is an interesting/challenging case, because as you note the previews > > > > decision is made during DidFinishNavigation, and the ordering of > invocations > > > > between MWCO and the previews observer appears to be not well defined. Are > > we > > > > sure ordering is not well defined? Is it the case that tab helper > > construction > > > > order guarantees WCO callback order? If that's the case, it may be > sensible > > to > > > > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We > > could > > > > add some tests to make sure this ordering is guaranteed, either at the WCO > > level > > > > or just in our specific observer implementations. > > > > > > The way that they are ordered is based on compiler generated addresses . See > > > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... > > > > > > > > This approach as far as I understand it, has the possibility to create > > different ordering on different compilers for the same code, so it's not > > something we can rely on I believe. > > > > > > > > > In general, I think the approach goes like this: > > > > > > previews then metrics: > > > It will pull it out of pending_loads_ using NavigationHandle. The only cases > > where pending_loads_ won't have the navigation are when subframes and > > ShouldTrackNavigation returns false. Since PLM supports all previews types in > > general, source_id_ will be valid for any previews navigations. > > > > > > metrics then previews: > > > pending_loads_ won't have the navigation_handle, so it will use > > committed_load_, and if committed_load_ is non-null, then the source_id_ will > be > > for this navigation_handle_. > > > > > > In general, I think the first case is hairier, because the fallback is > > somewhat frightening, as it will attribute it to the previous URL. > > > > > > That being said: > > > a) I could store committed_navigation_handle in MWCO and reduce this to > > GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are > > reasonable assurances, with the risk of having a dangling reference to a > deleted > > object -- this could be addressed by casting to void* or some other method. > > > > > > b) Get the GlobalRequestID in this class's ReadyToCommit method store it as > > committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO > > instead, but I think that carries risk for transfer navigations, as I don't > > fully understand them. > > > > > > c) csharrison@ was opposed to this, but Posting a task here to this tab > helper > > to ask MWCO the currently committed SourceId after both DidFinishNavigations > > run. > > > > Thanks! It sounds like, given the order can vary depending on compiler, we > need > > to safely handle both orderings as you've done. > > > > I'm a little bit concerned about allowing external code to get the source id > of > > uncommitted page loads given that we might filter them out, but I think it's > the > > most straightforward way to address this issue in the near term. > > > > We could potentially allow other components in chrome to observe commits at > the > > time they happen in MWCO. We have something like this for tests already: > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics... > > - I would not want to provide the actual PageLoadTracker to non-test code but > we > > could provide perhaps a PageLoadExtraInfo which would contain the source id. > > > > That said, this would make your consumer code more complex since either order > is > > possible. We could define a new object that only 'activates' once both commits > > have been observed - this would address the ordering issues nicely but is more > > complex and it's also not always clear that both PLM and previews activate on > > the same set of page loads. > > > > So in the near term can we add a comment on the new > > GetUKMSourceIdForNavigationHandle and GetUKMSourceForCommittedLoad noting that > > these are not intended for wide use yet, and consumers should ping code owners > > before using these APIs? If we get lots of pings asking to call these it > > suggests we should probably come up with a cleaner way to solve this class of > > problem; if this new code is the only code that uses it then we may be ok to > > leave as is. > > > > How does this sound? > > > > RE: retaining committed_navigation_handle, once an object is free, we're also > > free to re-use the same address for some other object, so this > > committed_navigation_handle could collide with a newly allocated navigation > > handle. So I think we want to avoid using pointers as identifiers after the > > memory being pointed to has been freed. > > I'll go ahead and add the comments as suggested because that approach SGTM. > > Agreed about the dangling reference for a lot of reasons. Done.
Description was changed from ========== Adding previews information to PLM UKM This adds two fields to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "previews_type" set to a bitmask of previews types when a preview was shown. When no preview is shown, a previews_type is not added, and is assumed to be not a preview. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The functionality uses the source_id from PageLoadTracker to write to the correct PLM UKM source. The infobar code sources this information at DidFinishNavigation time by asking PLM for a pending load with matching NavigationHandle and if none exists, it assumes that PLM DidFinishNavigation code already ran and uses the current load. Because both WCOs assign the current load during DidFinishNavigation of main frames, this functionality gets the correct source for each page load. BUG=701514,723711 ========== to ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711 ==========
Description was changed from ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711 ========== to ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bmcquade, rkaplow: ping (I'm guessing you are looking at gerrit and not this, let me know if you want me to move this to gerrit).
On 2017/07/12 18:38:48, RyanSturm wrote: > bmcquade, rkaplow: ping (I'm guessing you are looking at gerrit and not this, > let me know if you want me to move this to gerrit). mostly looks good on UKM usage, i think you missed my comment about needing to update the xml though. Also, I may have missed but have you filled out the privacy review? The template for adding new UKMs is on go/ukm-logs. Re; bitmask - I think we can start this way since it makes analysis much simpler. If we're concerned about the bandwidth use I'd rather do more built-in UKM stuff then everyone having to make custom bitmasks... I'd just rather avoid premature optimizations when they have known downsides.
LGTM, thanks for working through this one! I will take an AI to add support for looking up observer by type. I like your event broadcast solution in the meantime. Thanks! https://codereview.chromium.org/2952343004/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2952343004/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:144: void BroadcastEventToObservers(const void* const event_key); let's add to the comment that this API is subject to change and may be removed in the future - my hope is to get rid of this in favor for looking up observer by type, but that'll take a bit more time.
The CQ bit was checked by ryansturm@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 ryansturm@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...
Added ukm.xml change. rkaplow: PTAL, thanks https://codereview.chromium.org/2952343004/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2952343004/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.h:144: void BroadcastEventToObservers(const void* const event_key); On 2017/07/12 21:10:03, Bryan McQuade wrote: > let's add to the comment that this API is subject to change and may be removed > in the future - my hope is to get rid of this in favor for looking up observer > by type, but that'll take a bit more time. Done.
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL @ previews*
https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc:33: PreviewsUKMObserver::PreviewsUKMObserver() {} Is it useful to add sequence checker on this class? It is not obvious that opt-out broadcast events are received on the same sequence as the other notification events. https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc:112: client_lofi_seen_ = true; Is this |seen| or |requested|? https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.cc:257: NotifyPLMOfOptOut(web_contents); Does this class live on UI thread or IO thread? https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:258: bool opt_out_called_ = false; nit: It's somewhat confusing to have both ctor initialization and this in-place initialization in the same class. Is it possible to just use one or the other? https://codereview.chromium.org/2952343004/diff/140001/services/metrics/publi... File services/metrics/public/cpp/ukm_recorder.h (right): https://codereview.chromium.org/2952343004/diff/140001/services/metrics/publi... services/metrics/public/cpp/ukm_recorder.h:135: #endif // COMPONENTS_UKM_PUBLIC_UKM_RECORDER_H_ why was this changed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm lg % tbansal comments https://codereview.chromium.org/2952343004/diff/140001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2952343004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:756: Previews related metrics associated with a page load. can you point to what are Previews (maybe just a code pointer, or doc pointer) https://codereview.chromium.org/2952343004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:759: <summary> I would say '1' and not true
dougarnett@chromium.org changed reviewers: + dougarnett@chromium.org
https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.cc:121: PreviewsInfoBarDelegate::OptOutEventKey()); Should we be able to correlate this event somehow with the type of preview that was presented (and the user didn't like)?
Description was changed from ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711 ========== to ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711,728707 ==========
The CQ bit was checked by ryansturm@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 ryansturm@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...
tbansal: PTAL, thanks https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc:33: PreviewsUKMObserver::PreviewsUKMObserver() {} On 2017/07/12 21:58:45, tbansal1 wrote: > Is it useful to add sequence checker on this class? It is not obvious that > opt-out broadcast events are received on the same sequence as the other > notification events. Done. https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_ukm_observer.cc:112: client_lofi_seen_ = true; On 2017/07/12 21:58:45, tbansal1 wrote: > Is this |seen| or |requested|? They are synonymous in that adding the range header means the image could or could not be different regardless of what headers the server returns. https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/previews_ukm_observer_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/07/12 21:58:45, tbansal1 wrote: > 2017 Done. https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.cc:121: PreviewsInfoBarDelegate::OptOutEventKey()); On 2017/07/13 16:02:18, dougarnett wrote: > Should we be able to correlate this event somehow with the type of preview that > was presented (and the user didn't like)? That detection is done in the observer itself. Since this only pushes a notification for opt outs (and not for non-opt outs), using this code path to push type isn't helpful. https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.cc:257: NotifyPLMOfOptOut(web_contents); On 2017/07/12 21:58:45, tbansal1 wrote: > Does this class live on UI thread or IO thread? It's all UI thread. I renamed this to "InformPLMOfOptOut" https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:258: bool opt_out_called_ = false; On 2017/07/12 21:58:46, tbansal1 wrote: > nit: It's somewhat confusing to have both ctor initialization and this in-place > initialization in the same class. Is it possible to just use one or the other? Done. https://codereview.chromium.org/2952343004/diff/140001/services/metrics/publi... File services/metrics/public/cpp/ukm_recorder.h (right): https://codereview.chromium.org/2952343004/diff/140001/services/metrics/publi... services/metrics/public/cpp/ukm_recorder.h:135: #endif // COMPONENTS_UKM_PUBLIC_UKM_RECORDER_H_ On 2017/07/12 21:58:46, tbansal1 wrote: > why was this changed? Good catch, bad rebase. Done. https://codereview.chromium.org/2952343004/diff/140001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2952343004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:756: Previews related metrics associated with a page load. On 2017/07/13 15:43:31, rkaplow wrote: > can you point to what are Previews (maybe just a code pointer, or doc pointer) Done. https://codereview.chromium.org/2952343004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:759: <summary> On 2017/07/13 15:43:32, rkaplow wrote: > I would say '1' and not true Done.
lgtm
The CQ bit was unchecked by ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2952343004/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.cc:121: PreviewsInfoBarDelegate::OptOutEventKey()); On 2017/07/17 18:48:30, RyanSturm wrote: > On 2017/07/13 16:02:18, dougarnett wrote: > > Should we be able to correlate this event somehow with the type of preview > that > > was presented (and the user didn't like)? > > That detection is done in the observer itself. Since this only pushes a > notification for opt outs (and not for non-opt outs), using this code path to > push type isn't helpful. I'm asking a question not (yet) asking for a change. Do you expect we will be able to correlate this event to the type of preview server-side? Eg, is it grouped with page view group of metrics that would allow for that?
On 2017/07/17 20:41:21, dougarnett wrote: > https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... > File chrome/browser/previews/previews_infobar_delegate.cc (right): > > https://codereview.chromium.org/2952343004/diff/140001/chrome/browser/preview... > chrome/browser/previews/previews_infobar_delegate.cc:121: > PreviewsInfoBarDelegate::OptOutEventKey()); > On 2017/07/17 18:48:30, RyanSturm wrote: > > On 2017/07/13 16:02:18, dougarnett wrote: > > > Should we be able to correlate this event somehow with the type of preview > > that > > > was presented (and the user didn't like)? > > > > That detection is done in the observer itself. Since this only pushes a > > notification for opt outs (and not for non-opt outs), using this code path to > > push type isn't helpful. > > I'm asking a question not (yet) asking for a change. Do you expect we will be > able to correlate this event to the type of preview server-side? Eg, is it > grouped with page view group of metrics that would allow for that? Absolutely. In fact, in this case the type and opt out will be sent in the same proto (core metrics could be sent in a different proto and grouped server side potentially).
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1500318482777740,
"parent_rev": "4208cb5e466ae9c2a5d48a493ac9e68672e7ffb7", "commit_rev":
"1806f8e181472bb2fb3d32ce575c4b2ec8a36bf2"}
Message was sent while issue was closed.
Description was changed from ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711,728707 ========== to ========== Adding previews information to PLM UKM This adds four metrics to the PLM UKM for previews: "opt_out", set to true when the user hit "Show Original" on a preview; "server_lofi", "client_lofi", and "lite_page" set to true when a preview was shown. When no preview is shown, no metrics are added to the UKM. Similarly, when the user does not opt out, nothing is added for opt_out. This does not track offline previews yet, but may in the future. The infobar posts an event to page_load_metrics for the web contents when the user clicks "show original" on the page. BUG=701514,723711,728707 Review-Url: https://codereview.chromium.org/2952343004 Cr-Commit-Position: refs/heads/master@{#487238} Committed: https://chromium.googlesource.com/chromium/src/+/1806f8e181472bb2fb3d32ce575c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/1806f8e181472bb2fb3d32ce575c... |
