Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750503004:
Add metric to record users opening a new NTP. (Closed)
DescriptionAdd metric to record users returning to an NTP.
Record the number of users that open a completely new NTP.
In combination with the metric MobileNTPShown, this can help
to figure out how many users potentially see a "snap change"
of the contents, caused by screenshots made on leave.
BUG=700918
Review-Url: https://codereview.chromium.org/2750503004
Cr-Commit-Position: refs/heads/master@{#457454}
Committed: https://chromium.googlesource.com/chromium/src/+/bb383d154b568ef7aaf4e8d7c4056eb4f71a4ce0
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix major logic error #
Total comments: 4
Patch Set 3 : Observe ModelSelector to record new NTPs #
Total comments: 4
Patch Set 4 : Adjust description for action #
Messages
Total messages: 34 (16 generated)
fhorschig@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Hi Michael, could you please have a look at this new metric? I want to record every user who returns to an open NTP and hope that these places are sufficient to record that.
fhorschig@chromium.org changed reviewers: + holte@chromium.org
Hi Steven, could you please take a look at the new action?
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:513: if (mHasBeenShownBefore) { I don't understand the intention of this variable name. I would expect that only _if_ it has been shown before, you can show it _again_. The logic here does the opposite. https://codereview.chromium.org/2750503004/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2750503004/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10016: The The user returned to a New Tab Page (NTP) that was shown before. Remove 'The '. It's cleaner.
dgn@chromium.org changed reviewers: + dgn@chromium.org
https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:513: if (mHasBeenShownBefore) { nit: the java style is inline return
Thank you for pointing that error out! https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:513: if (mHasBeenShownBefore) { On 2017/03/13 16:46:11, dgn wrote: > nit: the java style is inline return Sorry, I didn't check the git-cl-format output. https://codereview.chromium.org/2750503004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:513: if (mHasBeenShownBefore) { On 2017/03/13 16:42:56, Michael van Ouwerkerk wrote: > I don't understand the intention of this variable name. I would expect that only > _if_ it has been shown before, you can show it _again_. The logic here does the > opposite. Yes, inverted the logic again. Thanks for spotting that error! https://codereview.chromium.org/2750503004/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2750503004/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10016: The The user returned to a New Tab Page (NTP) that was shown before. On 2017/03/13 16:42:56, Michael van Ouwerkerk wrote: > Remove 'The '. It's cleaner. Done.
lgtm with nit https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:102: private boolean mHasBeenShownBefore; nit: rename to "mShown" to align with everything else. Plus it's less verbose.
treib@chromium.org changed reviewers: + treib@chromium.org
LGTM, assuming you've verified that this actually works if the NTP got kicked out in the meantime. (Does the NewTabPage object still exist in that case?) Description nit: This doesn't really record the number of users; it records the number of times any user returns to an NTP. https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:517: RecordUserAction.record("MobileNTPVisitedAgain"); MobileNTPShownAgain, for consistency with the existing MobileNTPShown?
On 2017/03/14 08:22:37, Marc Treib wrote: > LGTM, assuming you've verified that this actually works if the NTP got kicked > out in the meantime. (Does the NewTabPage object still exist in that case?) Wait, no that won't work. If the NTP is destroyed and re-instantiated, its state is of course reset.
On 2017/03/14 11:21:35, Michael van Ouwerkerk wrote: > On 2017/03/14 08:22:37, Marc Treib wrote: > > LGTM, assuming you've verified that this actually works if the NTP got kicked > > out in the meantime. (Does the NewTabPage object still exist in that case?) > > Wait, no that won't work. If the NTP is destroyed and re-instantiated, its state > is of course reset. That's correct. I have been looking into ways to measure the right metric. (As soon as I have verified that it works, I will ping you again.)
Hi Michael, would you please take a look at the new approach to recording new NTPs? This is the inverted case by the way. I measure the total number of new NTPs and can then calculate with the existing NTPShown metric how often an NTP was created before it was shown. Updating description accordingly. https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:102: private boolean mHasBeenShownBefore; On 2017/03/13 17:04:15, Michael van Ouwerkerk wrote: > nit: rename to "mShown" to align with everything else. Plus it's less verbose. Done. https://codereview.chromium.org/2750503004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:517: RecordUserAction.record("MobileNTPVisitedAgain"); On 2017/03/14 08:22:36, Marc Treib wrote: > MobileNTPShownAgain, for consistency with the existing MobileNTPShown? Done.
Description was changed from ========== Add metric to record users returning to an NTP. Record the number of users that revisit an already open NTP. This can help to figure out how many users potentially see a "snap change" of the contents, caused by screenshots (made on leave). BUG=700918 ========== to ========== Add metric to record users returning to an NTP. Record the number of users that open a completely new NTP. In combination with the metric MobileNTPShown, this can help to figure out how many users potentially see a "snap change" of the contents, caused by screenshots made on leave. BUG=700918 ==========
https://codereview.chromium.org/2750503004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2750503004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:327: public void onNewTabCreated(Tab tab) { This seems like a really roundabout way of recording NTP instantiations. Why not just track this from the NewTabPage constructor? We already log things like NewTabPage.LoadType there so you could even look at that if all you need is to count instantiations. https://codereview.chromium.org/2750503004/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2750503004/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9967: The user manually created a New Tab Page (NTP) in a new tab that has not Have you confirmed that this indeed is not logged at startup when an NTP is created? Because in that case the user did not exactly manually create one.
https://codereview.chromium.org/2750503004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2750503004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:327: public void onNewTabCreated(Tab tab) { On 2017/03/14 16:07:36, Michael van Ouwerkerk wrote: > This seems like a really roundabout way of recording NTP instantiations. Why not > just track this from the NewTabPage constructor? We already log things like > NewTabPage.LoadType there so you could even look at that if all you need is to > count instantiations. I am aware about NewTabPage.LoadType. Sadly, any existing NTP that gets kicked out of the memory will trigger the constructor again which is what I explicitly want to avoid. I need to count instances but I don't accidentally want to count navigations back to an open NTP which triggers an instantiation. (If NewTabPage.LoadType has that same requirement, we should consider moving it) https://codereview.chromium.org/2750503004/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2750503004/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9967: The user manually created a New Tab Page (NTP) in a new tab that has not On 2017/03/14 16:07:36, Michael van Ouwerkerk wrote: > Have you confirmed that this indeed is not logged at startup when an NTP is > created? Because in that case the user did not exactly manually create one. Adjusted description to match my intention. If you like, I can exclude the "manually", too but by opening chrome, the user triggers that NTP. I know, the line is blurry here but I want it to be logged (and confirmed that), because that instantiation is never happening due to a navigation which is what I really want to figure out by measuring this. If it (surprisingly) turns out to be unwanted, we can use the LoadType metric to correct for cold starts. For warm starts, the navigation rules should apply as usual.
lgtm
fhorschig@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, would you please take a look at the changes affecting the ChromeActivity?
The CQ bit was checked by fhorschig@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.
Thanks for the additional details Friedrich. This also lgtm.
lgtm
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2750503004/#ps60001 (title: "Adjust description for action")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489679679172420,
"parent_rev": "ed13215635f34ea3026cbb0da8c91dad903815dc", "commit_rev":
"bb383d154b568ef7aaf4e8d7c4056eb4f71a4ce0"}
Message was sent while issue was closed.
Description was changed from ========== Add metric to record users returning to an NTP. Record the number of users that open a completely new NTP. In combination with the metric MobileNTPShown, this can help to figure out how many users potentially see a "snap change" of the contents, caused by screenshots made on leave. BUG=700918 ========== to ========== Add metric to record users returning to an NTP. Record the number of users that open a completely new NTP. In combination with the metric MobileNTPShown, this can help to figure out how many users potentially see a "snap change" of the contents, caused by screenshots made on leave. BUG=700918 Review-Url: https://codereview.chromium.org/2750503004 Cr-Commit-Position: refs/heads/master@{#457454} Committed: https://chromium.googlesource.com/chromium/src/+/bb383d154b568ef7aaf4e8d7c405... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bb383d154b568ef7aaf4e8d7c405... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
