[Zine] Add a metric to record number of visible sections on NTP.
Currently we record number of suggestions per section on NTP opened
(CountOnNTPOpened). However, recently we needed to check how many times
the user sees only one section.
This CL adds a new metric SectionCountOnNTPOpened to record number of
visible sections on each NTP.
BUG=721276
Review-Url: https://codereview.chromium.org/2886723002
Cr-Commit-Position: refs/heads/master@{#473853}
Committed: https://chromium.googlesource.com/chromium/src/+/f22c112e0e74aa001c68b93b95824a8a5b554921
3 years, 7 months ago
(2017-05-18 22:57:06 UTC)
#5
https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):
https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:76:
visibleCategoriesCount += 1;
nit: increment style consistency. Either change here to
`++visibleCategoriesCount` or line 80 to `categoryIndex += 1`. I don't feel
strongly about either style.
https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:95:
private boolean isCategoryHidden(@CategoryInt int category, boolean
alwaysAllowEmptySections) {
I don't like this because it adds JNI calls while the information should already
be available. After resetSection is called, if the section is visible, it should
be added to the mSections map.
So you should be able to just change line 75 to
if (mSections.get(category) != null) {
vitaliii
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-19 08:37:49 UTC)
#6
I've addressed your comment, please have a look. https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java (right): https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java#newcode76 chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:76: visibleCategoriesCount ...
3 years, 7 months ago
(2017-05-19 08:38:12 UTC)
#8
I've addressed your comment,
please have a look.
https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):
https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:76:
visibleCategoriesCount += 1;
On 2017/05/18 22:57:06, dgn wrote:
> nit: increment style consistency. Either change here to
> `++visibleCategoriesCount` or line 80 to `categoryIndex += 1`. I don't feel
> strongly about either style.
Done.
https://codereview.chromium.org/2886723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:95:
private boolean isCategoryHidden(@CategoryInt int category, boolean
alwaysAllowEmptySections) {
On 2017/05/18 22:57:06, dgn wrote:
> I don't like this because it adds JNI calls while the information should
already
> be available. After resetSection is called, if the section is visible, it
should
> be added to the mSections map.
>
> So you should be able to just change line 75 to
>
> if (mSections.get(category) != null) {
Done.
Thank you, I completely missed that all of these are JNI calls.
I did consider mSections, but it seemed a bit indirect.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 7 months ago
(2017-05-19 09:07:32 UTC)
#9
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/298099)
3 years, 7 months ago
(2017-05-19 09:07:33 UTC)
#10
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/443677)
3 years, 7 months ago
(2017-05-22 11:55:53 UTC)
#25
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/273836) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 7 months ago
(2017-05-23 09:10:04 UTC)
#37
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495533382030320, "parent_rev": "6846b19d50a21eed54c28a76ddc63d976ebff215", "commit_rev": "f22c112e0e74aa001c68b93b95824a8a5b554921"}
3 years, 7 months ago
(2017-05-23 10:49:11 UTC)
#43
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495533382030320,
"parent_rev": "6846b19d50a21eed54c28a76ddc63d976ebff215", "commit_rev":
"f22c112e0e74aa001c68b93b95824a8a5b554921"}
commit-bot: I haz the power
Description was changed from ========== [Zine] Add a metric to record number of visible sections ...
3 years, 7 months ago
(2017-05-23 10:49:28 UTC)
#44
Message was sent while issue was closed.
Description was changed from
==========
[Zine] Add a metric to record number of visible sections on NTP.
Currently we record number of suggestions per section on NTP opened
(CountOnNTPOpened). However, recently we needed to check how many times
the user sees only one section.
This CL adds a new metric SectionCountOnNTPOpened to record number of
visible sections on each NTP.
BUG=721276
==========
to
==========
[Zine] Add a metric to record number of visible sections on NTP.
Currently we record number of suggestions per section on NTP opened
(CountOnNTPOpened). However, recently we needed to check how many times
the user sees only one section.
This CL adds a new metric SectionCountOnNTPOpened to record number of
visible sections on each NTP.
BUG=721276
Review-Url: https://codereview.chromium.org/2886723002
Cr-Commit-Position: refs/heads/master@{#473853}
Committed:
https://chromium.googlesource.com/chromium/src/+/f22c112e0e74aa001c68b93b9582...
==========
commit-bot: I haz the power
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f22c112e0e74aa001c68b93b95824a8a5b554921
3 years, 7 months ago
(2017-05-23 10:49:30 UTC)
#45
Issue 2886723002: [Zine] Add a metric to record number of visible sections on NTP.
(Closed)
Created 3 years, 7 months ago by vitaliii
Modified 3 years, 7 months ago
Reviewers: dgn, jkrcal, Ilya Sherman
Base URL:
Comments: 5