Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1686643002:
Add metric with time between background tab being foregrounded and the first paint (Closed)
|
Created:
4 years, 10 months ago by pkotwicz Modified:
4 years, 10 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metric with time between background tab being foregrounded and the first paint
BUG=585229
Committed: https://crrev.com/780523e0eecff2a39c4428ea240c7551b9c0c7a2
Cr-Commit-Position: refs/heads/master@{#377975}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Total comments: 5
Patch Set 6 : #
Total comments: 13
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 51 (14 generated)
pkotwicz@chromium.org changed reviewers: + yfriedman@chromium.org
Yaron, can you take an initial look before I send the CL over to OWNERS?
According to UMA stats, PageLoad.Timing2.NavigationToFirstBackground <= 10ms: 33% of the time on M48 ~0% of the time on M49 I cannot explain the M48 number and am concerned that something similar will occur once M49 goes to stable. I hope that a bug fix caused the difference between M48 and M49
On 2016/02/09 19:48:08, pkotwicz wrote: > According to UMA stats, PageLoad.Timing2.NavigationToFirstBackground <= 10ms: > 33% of the time on M48 > ~0% of the time on M49 > > I cannot explain the M48 number and am concerned that something similar will > occur once M49 goes to stable. I hope that a bug fix caused the difference > between M48 and M49 The M48 background stats were due to https://code.google.com/p/chromium/issues/detail?id=552748 This was fixed in M49.
Does this account for cases where we kick off a background load and then the background tab gets evicted? If so, then we'd anticipate a histogram with two peaks - one centered around warm tabs that are mostly loaded and one which requires reparsing probably fetching? Regardless, I'm really not familiar with this code but if the answer to my above question is "yes", then I think this captures what we want. https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:131: if (info.started_in_foreground && I'm curious why this is necessary. Also, is it possible to have navigations that were started purely in background? Perhaps by a Service worker but then likely not a mainframe load?
Yaron, can you please take another look? https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:131: if (info.started_in_foreground && This is necessary because I changed when PageLoadExtraInfo::first_background_time is non-zero. Without my CL, PageLoadExtraInfo::first_background_time is only non-zero if the page starts in the foreground. With my CL, PageLoadExtraInfo::first_background_time can be non-zero both if the page starts in the foreground and if the page starts in the background. This change is necessary so that we record the internal::kHistogramBackgroundBeforePaint in the same cases with my CL as without my CL I changed the behavior of PageLoadExtraInfo::first_background_time/PageLoadExtraInfo::first_foreground_time to be able to detect: - A page starting in the background - Moving to the foreground - Not moving back to the background
No this does not account for cases where we kick off a background load and then the background tab gets evicted. We know how often background tabs get evicted prior to being shown for the first time from Tab.BackgroundLoadStatus. I suspect that the following are about the same: - The time between a frozen tab starting loading (because it was foregrounded) and the first paint - The time between a foreground tab starting loading and the first paint
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
I'm supportive of this change. Could you make sure EventOccurredInForeground and other consumers of the metrics you're changing will be unaffected? Suggestion: Not sure how much better this will make things, but it could be nice to have other utility functions like bool PageLoadedForegroundToBackground(info) bool LoadedBackgroundToForeground(info) Some of these conditionals are getting (and already are) pretty nasty. https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:202: timing.first_paint - info.first_foreground_time); Careful, first_paint will be 0 for pages that didn't reach that point in the load. Your histogram will have lots values clamped to 0ms. See EventOccurredInForeground.
PageLoadedForegroundToBackground(info) is shorthand for "info.started_in_foreground && !info.first_background_time.is_zero()" I do not think that PageLoadedForegroundToBackground(info) is very useful because the method has a long name and does not replace a lot of code. https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:202: timing.first_paint - info.first_foreground_time); I am confused. This if statement is nested inside the if statement on line 189 https://codereview.chromium.org/1686643002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:199: timing.first_paint < info.first_background_time)) { This if() statement is nasty. I was thinking of moving this logic to a function page_load_metrics::LoadStartedInBackgroundEventOccurredInForeground(event, info) However, this would require renaming page_load_metrics::EventOccurredInForeground() to page_load_metrics::LoadStartedInForegroundEventOccurredInForeground() which I do not think we want to do.
https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:202: timing.first_paint - info.first_foreground_time); On 2016/02/11 16:49:09, pkotwicz wrote: > I am confused. This if statement is nested inside the if statement on line 189 Sorry, my mistake. https://codereview.chromium.org/1686643002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:199: timing.first_paint < info.first_background_time)) { On 2016/02/11 16:49:09, pkotwicz wrote: > This if() statement is nasty. I was thinking of moving this logic to a function > page_load_metrics::LoadStartedInBackgroundEventOccurredInForeground(event, info) > > However, this would require renaming > page_load_metrics::EventOccurredInForeground() to > page_load_metrics::LoadStartedInForegroundEventOccurredInForeground() which I do > not think we want to do. I totally understand the frustration. Possibly we could change to page_load_metrics::StartInBackgroundEventInForeground page_load_metrics::StartInForegroundEventInForeground I don't think that's too too unreadable. You could also use a temporary bool.
On 2016/02/10 23:30:47, pkotwicz wrote: > No this does not account for cases where we kick off a background load and then > the > background tab gets evicted. We know how often background tabs get evicted prior > to being shown for the first time from Tab.BackgroundLoadStatus. > > I suspect that the following are about the same: > - The time between a frozen tab starting loading (because it was foregrounded) > and the first paint > - The time between a foreground tab starting loading and the first paint agreed. I guess what your'e saying is that if were to keep more background tabs alive in the open in new tab case, we won't really measure the user perceived change in load time directly but can extrapolate based on shifting in the other metric?
https://codereview.chromium.org/1686643002/diff/40001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/40001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:25: // page never moved to the background. a little confusing because of this, but would rather not make this even more verbose :) https://codereview.chromium.org/1686643002/diff/40001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:41: const PageLoadExtraInfo& info) { nit: indent
Yaron, can you please take another look? My intent with this metric is to figure out whether serializing page loads would be useful. The idea would be to delay new background tab loads till the previous background tab loads are completed. This will result in having few fully loaded tabs as opposed to many partially loaded tabs. Serializing page loads is only a viable option if the users are most likely to navigate to the oldest background tab as opposed to the newest background tab. (My suspicion is that users are in fact most likely to navigate to the oldest background tab)
Yaron, Ping!
conceptually lgtm but csharrion's review is what really matters as I'm not familiar enough with this code
csharrison@ can you please take another look?
Sorry, taking a look now.
csharrison@chromium.org changed reviewers: + kinuko@chromium.org
Looking good. Adding kinuko@ for OWNERS and because she has some knowledge of this background foreground stuff from previous reviews. As is, I think this new metric will only really be valuable in conjunction with a tab serialization experiment. Hope your hunch is right. I'm ccing Randy who is working on resource prioritization. He'll definitely be interested in this work. https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: !StartInForegroundEventInForeground(timing.first_paint, info)) { At this point I think this is less clear than started_in_foreground && !first_background_time.is_zero() && first_background_time < first_paint https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:243: if (info.started_in_foreground) { Thanks for adding cleaning this up. https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:17: // the page was always in the foreground. Nit: Not quite true. This function will return true for a load that starts in the foreground and immediately backgrounds. I would amend the comment to say "True if the page was started in the foreground." https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:18: bool MovedToForegroundBefore(const base::TimeDelta& event, I think this function could also be renamed to something more clear, like "InForegroundBeforeEvent." In this case, the transition is not as important as the background case, because it captures the simpler notion of "was the page ever in the foreground before the event occurred." https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:26: bool MovedToBackgroundAfter(const base::TimeDelta& event, This is a confusing name given the comment and what it actually represents. Naming these things is really tricky though. It seems like this is really trying to express "The event occurred before any background transition," not "the page moved to the background after the event." I'd suggest renaming this to more closely fit the former. https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:40: bool StartInBackgroundEventInForeground(const base::TimeDelta& event, nit: do you mind replacing all "const base::TimeDelta&"s you touch with raw "base::TimeDelta"s? The time classes are all POD and should not be passed by const reference. This was a mistake we made earlier.
csharrison@ can you please take another look? The reason that *I THINK* that users are most likely to switch to the oldest background rather than the newest background tab is that it is the easiest background tab to switch to. - The oldest background tab is topmost in the tab switcher. - Swiping right on the toolbar switches to the oldest background tab. https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:17: // the page was always in the foreground. I created a function InForeground() and made both StartInForegroundEventInForeground() and StartInBackgroundEventInForeground() call it. I like the new InForeground() function because its return value is very clear. https://codereview.chromium.org/1686643002/diff/60001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:40: bool StartInBackgroundEventInForeground(const base::TimeDelta& event, On 2016/02/22 16:08:32, csharrison wrote: > nit: do you mind replacing all "const base::TimeDelta&"s you touch with raw > "base::TimeDelta"s? The time classes are all POD and should not be passed by > const reference. This was a mistake we made earlier. Done. https://codereview.chromium.org/1686643002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: timing.first_paint > info.first_background_time)) { Expanded out the StartInForegroundEventInForeground() call. Note that the if statement is run if timing.first_paint.is_zero()
lgtm with nit. I like how simple the two exposed util methods became. https://codereview.chromium.org/1686643002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/80001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:19: bool InForeground(base::TimeDelta event, const PageLoadExtraInfo& info) { nit: Why not "EventInForeground" for symmetry? I like this though.
The logic looks good, I have a bit more bike-shedding comment though (sorry) https://codereview.chromium.org/1686643002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: timing.first_paint > info.first_background_time)) { Can we add some comment here, these conditional statements are not easy to interpret quickly. // Load started in foreground but the first paint occured after the tab was moved to background. https://codereview.chromium.org/1686643002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1686643002/diff/80001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.h:28: bool StartInForegroundEventInForeground(base::TimeDelta event, I understand what these methods mean after reading this comment, but these still feel a bit hard to parse-- maybe partly because this could be read as a function that starts something in foreground at a first look. Actually, I wonder how bad it is if we just define EventOccuredInForeground(event, info) helper and do write if (info.started_in_foreground && EventOccuredInForeground(event, info)) if (!info.started_in_foreground && EventOccuredInForeground(event, info)) ..?
Patchset #6 (id:100001) has been deleted
kinuko@ can you please take another look? https://codereview.chromium.org/1686643002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1686643002/diff/80001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.h:28: bool StartInForegroundEventInForeground(base::TimeDelta event, StartInForegroundEventInForeground() represents an important concept during metrics recording so I want to keep it as one function. I have renamed the function to WasStartedInForegroundEventInForeground() to make it clear that the function does not start anything.
> I have renamed the function to WasStartedInForegroundEventInForeground() to make > it clear that the function does not start anything. Thanks. Naming is hard, but the new ones work for me. lgtm
isherman@ for tools/metrics/histograms This is my last metrics related CL for this feature :)
Description was changed from ========== Add metric with time between background tab being foregrounded and the first paint BUG=585229 ========== to ========== Add metric with time between background tab being foregrounded and the first paint BUG=585229 ==========
pkotwicz@chromium.org changed reviewers: + isherman@chromium.org
isherman@ for tools/metrics/histograms This is my last metrics related CL for this feature :)
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
not lgtm https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: !StartInForegroundEventInForeground(timing.first_paint, info)) { On 2016/02/22 16:08:32, csharrison wrote: > At this point I think this is less clear than > started_in_foreground && > !first_background_time.is_zero() && > first_background_time < first_paint agree - this function is trying to do too much and is thus quite hard to reason about https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:19: bool InForeground(base::TimeDelta event, const PageLoadExtraInfo& info) { i'm not super keen on this change. I think you're trying to do too much in these functions. given that you're adding one new UMA, and the WasStartedInBackgroundEventInForeground is used exactly once, could we please inline the logic for that function at the one call site and leave the existing logic as is? if we find a need to use WasStartedInBackgroundEventInForeground more than once we can see about factoring it into common code at that time.
On 2016/02/24 19:34:44, Bryan McQuade wrote: > not lgtm > > https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/1686643002/diff/60001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: > !StartInForegroundEventInForeground(timing.first_paint, info)) { > On 2016/02/22 16:08:32, csharrison wrote: > > At this point I think this is less clear than > > started_in_foreground && > > !first_background_time.is_zero() && > > first_background_time < first_paint > > agree - this function is trying to do too much and is thus quite hard to reason > about > > https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... > File components/page_load_metrics/browser/page_load_metrics_util.cc (right): > > https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... > components/page_load_metrics/browser/page_load_metrics_util.cc:19: bool > InForeground(base::TimeDelta event, const PageLoadExtraInfo& info) { > i'm not super keen on this change. I think you're trying to do too much in these > functions. > > given that you're adding one new UMA, and the > WasStartedInBackgroundEventInForeground is used exactly once, could we please > inline the logic for that function at the one call site and leave the existing > logic as is? > > if we find a need to use WasStartedInBackgroundEventInForeground more than once > we can see about factoring it into common code at that time. let me take a few more minutes to look at this first. will re-review.
https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:92: "PageLoad.Timing2.SwitchToForegroundToFirstPaint"; can we shorten to PageLoad.Timing2.ForegroundToFirstPaint? the 'switchto' part isn't consistent with the naming for any other metrics here, so i'd like to remove it. https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: if (info.started_in_foreground && !info.first_background_time.is_zero() && thanks for this. good fix. https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:200: if (WasStartedInBackgroundEventInForeground(timing.first_paint, info)) { just to clarify your goal here, if the page does the following: * start in background * move to foreground * move to background * move to foreground * first paint do you want to log your metric in this case? i don't think we can easily support that case, but i want to make sure you were aware that it won't be covered by the current impl. https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:248: if (info.started_in_foreground) { likewise, thanks for this https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:229: if (!background_time_.is_null()) I see that you need these changes, but I worry that some code may depend on first bg time only being set for pages that start in the fg, and the other way around. It looks like you fixed some of those cases but I worry there may be others. I will do a second sweep through the code to make sure we didn't miss anything. https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:40: bool WasStartedInBackgroundEventInForeground(base::TimeDelta event, So, this is going to be disappointing, but I'd like to reduce the amount of code being changed here. Unless you're fixing known bugs, I'd like to minimize the changes we make to this code, in order to avoid possible regressions in metrics. We just had a bug in our performance.timing metrics make it out to stable, so I'd like to be extra cautious and try to minimize changes in our metrics code. The old implementation of EventOccurredInForeground is substantially simpler than the new one you've added which delegates to the more complex InForeground, and given how frequently we use EventOccurredInForeground, I think simplicity is preferred. Could you restore the original implementation of WasStartedInForegroundEventInForeground as it was in EventOccurredInForeground, and implement the logic you need for WasStartedInBackgroundEventInForeground separately, without any shared logic? Let's keep the 2 fixes you made (the one for where we log kHistogramBackgroundBeforePaint, and the other where we log kHistogramFirstBackground). Thanks for those!
https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:92: "PageLoad.Timing2.SwitchToForegroundToFirstPaint"; Ok https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:133: if (info.started_in_foreground && !info.first_background_time.is_zero() && This is not a fix. Just keeping the logic the same as before https://codereview.chromium.org/1686643002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:200: if (WasStartedInBackgroundEventInForeground(timing.first_paint, info)) { I understand. I do not really want to record metrics for the scenario that you have described. https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:229: if (!background_time_.is_null()) I looked through all uses of |background_time_| and |foreground_time_| to make sure that I did not miss anything. However, I do appreciate a second set of eyes. https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:40: bool WasStartedInBackgroundEventInForeground(base::TimeDelta event, Can you discuss with csharrison@? Patch set #1 does not modify this file at all. I made the changes in page_load_metrics_util.cc based on csharrison@'s code review feedback (Comment #11)
metrics lgtm
https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1686643002/diff/120001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:40: bool WasStartedInBackgroundEventInForeground(base::TimeDelta event, On 2016/02/24 at 22:03:55, pkotwicz wrote: > Can you discuss with csharrison@? Patch set #1 does not modify this file at all. I made the changes in page_load_metrics_util.cc based on csharrison@'s code review feedback (Comment #11) I think I saw the comment you're referring to - Charles said: """ Possibly we could change to page_load_metrics::StartInBackgroundEventInForeground page_load_metrics::StartInForegroundEventInForeground I don't think that's too too unreadable. You could also use a temporary bool. """ this was in response to your comment """ this would require renaming > page_load_metrics::EventOccurredInForeground() to > page_load_metrics::LoadStartedInForegroundEventOccurredInForeground() """ I don't think that he was suggesting that you factor the implementation of EventOccurredInForeground into a common method that ends up being more complex than the original EventOccurredInForeground. I could be misunderstanding, but in any case I don't want the existing method to become any more complex unless it's to fix a bug. Let's please revert to the original implementation used on EventOccurredInForeground. I'm fine with renaming to StartInForegroundEventInForeground as your change does. I don't think the increased complexity of the implementation makes having shared code worthwhile, however. I'll take an AI to improve unit testing for these methods in a future CL so this is marginally less fragile as we go forward.
Patchset #8 (id:160001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:180001) has been deleted
bmcquade@ can you please take another look? I believe that I have addressed all of your concerns
bmcquade@ can you please take another look? I believe that I have addressed all of your concerns
Thanks. Your change looks good. I'm going to go through and double check existing uses of foreground/background time once more - will follow up later today. Thank you. https://codereview.chromium.org/1686643002/diff/200001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1686643002/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:163: background_time_ = base::TimeTicks::Now(); similar to below, can you add // Make sure we either started in the foreground and haven't been foregrounded yet, or started in the background and have already been foregrounded. DCHECK_EQ(started_in_foreground_, foreground_time_.is_null()); https://codereview.chromium.org/1686643002/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:169: foreground_time_ = base::TimeTicks::Now(); To make sure WebContentsShown and WebContentsHidden are called in the way we expect, I'd like to sanity check that the first time we get a WebContentsShown, we either started in the background and haven't been backgrounded yet, or started in the foreground and have already been backgrounded. To do this, can you add // Make sure we either started in the background and haven't been backgrounded yet, or started in the foreground and have already been backgrounded. DCHECK_NE(started_in_foreground_, background_time_.is_null());
LGTM, thanks for making this change.
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, csharrison@chromium.org, kinuko@chromium.org, isherman@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/1686643002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686643002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686643002/220001
Message was sent while issue was closed.
Description was changed from ========== Add metric with time between background tab being foregrounded and the first paint BUG=585229 ========== to ========== Add metric with time between background tab being foregrounded and the first paint BUG=585229 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add metric with time between background tab being foregrounded and the first paint BUG=585229 ========== to ========== Add metric with time between background tab being foregrounded and the first paint BUG=585229 Committed: https://crrev.com/780523e0eecff2a39c4428ea240c7551b9c0c7a2 Cr-Commit-Position: refs/heads/master@{#377975} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/780523e0eecff2a39c4428ea240c7551b9c0c7a2 Cr-Commit-Position: refs/heads/master@{#377975} |
