Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833473002:
Record NTP.LogoShownTime for timely refreshs only (Closed)
DescriptionRecord NTP.LogoShownTime for timely refreshs only
This CL corrects the load time metrics for Logos on the NTP.
The old behavior:
-----------------
If the doodle cache is empty, the LogoShownTime will be recorded when
the next DoodleService#Refresh triggers the observer who requested a
Refresh.
If the requested refresh is skipped, the observer records the time when
the next notification happens due to a refresh triggered by a different
caller. This can be timely but is much more likely to be 15 min or more
in the future. This delay blurs the metric.
The wanted/new behavior:
------------------------
This CL introduces a way to know that the refresh was skipped.
The observer that records LogoShownTime uses it to prevent recording
any metrics for this case.
BUG=713166
Review-Url: https://codereview.chromium.org/2833473002
Cr-Commit-Position: refs/heads/master@{#467989}
Committed: https://chromium.googlesource.com/chromium/src/+/02ed0d5fe2949096abd3215af102cf221144da5d
Patch Set 1 #
Total comments: 12
Patch Set 2 : Refactor test and improve comments #
Total comments: 2
Patch Set 3 : Decide recording always in onLogoAvailable. #Patch Set 4 : Record metric only in first onLogoAvailable, never in a second one #
Total comments: 4
Patch Set 5 : Split refresh callback into own interface #
Total comments: 16
Patch Set 6 : Remove skippedCallback and call onLogoAvailable more explicitly #
Total comments: 5
Patch Set 7 : Fix compile errors and comments #
Total comments: 8
Patch Set 8 : Add guarantee to getCurrentLogo comment #Patch Set 9 : Make comments even more precise #Patch Set 10 : Replace |Refresh| return value with treib@'s revalidation event #
Total comments: 8
Patch Set 11 : Trigger OnDoodleConfigRevalidated if expired configs don't change the config #
Total comments: 2
Patch Set 12 : Move test expectation close to code under test #Patch Set 13 : Fix compile error #
Messages
Total messages: 49 (21 generated)
fhorschig@chromium.org changed reviewers: + treib@chromium.org, tschumann@chromium.org
Hi Tim, as Marc is OOO, would you please have a look at this fix for the Doodle metric?
nit about the CL description: Can you add a sentence in the beginning indicating the first paragraphs are about the *old* behavior? Took me a while to realize if it was describing the old or newly introduced behavior. https://codereview.chromium.org/2833473002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:108: public void onLogoRefreshSkipped() { omg, this is really convoluted. Is there any good reason why the metric does not live in the doodle service where we can properly connect the triggering and completion of a request? (1) If the metrics need to live in Java, couldn't we just collect the load-time in the doodle service and pass it into onLogoAvailable? (2) This would also allow to only register a single observer (no need to unregister the old ones in every call to getCurrentLogo() -- which is also quite unexpected). Looking at the call sites of getSearchProviderLogo() it doesn't seem like there's an actual need to provide new instances every time. I might be missing something, of course. As we intent to merge this into M59, I'd suggest to do (2) in a separate CL. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:63: // refreshing was skipped (because the refresh interval hasn't passed yet). nit: i'd simplify the last sentence. what about: Returns false if no fetch was issued because the service has fresh data in it's cache. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:497: // Trigger a refresh within the min refresh interval that is not skipped. if find the last part of the sentence extremely confusing. I guess you're trying to say that this actually results in a fetch? (also see next comment below). https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:499: fetcher()->ServeAllCallbacks(DoodleState::NO_DOODLE, base::TimeDelta(), can you explain the parameters with /**/ comments? Alternatively extending the comment above to something like: // Trigger a refresh that gets the information about no doodle being available. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:503: task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(5)); why forward the time at all? https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:506: // This should not have resulted in a request. nit: combine this comment with the one above and write the 2 expectations in one block?
Some high-level responses first. I am still addressing the other comments. https://codereview.chromium.org/2833473002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:108: public void onLogoRefreshSkipped() { On 2017/04/20 18:53:11, tschumann wrote: > omg, this is really convoluted. That's why I really don't want to defend this. But... > Is there any good reason why the metric does not live in the doodle service > where we can properly connect the triggering and completion of a request? As far as I understood, this metric really wants to capture the end-to-end experience from "This NTP is shown" to "The logo is finally visible". We do have separate metrics for Doodle.ConfigDownloadTime and NewTabPage.LogoDownloadTime. Moving this into the service would capture a similar value but exempt the drawing of the UI. The problem is, that we (afaik) need this metric to measure the _change_ that the new Doodle component brings compared to the old LogoTracker. Therefore, moving it into the Service requires us to duplicate the measurement code for the legacy LogoTracker as well. > > (1) If the metrics need to live in Java, couldn't we just collect the load-time > in the doodle service and pass it into onLogoAvailable? See the reason above. This definitely makes a lot of sense when the legacy code gets removed. Currently, we could still do this, with a lot of code duplication. WDYT is more appropriate here? BTW: We still need a separate callback/observer/boolean to notify about the skipped refresh. Passing the load time along the result would not suffice to fix what this CL currently fixes. > (2) This would also allow to only register a single observer (no need to > unregister the old ones in every call to getCurrentLogo() -- which is also quite > unexpected). Looking at the call sites of getSearchProviderLogo() it doesn't > seem like there's an actual need to provide new instances every time. It is indeed unexpected to see observers frequently reregistered. Maybe it would have been a better idea to call it CallbackWrapper (which is the case on the Java side and makes their use much clearer). I am not sure about the side effects of making the OnceCallbacks persistent. At least, the CallbackWrapper initializes a member for metrics on construction. > I might be missing something, of course. As we intent to merge this into M59, > I'd suggest to do (2) in a separate CL. I agree. In fact, changing the observer to pass the loading time requires a lot of changes in different places. Therefore, I would really like to go for a simple solution first.
Description was changed from ========== Record NTP.LogoShownTime for timely refreshs only If the doodle cache is empty, the LogoShownTime will be recorded when the next DoodleService#Refresh triggers the observer who requested a Refresh. If the requested refresh is skipped, the observer records the time when the next notification happens due to a refresh triggered by a different caller. This can be timely but is much more likely to be 15 min or more in the future. This delay blurs the metric. This CL introduces a way to know that the refresh was skipped. The observer that records LogoShownTime uses it to prevent recording any metrics for this case. BUG=713166 ========== to ========== Record NTP.LogoShownTime for timely refreshs only This CL corrects the load time metrics for Logos on the NTP. The old behavior: ----------------- If the doodle cache is empty, the LogoShownTime will be recorded when the next DoodleService#Refresh triggers the observer who requested a Refresh. If the requested refresh is skipped, the observer records the time when the next notification happens due to a refresh triggered by a different caller. This can be timely but is much more likely to be 15 min or more in the future. This delay blurs the metric. The wanted/new behavior: ------------------------ This CL introduces a way to know that the refresh was skipped. The observer that records LogoShownTime uses it to prevent recording any metrics for this case. BUG=713166 ==========
Patchset #2 (id:20001) has been deleted
Structured the description and addressed all other comments. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:63: // refreshing was skipped (because the refresh interval hasn't passed yet). On 2017/04/20 18:53:11, tschumann wrote: > nit: i'd simplify the last sentence. what about: > Returns false if no fetch was issued because the service has fresh data in it's > cache. > Sounds good! https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:497: // Trigger a refresh within the min refresh interval that is not skipped. On 2017/04/20 18:53:11, tschumann wrote: > if find the last part of the sentence extremely confusing. I guess you're trying > to say that this actually results in a fetch? (also see next comment below). Yes, changed/merged. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:499: fetcher()->ServeAllCallbacks(DoodleState::NO_DOODLE, base::TimeDelta(), On 2017/04/20 18:53:11, tschumann wrote: > can you explain the parameters with /**/ comments? > Alternatively extending the comment above to something like: > // Trigger a refresh that gets the information about no doodle being available. Done. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:503: task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(5)); On 2017/04/20 18:53:11, tschumann wrote: > why forward the time at all? Gone. https://codereview.chromium.org/2833473002/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:506: // This should not have resulted in a request. On 2017/04/20 18:53:11, tschumann wrote: > nit: combine this comment with the one above and write the 2 expectations in one > block? Done.
I'm not sure if the new behavior is what we want: The metric should record the time it takes for the logo to show up, whether it was cached or not. I think the new behavior would not record anything if the logo was cached, because onLogoRefreshSkipped() will get called before onLogoAvailable() (which has to get the image data from the HTTP cache), right? https://codereview.chromium.org/2833473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:110: if (mIsDestroyed) return; double line
It's absolutely right that cached doodles would have been ignored. I changed that. Additionally, metric recording happens now timely or never. BTW: This CL is kind of a fallback if the (offline discussed) rework of metrics recording in the Bridge turns out to be too large for merging. I will probably tackle https://crbug.com/711199 to test that. https://codereview.chromium.org/2833473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:110: if (mIsDestroyed) return; On 2017/04/25 06:43:42, Marc Treib (OOO till April 25) wrote: > double line Gone.
https://codereview.chromium.org/2833473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:112: public void onLogoRefreshSkipped() { Of the three implementations of this interface, only one implements this method, and that's a wrapper. Since this is the only place that calls LogoBridge.getCurrentLogo, maybe we should split things up, to get onLogoRefreshSkipped out of the LogoObserver interface? https://codereview.chromium.org/2833473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:116: mRecordLoadTimeForNoDoodle = !mHasRecordedLoadTime; This comment (and the name mRecordLoadTimeForNoDoodle) confuse me... What does mRecordLoadTimeForNoDoodle mean? Above, we still only ever record the time if the logo isn't null.
https://codereview.chromium.org/2833473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:112: public void onLogoRefreshSkipped() { On 2017/04/25 16:51:26, Marc Treib wrote: > Of the three implementations of this interface, only one implements this method, > and that's a wrapper. > Since this is the only place that calls LogoBridge.getCurrentLogo, maybe we > should split things up, to get onLogoRefreshSkipped out of the LogoObserver > interface? Done. https://codereview.chromium.org/2833473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:116: mRecordLoadTimeForNoDoodle = !mHasRecordedLoadTime; On 2017/04/25 16:51:26, Marc Treib wrote: > This comment (and the name mRecordLoadTimeForNoDoodle) confuse me... > What does mRecordLoadTimeForNoDoodle mean? Above, we still only ever record the > time if the logo isn't null. Updated the deprecated comment. The name of the variable was misleading. Renamed.
https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java (right): https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:69: public interface RefreshCallbackWrapper { Just RefreshCallback, or RefreshStatusCallback or something? It doesn't wrap anything, right? https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:55: mShouldRecordLoadTime = true; I think the initialization can happen inline with the declaration (though not sure if that's the preferred style in Java?) https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:99: if (mShouldRecordLoadTime && !mHasSkippedFirstDoodleRefresh) { Hm. Won't onRefreshSkipped() get called before onLogoAvailable(cached_logo)? If so, then we'll never record a time for cached logos now. https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:112: LogoBridge.RefreshCallbackWrapper refreshCallbackWrapper = Also here: No wrapping involved https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:117: mHasSkippedFirstDoodleRefresh = true; nit: I'd remove the "First" from the name, since it'll get set for any non-first skipped refreshes too. https://codereview.chromium.org/2833473002/diff/100001/chrome/browser/android... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/2833473002/diff/100001/chrome/browser/android... chrome/browser/android/logo_bridge.cc:232: DoodleConfigReceived(doodle_service_->config(), /*from_cache=*/true); Ah, here's the problem I'm talking about above, and I see that the comment here is misleading: This won't usually call onLogoAvailable immediately, since it first has to fetch the actual image. https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... components/doodle/doodle_service.h:62: // Returns false if no fetch was issued because the service has fresh data in nit: No newline. Also, maybe just "Returns whether a network request was actually sent."?
On 2017/04/26 10:17:17, Marc Treib wrote: > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java > (right): > > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:69: > public interface RefreshCallbackWrapper { > Just RefreshCallback, or RefreshStatusCallback or something? It doesn't wrap > anything, right? > > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java > (right): > > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:55: > mShouldRecordLoadTime = true; > I think the initialization can happen inline with the declaration (though not > sure if that's the preferred style in Java?) > > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:99: > if (mShouldRecordLoadTime && !mHasSkippedFirstDoodleRefresh) { > Hm. Won't onRefreshSkipped() get called before onLogoAvailable(cached_logo)? If > so, then we'll never record a time for cached logos now. > > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:112: > LogoBridge.RefreshCallbackWrapper refreshCallbackWrapper = > Also here: No wrapping involved > > https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:117: > mHasSkippedFirstDoodleRefresh = true; > nit: I'd remove the "First" from the name, since it'll get set for any non-first > skipped refreshes too. > > https://codereview.chromium.org/2833473002/diff/100001/chrome/browser/android... > File chrome/browser/android/logo_bridge.cc (right): > > https://codereview.chromium.org/2833473002/diff/100001/chrome/browser/android... > chrome/browser/android/logo_bridge.cc:232: > DoodleConfigReceived(doodle_service_->config(), /*from_cache=*/true); > Ah, here's the problem I'm talking about above, and I see that the comment here > is misleading: This won't usually call onLogoAvailable immediately, since it > first has to fetch the actual image. > > https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... > File components/doodle/doodle_service.h (right): > > https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... > components/doodle/doodle_service.h:62: // Returns false if no fetch was issued > because the service has fresh data in > nit: No newline. Also, maybe just "Returns whether a network request was > actually sent."? The timely calling of RefreshSkipped is indeed a pretty difficult issue. It seems to require much more callbacks than the (offline discussed) rework: https://codereview.chromium.org/2838833005
The UI should now always get a fastest possible notification after having called getCurrentLogo(). No additional notifications and no late notifications. As discussed, we now only call java_onLogoAvailable after a UI-triggered getCurrentLogo in these cases: a) We have a cached logo. b) We just received a new logo. c) There is no chance that the "no logo" state changes. https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java (right): https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:69: public interface RefreshCallbackWrapper { On 2017/04/26 10:17:16, Marc Treib wrote: > Just RefreshCallback, or RefreshStatusCallback or something? It doesn't wrap > anything, right? Gone. https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:55: mShouldRecordLoadTime = true; On 2017/04/26 10:17:16, Marc Treib wrote: > I think the initialization can happen inline with the declaration (though not > sure if that's the preferred style in Java?) It's discouraged (especially for non-final fields): https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.... https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:99: if (mShouldRecordLoadTime && !mHasSkippedFirstDoodleRefresh) { On 2017/04/26 10:17:16, Marc Treib wrote: > Hm. Won't onRefreshSkipped() get called before onLogoAvailable(cached_logo)? If > so, then we'll never record a time for cached logos now. Gone. https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:112: LogoBridge.RefreshCallbackWrapper refreshCallbackWrapper = On 2017/04/26 10:17:16, Marc Treib wrote: > Also here: No wrapping involved Gone. https://codereview.chromium.org/2833473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:117: mHasSkippedFirstDoodleRefresh = true; On 2017/04/26 10:17:16, Marc Treib wrote: > nit: I'd remove the "First" from the name, since it'll get set for any non-first > skipped refreshes too. Gone. https://codereview.chromium.org/2833473002/diff/100001/chrome/browser/android... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/2833473002/diff/100001/chrome/browser/android... chrome/browser/android/logo_bridge.cc:232: DoodleConfigReceived(doodle_service_->config(), /*from_cache=*/true); On 2017/04/26 10:17:16, Marc Treib wrote: > Ah, here's the problem I'm talking about above, and I see that the comment here > is misleading: This won't usually call onLogoAvailable immediately, since it > first has to fetch the actual image. OnLogoAvailable is now exactly called when a) we have a config (called with doodle). b) we will not receive a config (called with null). https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... components/doodle/doodle_service.h:62: // Returns false if no fetch was issued because the service has fresh data in On 2017/04/26 10:17:16, Marc Treib wrote: > nit: No newline. Hm? There is no unnecessary newline, or am I missing something? > Also, maybe just "Returns whether a network request was > actually sent."? "Returns true when a network request was actually sent."? I want to be explicit about when it's true.
fine with me if we properly document the craziness happening. https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... chrome/browser/android/logo_bridge.h:42: // it to the observer. Can you better describe the details of this method. Under which situations will the passed observer be called? Given the complexity (including of the observer we pass in), it would be really good to properly explain those contracts.
Thanks! Looking good now; some last comments. https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... components/doodle/doodle_service.h:62: // Returns false if no fetch was issued because the service has fresh data in On 2017/04/27 10:31:44, fhorschig wrote: > On 2017/04/26 10:17:16, Marc Treib wrote: > > nit: No newline. > > Hm? There is no unnecessary newline, or am I missing something? I meant that there's a line break in the comment that seems unnecessary to me. Not a big deal. > > Also, maybe just "Returns whether a network request was > > actually sent."? > > "Returns true when a network request was actually sent."? > I want to be explicit about when it's true. s/when/if/, otherwise okay. (I wanted to cover both cases, but this is fine.) https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... chrome/browser/android/logo_bridge.cc:261: NotifyNoLogoAvailable(/*from_cache=*/false) return; Missing a ";" and newline before "return". https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... chrome/browser/android/logo_bridge.h:42: // it to the observer. On 2017/04/27 10:59:32, tschumann wrote: > Can you better describe the details of this method. Under which situations will > the passed observer be called? > > Given the complexity (including of the observer we pass in), it would be really > good to properly explain those contracts. Indeed, this needs a better comment. Since it's orthogonal to this CL, you can just put a TODO(treib) there for now.
https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/100001/components/doodle/dood... components/doodle/doodle_service.h:62: // Returns false if no fetch was issued because the service has fresh data in On 2017/04/27 11:15:50, Marc Treib wrote: > On 2017/04/27 10:31:44, fhorschig wrote: > > On 2017/04/26 10:17:16, Marc Treib wrote: > > > nit: No newline. > > > > Hm? There is no unnecessary newline, or am I missing something? > > I meant that there's a line break in the comment that seems unnecessary to me. > Not a big deal. > > > > Also, maybe just "Returns whether a network request was > > > actually sent."? > > > > "Returns true when a network request was actually sent."? > > I want to be explicit about when it's true. > > s/when/if/, otherwise okay. (I wanted to cover both cases, but this is fine.) Done. https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... chrome/browser/android/logo_bridge.cc:261: NotifyNoLogoAvailable(/*from_cache=*/false) return; On 2017/04/27 11:15:50, Marc Treib wrote: > Missing a ";" and newline before "return". Added. https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/120001/chrome/browser/android... chrome/browser/android/logo_bridge.h:42: // it to the observer. On 2017/04/27 11:15:50, Marc Treib wrote: > On 2017/04/27 10:59:32, tschumann wrote: > > Can you better describe the details of this method. Under which situations > will > > the passed observer be called? > > > > Given the complexity (including of the observer we pass in), it would be > really > > good to properly explain those contracts. > > Indeed, this needs a better comment. Since it's orthogonal to this CL, you can > just put a TODO(treib) there for now. Added that and a few lines.
last nit https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:104: // If we haven't received a logo on demand, don't wait for random refreshes. nit: this comment implies that the first call of this function is always connected with the on-demand fetch. Is this guaranteed? If so, we should also document this at GetCurrentLogo(). If not, we should write a bit more here why this (usually?) works.
fhorschig@chromium.org changed reviewers: + bauerb@chromium.org
Hi Berhard, would you please take a look at this fix for the incorrectly recorded NTP.LogoShownTime metric? (Especially at the LogoDelegateImpl.java and the logo_bridge.*)
https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:104: // If we haven't received a logo on demand, don't wait for random refreshes. On 2017/04/27 11:52:08, tschumann wrote: > nit: this comment implies that the first call of this function is always > connected with the on-demand fetch. > > Is this guaranteed? If so, we should also document this at GetCurrentLogo(). If > not, we should write a bit more here why this (usually?) works. It is guaranteed. Added that.
lgtm Thanks! https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:104: // If we haven't received a logo on demand, don't wait for random refreshes. On 2017/04/27 11:52:08, tschumann wrote: > nit: this comment implies that the first call of this function is always > connected with the on-demand fetch. > > Is this guaranteed? If so, we should also document this at GetCurrentLogo(). If > not, we should write a bit more here why this (usually?) works. It's not really guaranteed, but it doesn't really matter. How about: // If there currently is no Doodle, don't record the time if a refresh happens later. https://codereview.chromium.org/2833473002/diff/140001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/browser/android... chrome/browser/android/logo_bridge.h:48: // is called with an empty object. s/empty/null/ ?
Thanks! https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:104: // If we haven't received a logo on demand, don't wait for random refreshes. On 2017/04/27 11:56:09, Marc Treib wrote: > On 2017/04/27 11:52:08, tschumann wrote: > > nit: this comment implies that the first call of this function is always > > connected with the on-demand fetch. > > > > Is this guaranteed? If so, we should also document this at GetCurrentLogo(). > If > > not, we should write a bit more here why this (usually?) works. > > It's not really guaranteed, but it doesn't really matter. How about: > // If there currently is no Doodle, don't record the time if a refresh happens > later. Done. (Sorry, I updated the wrong comment before. Here it is not guaranteed, true.) https://codereview.chromium.org/2833473002/diff/140001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/browser/android... chrome/browser/android/logo_bridge.h:48: // is called with an empty object. On 2017/04/27 11:56:09, Marc Treib wrote: > s/empty/null/ ? Done.
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.
Patchset #10 (id:200001) has been deleted
Hey Marc, As you suggested, I integrated your draft CL [1] into mine to fix the missing notification for validated, non-existent DoodleConfigs. Would you please have another look as behavior and tests have changed quite significantly? [1] https://codereview.chromium.org/2843403003/
https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... chrome/browser/android/logo_bridge.cc:229: // Immediately hand out any current cached config. nit: I'd remove the "Immediately", since it has already confused us - this does *not* mean the Java callback will get called immediately. https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... chrome/browser/android/logo_bridge.h:47: // c) The current doodle was revalidated.. The comment above is now slightly misleading: This can't be the "once" call, because if there is a current doodle, then a) will have happened before. https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... components/doodle/doodle_service.cc:206: } else if (!expired) { Why the "if (!expired)"? This seems like an edge case where we'd end up not calling the UI again? https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... components/doodle/doodle_service.h:62: // config changes or active configs remain valid. s/changes/changed/ s/active configs remain/the active config remains/
https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... chrome/browser/android/logo_bridge.cc:229: // Immediately hand out any current cached config. On 2017/04/27 15:22:55, Marc Treib wrote: > nit: I'd remove the "Immediately", since it has already confused us - this does > *not* mean the Java callback will get called immediately. Done. https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/2833473002/diff/220001/chrome/browser/android... chrome/browser/android/logo_bridge.h:47: // c) The current doodle was revalidated.. On 2017/04/27 15:22:55, Marc Treib wrote: > The comment above is now slightly misleading: This can't be the "once" call, > because if there is a current doodle, then a) will have happened before. Gone. Correct, it's not relevant in this context. https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... components/doodle/doodle_service.cc:206: } else if (!expired) { On 2017/04/27 15:22:55, Marc Treib wrote: > Why the "if (!expired)"? This seems like an edge case where we'd end up not > calling the UI again? Gone. We previously checked in the test DisregardsAlreadyExpiredConfigs that no observer should be called. But having no config because the most recent config is expired can be treated as revalidation. This is in line with what this event means --> Test expectations changed. https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2833473002/diff/220001/components/doodle/dood... components/doodle/doodle_service.h:62: // config changes or active configs remain valid. On 2017/04/27 15:22:55, Marc Treib wrote: > s/changes/changed/ > s/active configs remain/the active config remains/ Done.
LGTM with one more nit. Thanks for sticking with this! https://codereview.chromium.org/2833473002/diff/240001/components/doodle/dood... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2833473002/diff/240001/components/doodle/dood... components/doodle/doodle_service_unittest.cc:396: EXPECT_CALL(observer, OnDoodleConfigRevalidated(Eq(/*from_cache=*/false))); I like to have EXPECT_CALLs as close as possible to the thing that actually triggers them. Can we move this to just before ServeAllCallbacks?
Thanks! https://codereview.chromium.org/2833473002/diff/240001/components/doodle/dood... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2833473002/diff/240001/components/doodle/dood... components/doodle/doodle_service_unittest.cc:396: EXPECT_CALL(observer, OnDoodleConfigRevalidated(Eq(/*from_cache=*/false))); On 2017/04/28 08:22:59, Marc Treib wrote: > I like to have EXPECT_CALLs as close as possible to the thing that actually > triggers them. Can we move this to just before ServeAllCallbacks? Sure.
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:41: private boolean mShouldRecordLoadTime; Initialize this here?
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2833473002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:41: private boolean mShouldRecordLoadTime; On 2017/04/28 10:13:28, Bernhard Bauer wrote: > Initialize this here? Done. Just out of curiosity: Shouldn't the code style discussion [1] apply here? I would understand if the class was final (which seems reasonable, btw). [1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....
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.
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, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2833473002/#ps280001 (title: "Fix compile error")
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": 280001, "attempt_start_ts": 1493387563239170,
"parent_rev": "8041ea0d943535dcbb475fe0bf9d0df0c96377ef", "commit_rev":
"02ed0d5fe2949096abd3215af102cf221144da5d"}
Message was sent while issue was closed.
Description was changed from ========== Record NTP.LogoShownTime for timely refreshs only This CL corrects the load time metrics for Logos on the NTP. The old behavior: ----------------- If the doodle cache is empty, the LogoShownTime will be recorded when the next DoodleService#Refresh triggers the observer who requested a Refresh. If the requested refresh is skipped, the observer records the time when the next notification happens due to a refresh triggered by a different caller. This can be timely but is much more likely to be 15 min or more in the future. This delay blurs the metric. The wanted/new behavior: ------------------------ This CL introduces a way to know that the refresh was skipped. The observer that records LogoShownTime uses it to prevent recording any metrics for this case. BUG=713166 ========== to ========== Record NTP.LogoShownTime for timely refreshs only This CL corrects the load time metrics for Logos on the NTP. The old behavior: ----------------- If the doodle cache is empty, the LogoShownTime will be recorded when the next DoodleService#Refresh triggers the observer who requested a Refresh. If the requested refresh is skipped, the observer records the time when the next notification happens due to a refresh triggered by a different caller. This can be timely but is much more likely to be 15 min or more in the future. This delay blurs the metric. The wanted/new behavior: ------------------------ This CL introduces a way to know that the refresh was skipped. The observer that records LogoShownTime uses it to prevent recording any metrics for this case. BUG=713166 Review-Url: https://codereview.chromium.org/2833473002 Cr-Commit-Position: refs/heads/master@{#467989} Committed: https://chromium.googlesource.com/chromium/src/+/02ed0d5fe2949096abd3215af102... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as https://chromium.googlesource.com/chromium/src/+/02ed0d5fe2949096abd3215af102... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
