Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763703002:
[Doodle] Track the time to took for the Doodle to show up on the NTP (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago CC:
chromium-reviews, noyau+watch_chromium.org, asvitkine+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org, Michael van Ouwerkerk Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Doodle] Track the time to took for the Doodle to show up on the NTP
We'll use this to compare the old components/search_provider_logos with
the new components/doodle, to make sure it doesn't regress.
While we're here, also update/clarify some of the existing
NewTabPage.Logo* histogram descriptions.
BUG=703227
Review-Url: https://codereview.chromium.org/2763703002
Cr-Commit-Position: refs/heads/master@{#458580}
Committed: https://chromium.googlesource.com/chromium/src/+/25f0fa80874f1f91402aa3de3fbb9738d03e05e6
Patch Set 1 #
Total comments: 5
Patch Set 2 : review #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 30 (20 generated)
Description was changed from ========== [Doodle] Track the time to took for the Doodle to show up on the NTP We'll use this to compare the old components/search_provider_logos with the new components/doodle, to make sure it doesn't regress. BUG=703227 ========== to ========== [Doodle] Track the time to took for the Doodle to show up on the NTP We'll use this to compare the old components/search_provider_logos with the new components/doodle, to make sure it doesn't regress. While we're here, also update/clarify some of the existing NewTabPage.Logo* histogram descriptions. BUG=703227 ==========
The CQ bit was checked by treib@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.
treib@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
PTAL!
dgn@chromium.org changed reviewers: + dgn@chromium.org
(michael ooo today) https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:87: mLoadTimeStart = System.currentTimeMillis(); this could be a final local variable rather than a member of the class. https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:98: if (!mHasRecordedLoadTime) { why is this guarded by another boolean? Do we fetch the logo multiple times? If that's the case, adding a comment to explain how that happens and maybe filing a bug to fix it would be nice
https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:87: mLoadTimeStart = System.currentTimeMillis(); On 2017/03/21 10:28:10, dgn wrote: > this could be a final local variable rather than a member of the class. Nice! Done. https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:98: if (!mHasRecordedLoadTime) { On 2017/03/21 10:28:10, dgn wrote: > why is this guarded by another boolean? Do we fetch the logo multiple times? If > that's the case, adding a comment to explain how that happens and maybe filing a > bug to fix it would be nice The mLogoBridge.getCurrentLogo() call below also registers us as an observer (it should really be renamed, I'll do that eventually). If the Doodle changes afterwards, we'll update the NTP in-place; that's by design.
(and thanks for jumping in!)
treib@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms.xml. PTAL!
lgtm https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2763703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:98: if (!mHasRecordedLoadTime) { On 2017/03/21 10:44:15, Marc Treib wrote: > On 2017/03/21 10:28:10, dgn wrote: > > why is this guarded by another boolean? Do we fetch the logo multiple times? > If > > that's the case, adding a comment to explain how that happens and maybe filing > a > > bug to fix it would be nice > > The mLogoBridge.getCurrentLogo() call below also registers us as an observer (it > should really be renamed, I'll do that eventually). If the Doodle changes > afterwards, we'll update the NTP in-place; that's by design. And we care about the time it takes to the first display, not if it gets updated afterwards, got it.
The CQ bit was checked by treib@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 treib@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.
treib@chromium.org changed reviewers: - mvanouwerkerk@chromium.org
LGTM, thanks. https://codereview.chromium.org/2763703002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2763703002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:27: private static final String LOGO_SHOWN_TIME_UMA_NAME = "NewTabPage.LogoShownTime"; Out of curiosity, what's the reason for defining this as a top-level constant rather than just inlining the string?
https://codereview.chromium.org/2763703002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2763703002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:27: private static final String LOGO_SHOWN_TIME_UMA_NAME = "NewTabPage.LogoShownTime"; On 2017/03/21 21:01:54, Ilya Sherman wrote: > Out of curiosity, what's the reason for defining this as a top-level constant > rather than just inlining the string? Nothing in particular, just consistency with the other histogram names here. One of them is used twice, so I'd guess the original author wanted to not duplicate the string.
The CQ bit was checked by treib@chromium.org
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": 20001, "attempt_start_ts": 1490134239779520,
"parent_rev": "58a3da9e52b42cc3f8df506fd40ef4d52e6d2c97", "commit_rev":
"25f0fa80874f1f91402aa3de3fbb9738d03e05e6"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Track the time to took for the Doodle to show up on the NTP We'll use this to compare the old components/search_provider_logos with the new components/doodle, to make sure it doesn't regress. While we're here, also update/clarify some of the existing NewTabPage.Logo* histogram descriptions. BUG=703227 ========== to ========== [Doodle] Track the time to took for the Doodle to show up on the NTP We'll use this to compare the old components/search_provider_logos with the new components/doodle, to make sure it doesn't regress. While we're here, also update/clarify some of the existing NewTabPage.Logo* histogram descriptions. BUG=703227 Review-Url: https://codereview.chromium.org/2763703002 Cr-Commit-Position: refs/heads/master@{#458580} Committed: https://chromium.googlesource.com/chromium/src/+/25f0fa80874f1f91402aa3de3fbb... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/25f0fa80874f1f91402aa3de3fbb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
