Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2152483002:
Force early creation of NTPUserDataLogger. (Closed)
|
Created:
4 years, 5 months ago by sfiera Modified:
4 years, 5 months ago Reviewers:
Marc Treib CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce early creation of NTPUserDataLogger.
Without this, it won't be created early enough during startup to detect
that startup is happening.
BUG=625990
Committed: https://crrev.com/c156d90aec850be4fe24b3f218ede84ea11a2c32
Cr-Commit-Position: refs/heads/master@{#405476}
Patch Set 1 #Patch Set 2 : #Messages
Total messages: 13 (3 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
Here seems to work. In the pathological case where the user runs with "pick up where I left off" and closes the browser with multiple NTPs open, then only one of them will be detected as being startup. This is maybe OK.
On 2016/07/13 16:54:43, sfiera wrote: > Here seems to work. > > In the pathological case where the user runs with "pick up where I left off" and > closes the browser with multiple NTPs open, then only one of them will be > detected as being startup. This is maybe OK. Why? But yes, this is probably okay. I find it a bit surprising that creating the logger *after* the page has finished loading is still early enough for the startup detection. But if it works... LGTM
On 2016/07/14 08:21:37, Marc Treib wrote: > On 2016/07/13 16:54:43, sfiera wrote: > > Here seems to work. > > > > In the pathological case where the user runs with "pick up where I left off" > and > > closes the browser with multiple NTPs open, then only one of them will be > > detected as being startup. This is maybe OK. > > Why? Specifically, it seems that only the first tab gets marked as Startup. I guess Chrome defines startup as having at least one window, and a window is defined as having at least one tab. > But yes, this is probably okay. > > I find it a bit surprising that creating the logger *after* the page has > finished loading is still early enough for the startup detection. But if it > works... It looks like startup is marked complete by an observer of the first DidFinishLoad() call, so I guess this CL is reliant on getting that DidFinishLoad() first.
On 2016/07/14 10:21:14, sfiera wrote: > On 2016/07/14 08:21:37, Marc Treib wrote: > > On 2016/07/13 16:54:43, sfiera wrote: > > > Here seems to work. > > > > > > In the pathological case where the user runs with "pick up where I left off" > > and > > > closes the browser with multiple NTPs open, then only one of them will be > > > detected as being startup. This is maybe OK. > > > > Why? > > Specifically, it seems that only the first tab gets marked as Startup. I guess > Chrome defines startup as having at least one window, and a window is defined as > having at least one tab. That means at least it's not a regression. Still not great, but okay. > > But yes, this is probably okay. > > > > I find it a bit surprising that creating the logger *after* the page has > > finished loading is still early enough for the startup detection. But if it > > works... > > It looks like startup is marked complete by an observer of the first > DidFinishLoad() call, so I guess this CL is reliant on getting that > DidFinishLoad() first. Hm, that sounds fragile. What about OnTabActivated? That should be called earlier, right?
Done. I thought I had tried that before and concluded it didn't work--it doesn't work unless the NTP is frontmost, but every solution has that problem. Plus, it's probably not useful to track "load time of an NTP in the background at startup" because the load time of an NTP that the user isn't looking at is not interesting.
On 2016/07/14 11:24:25, sfiera wrote: > Done. I thought I had tried that before and concluded it didn't work--it doesn't > work unless the NTP is frontmost, but every solution has that problem. So we'll still count the load, but not as a ".Startup" one? > Plus, it's probably not useful to track "load time of an NTP in the background > at startup" because the load time of an NTP that the user isn't looking at is > not interesting. Yeah, that makes sense. Kinda makes me wonder if we should differentiate "foreground" and "background" loads, but it's probably not worth the effort. LGTM
On 2016/07/14 11:27:25, Marc Treib wrote: > On 2016/07/14 11:24:25, sfiera wrote: > > Done. I thought I had tried that before and concluded it didn't work--it > doesn't > > work unless the NTP is frontmost, but every solution has that problem. > > So we'll still count the load, but not as a ".Startup" one? Yes. We get stats for all NTP loads, but it only counts as "Startup" if it is frontmost at startup. This is good for the Startup metric--the frontmost tab is what matters as far as user perception. Maybe it's bad for the NewTab metrics, since slower startup loads might pollute it, but probably not significantly. > > Plus, it's probably not useful to track "load time of an NTP in the background > > at startup" because the load time of an NTP that the user isn't looking at is > > not interesting. > > Yeah, that makes sense. Kinda makes me wonder if we should differentiate > "foreground" and "background" loads, but it's probably not worth the effort. > > LGTM When can a background NTP load happen other than startup? I guess maybe Javascript can triggger window.history.back(). Neither case is probably worth breaking out.
The CQ bit was checked by sfiera@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Force early creation of NTPUserDataLogger. Without this, it won't be created early enough during startup to detect that startup is happening. BUG=625990 ========== to ========== Force early creation of NTPUserDataLogger. Without this, it won't be created early enough during startup to detect that startup is happening. BUG=625990 Committed: https://crrev.com/c156d90aec850be4fe24b3f218ede84ea11a2c32 Cr-Commit-Position: refs/heads/master@{#405476} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c156d90aec850be4fe24b3f218ede84ea11a2c32 Cr-Commit-Position: refs/heads/master@{#405476} |
