Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006203002:
Fix that NewTabPage.Snippets.CardShown was recorded too early/often. (Closed)
|
Created:
4 years, 7 months ago by jkrcal Modified:
4 years, 6 months ago CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix that NewTabPage.Snippets.CardShown was recorded too early/often.
The NewTabPage.Snippets.CardShown histogram for the first snippet was
recorded before actually seeing the snippet (only the peeking card).
Furthermore, the histogram was recorded whenever a snippet becomes
visible - by scrolling up and down, a snippet could get many
impressions.
Now, impression is tracked only once per snippet per NTP instance.
Furthermore, it is tracked whenever at least 1/3 of height of the snippet gets visible on the screen.
A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well.
BUG=610687
Committed: https://crrev.com/7b698748e1f0b4b3bf928be00a1ed423d2bc88c4
Cr-Commit-Position: refs/heads/master@{#396477}
Patch Set 1 #Patch Set 2 : Looping over visible snippets more efficiently #Patch Set 3 : Minor polish #
Total comments: 8
Patch Set 4 : Code review #
Total comments: 14
Patch Set 5 : After code review #2 #
Total comments: 8
Patch Set 6 : After code review #3 #Patch Set 7 : Rebase #
Total comments: 4
Patch Set 8 : A minor polish #
Dependent Patchsets: Messages
Total messages: 42 (16 generated)
Description was changed from ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever a visible snippet gets scrolled in the RecyclerView. BUG=608365 ========== to ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever a visible snippet gets scrolled in the RecyclerView. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=608365 ==========
The CQ bit was checked by jkrcal@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/2006203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006203002/40001
jkrcal@chromium.org changed reviewers: + tedchoc@chromium.org
PTAL
mastiz@chromium.org changed reviewers: + mastiz@chromium.org
High-level: the linked bug documents that one of the conditions to count an impression is to be below the fold. This doesn't seem to be implemented in this patch, so I suggest you either change the implementation or update the bug with a new proposal.
Description was changed from ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever a visible snippet gets scrolled in the RecyclerView. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=608365 ========== to ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever a visible snippet gets scrolled in the RecyclerView. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 ==========
On 2016/05/24 12:45:10, mastiz wrote: > High-level: the linked bug documents that one of the conditions to count an > impression is to be below the fold. This doesn't seem to be implemented in this > patch, so I suggest you either change the implementation or update the bug with > a new proposal. I'll update the bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
peconn@chromium.org changed reviewers: + peconn@chromium.org
https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:526: private void notifySnippetsImpressed() { Could you move this to the NewTabPageRecyclerView - I don't think there's any need for it to be in this file (and NewTabPageView is pretty massive as it is). https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:669: if (dy != 0) { If you add some logging, does this ever get called with dy == 0? I'd assume that (since the callback is called 'onScrolled') it is only ever called when scrolling has occured, and since we can only scroll vertically, I doubt dy will ever be 0. Maybe it's worth keeping this check in though, just in case. https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:29: private boolean mImpressionTracked = false; You don't need the '= false' here, it will be initialized to false by default. https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:91: RecordHistogram.recordSparseSlowlyHistogram("NewTabPage.Snippets.CardShown", mPosition); So, the position is passed in when the SnippetArticle is created, but it is not updated as the snippets are dismissed. This may be what you want (if you want to know that people liked to view the 5th website that the server sends to the client), or maybe not (if you want to know that people scroll to see the 7th item in the list). You are currently tracking the former, if you wanted to track the latter you would need to pass in 'getLayoutPosition()' from the ViewHolder (and account for the layout position including the above the fold layout).
On 2016/05/24 13:05:04, PEConn1 wrote: > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > (right): > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:526: > private void notifySnippetsImpressed() { > Could you move this to the NewTabPageRecyclerView - I don't think there's any > need for it to be in this file (and NewTabPageView is pretty massive as it is). > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:669: > if (dy != 0) { > If you add some logging, does this ever get called with dy == 0? I'd assume that > (since the callback is called 'onScrolled') it is only ever called when > scrolling has occured, and since we can only scroll vertically, I doubt dy will > ever be 0. > > Maybe it's worth keeping this check in though, just in case. > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java > (right): > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:29: > private boolean mImpressionTracked = false; > You don't need the '= false' here, it will be initialized to false by default. > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:91: > RecordHistogram.recordSparseSlowlyHistogram("NewTabPage.Snippets.CardShown", > mPosition); > So, the position is passed in when the SnippetArticle is created, but it is not > updated as the snippets are dismissed. This may be what you want (if you want to > know that people liked to view the 5th website that the server sends to the > client), or maybe not (if you want to know that people scroll to see the 7th > item in the list). > > You are currently tracking the former, if you wanted to track the latter you > would need to pass in 'getLayoutPosition()' from the ViewHolder (and account for > the layout position including the above the fold layout). For the later, does a scroll-based tracking work? Discarding a snippet can cause a new one to be shown. Does that trigger a scroll event? Looks fragile.
On 2016/05/24 13:13:32, mastiz wrote: > On 2016/05/24 13:05:04, PEConn1 wrote: > > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > > (right): > > > > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:526: > > private void notifySnippetsImpressed() { > > Could you move this to the NewTabPageRecyclerView - I don't think there's any > > need for it to be in this file (and NewTabPageView is pretty massive as it > is). > > > > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:669: > > if (dy != 0) { > > If you add some logging, does this ever get called with dy == 0? I'd assume > that > > (since the callback is called 'onScrolled') it is only ever called when > > scrolling has occured, and since we can only scroll vertically, I doubt dy > will > > ever be 0. > > > > Maybe it's worth keeping this check in though, just in case. > > > > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java > > (right): > > > > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:29: > > private boolean mImpressionTracked = false; > > You don't need the '= false' here, it will be initialized to false by default. > > > > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:91: > > RecordHistogram.recordSparseSlowlyHistogram("NewTabPage.Snippets.CardShown", > > mPosition); > > So, the position is passed in when the SnippetArticle is created, but it is > not > > updated as the snippets are dismissed. This may be what you want (if you want > to > > know that people liked to view the 5th website that the server sends to the > > client), or maybe not (if you want to know that people scroll to see the 7th > > item in the list). > > > > You are currently tracking the former, if you wanted to track the latter you > > would need to pass in 'getLayoutPosition()' from the ViewHolder (and account > for > > the layout position including the above the fold layout). > > For the later, does a scroll-based tracking work? Discarding a snippet can cause > a new one to be shown. Does that trigger a scroll event? Looks fragile. No matter which of the two we are tracking, if the user dismisses a snippet at the bottom of the page and a new snippet slides up to take its place, that new snippet is not registered as being displayed until the user scrolls somewhere.
I reflected all the comments so far. As Mikel points out, relying on scrolling is a bit fragile. Now I do it this way only for the first snippet in NewTabPageView. The remaining snippets can get recorded whenever their view gets attached to the window. Frankly, I am not sure if the current version is better than the previous one in terms of robustness. I just do not see any better way to implement the expected behaviour that I documented in histograms.xml https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:526: private void notifySnippetsImpressed() { On 2016/05/24 13:05:03, PEConn1 wrote: > Could you move this to the NewTabPageRecyclerView - I don't think there's any > need for it to be in this file (and NewTabPageView is pretty massive as it is). I changed the code. Now I would have to move/reimplement getFirstViewMatchingViewType(). Does it still make sense? https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:669: if (dy != 0) { On 2016/05/24 13:05:04, PEConn1 wrote: > If you add some logging, does this ever get called with dy == 0? I'd assume that > (since the callback is called 'onScrolled') it is only ever called when > scrolling has occured, and since we can only scroll vertically, I doubt dy will > ever be 0. > > Maybe it's worth keeping this check in though, just in case. It is explained in the comment above. I've tested it, it gets called with both dx and dy equal to 0 at the beginning when the NTP opens. Don't know whether this is expected behaviour of RecyclerView... https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:29: private boolean mImpressionTracked = false; On 2016/05/24 13:05:04, PEConn1 wrote: > You don't need the '= false' here, it will be initialized to false by default. Done. https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:91: RecordHistogram.recordSparseSlowlyHistogram("NewTabPage.Snippets.CardShown", mPosition); On 2016/05/24 13:05:04, PEConn1 wrote: > So, the position is passed in when the SnippetArticle is created, but it is not > updated as the snippets are dismissed. This may be what you want (if you want to > know that people liked to view the 5th website that the server sends to the > client), or maybe not (if you want to know that people scroll to see the 7th > item in the list). > > You are currently tracking the former, if you wanted to track the latter you > would need to pass in 'getLayoutPosition()' from the ViewHolder (and account for > the layout position including the above the fold layout). Good point. I do not see strong reasons for either of the two options. I see some weak contradicting reasons for each of them. I suggest to stick to the position passed by the server. I'll document it in the histogram description.
On 2016/05/24 15:58:13, jkrcal wrote: > I reflected all the comments so far. As Mikel points out, relying on scrolling > is a bit fragile. Now I do it this way only for the first snippet in > NewTabPageView. The remaining snippets can get recorded whenever their view gets > attached to the window. > > Frankly, I am not sure if the current version is better than the previous one in > terms of robustness. I just do not see any better way to implement the expected > behaviour that I documented in histograms.xml > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > (right): > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:526: > private void notifySnippetsImpressed() { > On 2016/05/24 13:05:03, PEConn1 wrote: > > Could you move this to the NewTabPageRecyclerView - I don't think there's any > > need for it to be in this file (and NewTabPageView is pretty massive as it > is). > > I changed the code. Now I would have to move/reimplement > getFirstViewMatchingViewType(). Does it still make sense? > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:669: > if (dy != 0) { > On 2016/05/24 13:05:04, PEConn1 wrote: > > If you add some logging, does this ever get called with dy == 0? I'd assume > that > > (since the callback is called 'onScrolled') it is only ever called when > > scrolling has occured, and since we can only scroll vertically, I doubt dy > will > > ever be 0. > > > > Maybe it's worth keeping this check in though, just in case. > > It is explained in the comment above. I've tested it, it gets called with both > dx and dy equal to 0 at the beginning when the NTP opens. Don't know whether > this is expected behaviour of RecyclerView... > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java > (right): > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:29: > private boolean mImpressionTracked = false; > On 2016/05/24 13:05:04, PEConn1 wrote: > > You don't need the '= false' here, it will be initialized to false by default. > > Done. > > https://codereview.chromium.org/2006203002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:91: > RecordHistogram.recordSparseSlowlyHistogram("NewTabPage.Snippets.CardShown", > mPosition); > On 2016/05/24 13:05:04, PEConn1 wrote: > > So, the position is passed in when the SnippetArticle is created, but it is > not > > updated as the snippets are dismissed. This may be what you want (if you want > to > > know that people liked to view the 5th website that the server sends to the > > client), or maybe not (if you want to know that people scroll to see the 7th > > item in the list). > > > > You are currently tracking the former, if you wanted to track the latter you > > would need to pass in 'getLayoutPosition()' from the ViewHolder (and account > for > > the layout position including the above the fold layout). > > Good point. I do not see strong reasons for either of the two options. I see > some weak contradicting reasons for each of them. I suggest to stick to the > position passed by the server. > > I'll document it in the histogram description. A quick additional rationale behind the last change: - The new version is definitely more robust under the assumption that one can only get below to fold by scrolling down. I am just not sure how future-proof this assumption is.
https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:661: if (dy != 0) { what happens if the user clicks on the fake box? Does this get triggered since it triggers scrolling programmatically? Do we want it too (there is a shield from the omnibox and this might be covered up by the keyboard)? https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:85: public void trackImpression() { for my own education, SnippetArticle[s] are queried only once per NTP and they are the data objects backing the list? Just making sure they're not getting regenerated where mImpressionTracked needs to be done at a different level https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:87: if (mImpressionTracked) { you can put this all on a single line since you're in java land (where the statement and conditional can all fit on a single line) https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:50: private SnippetArticle mArticle = null; null is the default, so you don't need this https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:88: trackImpression(); I do wonder if we could use a combination of: https://developer.android.com/reference/android/view/View.html#getViewTreeObs... and https://developer.android.com/reference/android/view/ViewParent.html#getChild..., android.graphics.Rect, android.graphics.Point) In particular, I think you'll end up getting similar problems you have with the first entry as you will with the last where potentially only a sliver of it is visible. I'm <hand wavy> thinking you could track when the parent is getting drawn and then comparing the visibility region of its children to see if it exceeds some threshold that you deem impression worthy. Granted, that would be for a truly correct solution, but maybe that is simply not worth it. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:106: public void trackImpression() { javadoc https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:108: mArticle.trackImpression(); same comment here about the one-liner-ness (and above for that matter).
Another complete overhaul :) PTAL again. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:661: if (dy != 0) { On 2016/05/24 20:46:53, Ted C wrote: > what happens if the user clicks on the fake box? Does this get triggered since > it triggers scrolling programmatically? Do we want it too (there is a shield > from the omnibox and this might be covered up by the keyboard)? Not it was not recorded, the transition probably was not scrolling. Anyway, I've changed the implementation as you had suggested below. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:85: public void trackImpression() { On 2016/05/24 20:46:54, Ted C wrote: > for my own education, SnippetArticle[s] are queried only once per NTP and they > are the data objects backing the list? Just making sure they're not getting > regenerated where mImpressionTracked needs to be done at a different level Yes, SnippetArticle[s] are the data objects for the list. The SnippetArticle objects get generated whenever a new set of snippets is loaded. At the moment this happens only once per NTP instance as the android NTP UI does not listen to any changes in the data set of the snippet service. This may potentially change in the future, though. I've been thinking about this and I think it is okay to record it as another impression if the UI changes (new snippets get displayed). Thus, I suggest to keep the recording here; I've updated the definition of the histogram to be more robust with respect to implementation changes. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:87: if (mImpressionTracked) { On 2016/05/24 20:46:54, Ted C wrote: > you can put this all on a single line since you're in java land (where the > statement and conditional can all fit on a single line) Done. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:50: private SnippetArticle mArticle = null; On 2016/05/24 20:46:54, Ted C wrote: > null is the default, so you don't need this Done. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:88: trackImpression(); On 2016/05/24 20:46:54, Ted C wrote: > I do wonder if we could use a combination of: > https://developer.android.com/reference/android/view/View.html#getViewTreeObs... > > and > > https://developer.android.com/reference/android/view/ViewParent.html#getChild..., > android.graphics.Rect, android.graphics.Point) > > In particular, I think you'll end up getting similar problems you have with the > first entry as you will with the last where potentially only a sliver of it is > visible. I'm <hand wavy> thinking you could track when the parent is getting > drawn and then comparing the visibility region of its children to see if it > exceeds some threshold that you deem impression worthy. > > Granted, that would be for a truly correct solution, but maybe that is simply > not worth it. Done. I am not sure either if it was worth it :) At least I like it now much better that any previous implementation. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:106: public void trackImpression() { On 2016/05/24 20:46:54, Ted C wrote: > javadoc Done. https://codereview.chromium.org/2006203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:108: mArticle.trackImpression(); On 2016/05/24 20:46:54, Ted C wrote: > same comment here about the one-liner-ness (and above for that matter). Done.
lgtm https://codereview.chromium.org/2006203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2006203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:90: cardView.getParent().getChildVisibleRect(cardView, r, null); this actually works?! yay!
jkrcal@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei: PTAL at histograms.xml. https://codereview.chromium.org/2006203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2006203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:90: cardView.getParent().getChildVisibleRect(cardView, r, null); On 2016/05/25 18:53:32, Ted C wrote: > this actually works?! yay! Yes! Brilliant! :)
Description was changed from ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever a visible snippet gets scrolled in the RecyclerView. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 ========== to ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever at least 1/3 of height of the snippet gets visible on the screen. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 ==========
The CQ bit was checked by jkrcal@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/2006203002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006203002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33700: + is shown. We track the position the snippet had in the list when NTP was Nit: Extra space after . https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33701: + loaded. This tracked position is thus different from the position observed Nit: Extra space after thus. https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33703: + discards some snippets in the top of the list. Maybe worth mentioning that previously it was recorded differently?
Thanks, Alexei! PTAL, again. https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33700: + is shown. We track the position the snippet had in the list when NTP was On 2016/05/25 20:23:08, Alexei Svitkine (slow) wrote: > Nit: Extra space after . Done. https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33701: + loaded. This tracked position is thus different from the position observed On 2016/05/25 20:23:08, Alexei Svitkine (slow) wrote: > Nit: Extra space after thus. Done. https://codereview.chromium.org/2006203002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33703: + discards some snippets in the top of the list. On 2016/05/25 20:23:08, Alexei Svitkine (slow) wrote: > Maybe worth mentioning that previously it was recorded differently? Done.
The CQ bit was checked by jkrcal@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/2006203002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006203002/120001
LGTM https://codereview.chromium.org/2006203002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2006203002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:95: // Track impression if at least one third of the snippet is shown Nit: final dot missing. https://codereview.chromium.org/2006203002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:156: mScore = mArticle.mScore; Are all these copies needed now that a reference to mArticle is held? If not, add a TODO?
Thanks, Mikel for the comments! Alexei, PTAL again on histograms.xml. https://codereview.chromium.org/2006203002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2006203002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:95: // Track impression if at least one third of the snippet is shown On 2016/05/27 12:46:33, mastiz wrote: > Nit: final dot missing. Done. https://codereview.chromium.org/2006203002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:156: mScore = mArticle.mScore; On 2016/05/27 12:46:33, mastiz wrote: > Are all these copies needed now that a reference to mArticle is held? If not, > add a TODO? I've already removed them in a follow-up CL https://codereview.chromium.org/2013423002/. Could have done it here, that's true.
lgtm
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mastiz@chromium.org Link to the patchset: https://codereview.chromium.org/2006203002/#ps140001 (title: "A minor polish")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006203002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006203002/140001
Message was sent while issue was closed.
Description was changed from ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever at least 1/3 of height of the snippet gets visible on the screen. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 ========== to ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever at least 1/3 of height of the snippet gets visible on the screen. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever at least 1/3 of height of the snippet gets visible on the screen. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 ========== to ========== Fix that NewTabPage.Snippets.CardShown was recorded too early/often. The NewTabPage.Snippets.CardShown histogram for the first snippet was recorded before actually seeing the snippet (only the peeking card). Furthermore, the histogram was recorded whenever a snippet becomes visible - by scrolling up and down, a snippet could get many impressions. Now, impression is tracked only once per snippet per NTP instance. Furthermore, it is tracked whenever at least 1/3 of height of the snippet gets visible on the screen. A further CL will move clicks tracking from SnippetArticleViewHolder into SnippetArticle as well. BUG=610687 Committed: https://crrev.com/7b698748e1f0b4b3bf928be00a1ed423d2bc88c4 Cr-Commit-Position: refs/heads/master@{#396477} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7b698748e1f0b4b3bf928be00a1ed423d2bc88c4 Cr-Commit-Position: refs/heads/master@{#396477} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
