Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2423383002:
[Prerender] first contentful paint histograms. (Closed)
|
Created:
4 years, 2 months ago by mattcary Modified:
3 years, 11 months ago CC:
ajuma+watch-canvas_chromium.org, blink-reviews, blink-reviews-html_chromium.org, blink-reviews-paint_chromium.org, Rik, cbentzel+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, dglazkov+blink, dshwang, gavinp+prer_chromium.org, Justin Novosad, loading-reviews+metrics_chromium.org, tburkard+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Prerender] Add first contentful paint histograms
Uses perceived FCP measured from prerender swap time when appropriate.
Changes no state prefetch to use the same FCP histogram for easier
experimentation. If a page is hidden during prerendering, the paint
may be delayed in an unpredictable manner; therefore the histograms are
split between hidden and visible.
BUG=657762, 667243
Committed: https://crrev.com/f564b5a80caa74b761951b2d9a6772eb80c473b0
Cr-Commit-Position: refs/heads/master@{#441056}
Patch Set 1 #Patch Set 2 : enable prerender in page load metrics and tighten tab helper timing recording #Patch Set 3 : Wait on FCP; general cleanup #Patch Set 4 : Browser tests and related bug fixes #Patch Set 5 : cleanup #
Total comments: 26
Patch Set 6 : comments #
Total comments: 12
Patch Set 7 : comment comment #Patch Set 8 : comments: hidden and nav start #Patch Set 9 : Generalize internal histogram function name #
Total comments: 23
Patch Set 10 : remove debug code #
Total comments: 7
Patch Set 11 : comments #Patch Set 12 : comments, clock tweak #Patch Set 13 : comments #
Total comments: 6
Patch Set 14 : metrics test #Patch Set 15 : Unify with nostate histograms #
Total comments: 17
Patch Set 16 : comments #
Total comments: 21
Patch Set 17 : comments #
Total comments: 6
Patch Set 18 : comments #
Total comments: 44
Patch Set 19 : comments #
Total comments: 4
Patch Set 20 : comments #
Total comments: 4
Patch Set 21 : comments #Patch Set 22 : non-prerender page load metric check #
Total comments: 3
Patch Set 23 : clarify page load metric test #
Total comments: 8
Patch Set 24 : histogram comments #Patch Set 25 : rebase #Patch Set 26 : MSVC compilation error #Dependent Patchsets: Messages
Total messages: 111 (26 generated)
Description was changed from ========== Perceived FCP for prerender. BUG= ========== to ========== Perceived FCP for prerender. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
mattcary@chromium.org changed reviewers: + pasko@chromium.org
The CL is not finished (in particular there's a race with waiting for the FCP to happen that I'm currently using a sleep for). I wanted to get your take on if I'm using reasonable timing and if the PageLoadMetricsEmbedder changes to enabling tracking prerendering metrics are the right approach.
mattcary@chromium.org changed reviewers: + droger@chromium.org
+droger for general histogram advice.
Description was changed from ========== Perceived FCP for prerender. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Perceived FCP for prerender. BUG=657762 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Perceived FCP for prerender. BUG=657762 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Perceived FCP for prerender. BUG=657762 ==========
PageLoadMetricsEmbedder look good to me, but I am not familiar with this part at all, let's ask OWNERs to carefully inspect changes there. overall the approach looks good as well I am getting increasingly worried that our delta logic may be inappropriate for some edge cases. It would be important to try to catch these with a few browsertests. I am thinking of these cases, but you are welcome to add more: 1. prerender gets stopped and reused; verify that the PerceivedTTFCP is not starting from the 1st prerender request 2. prerender1 for the URL, gets discarded, prerender2 for the same URL in a new process; 3. prerender1 starts, but SwapInInternal happens before navigation commit, so the prerender is not used, regular navigation continues 4. maybe different tests for browser- and renderer-initiated navigations in 3 WDYT?
On 2016/10/20 13:23:15, pasko wrote: > PageLoadMetricsEmbedder look good to me, but I am not familiar with this part at > all, let's ask OWNERs to carefully inspect changes there. > > overall the approach looks good as well > > > I am getting increasingly worried that our delta logic may be inappropriate for > some edge cases. It would be important to try to catch these with a few > browsertests. I am thinking of these cases, but you are welcome to add more: > 1. prerender gets stopped and reused; verify that the PerceivedTTFCP is not > starting from the 1st prerender request > 2. prerender1 for the URL, gets discarded, prerender2 for the same URL in a new > process; > 3. prerender1 starts, but SwapInInternal happens before navigation commit, so > the prerender is not used, regular navigation continues > 4. maybe different tests for browser- and renderer-initiated navigations in 3 > > WDYT? Those are good points. However, it will be a little tricky to test what you have in mind, I think, because in most of those cases there will be a histogram entry recorded, but it will not be clear if it is recording the right time (all the page load metrics tests I see only look for a histogram being recorded, with no check of its value). So any ideas you have will be welcome. At any rate, I'll start investigating.
On 2016/10/20 13:29:54, mattcary wrote: > On 2016/10/20 13:23:15, pasko wrote: > > PageLoadMetricsEmbedder look good to me, but I am not familiar with this part > at > > all, let's ask OWNERs to carefully inspect changes there. > > > > overall the approach looks good as well > > > > > > I am getting increasingly worried that our delta logic may be inappropriate > for > > some edge cases. It would be important to try to catch these with a few > > browsertests. I am thinking of these cases, but you are welcome to add more: > > 1. prerender gets stopped and reused; verify that the PerceivedTTFCP is not > > starting from the 1st prerender request > > 2. prerender1 for the URL, gets discarded, prerender2 for the same URL in a > new > > process; > > 3. prerender1 starts, but SwapInInternal happens before navigation commit, so > > the prerender is not used, regular navigation continues > > 4. maybe different tests for browser- and renderer-initiated navigations in 3 > > > > WDYT? > > Those are good points. However, it will be a little tricky to test what you have > in mind, I think, because in most of those cases there will be a histogram entry > recorded, but it will not be clear if it is recording the right time (all the > page load metrics tests I see only look for a histogram being recorded, with no > check of its value). Good to know about the nonexistence of histogram tests that'd verify the numbers. It seems that we would need to mock out histogram recording functions for that. Also, there are some limitations wrt how many prerenders a single test can toss at a time. May be complicated indeed. > So any ideas you have will be welcome. At any rate, I'll start investigating.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
FYI we do have histogram unit tests that verify numbers: e.g. https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...
Updated with browser tests. Doing that actually found a bug! Turns out the prerender tab helper was resetting the origin and navigation type after the page load completed, which doesn't work as the first contentful paint may or may not occur. Hopefully my fix is correct. Notes on the tests: * I think I got your tests 1-3. I'm not sure how to do browser initiated navigation; the omnibox browser tests are all disabled so I suspect they're flakey or something. Anyone know anything? * As testing prerender requires quite a bit of test machinery, the tests are over in the prerender browser tests instead of the page load metrics test. I hope that's okay. * I spent a couple of days trying to get page load metric prerender unittests, but as perhaps should be obvious, that failed.
> Notes on the tests: > * I think I got your tests 1-3. I'm not sure how to do browser initiated > navigation; the omnibox browser tests are all disabled so I suspect they're > flakey or something. Anyone know anything? > Seems to be crbug/294592. I don't know if the long command line is omnibox specific, but it seems other omnibox tests unrelated to prerender were also disabled in the same CL (c4c0c0a16).
The CQ bit was checked by mattcary@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_...)
On 2016/11/16 09:38:37, mattcary wrote: > > Notes on the tests: > > * I think I got your tests 1-3. I'm not sure how to do browser initiated > > navigation; the omnibox browser tests are all disabled so I suspect they're > > flakey or something. Anyone know anything? > > > Seems to be crbug/294592. I don't know if the long command line is omnibox > specific, but it seems other omnibox tests unrelated to prerender were also > disabled in the same CL (c4c0c0a16). I did not know the status of omnibox prerender tests until now. One test was re-enabled after the massive disable, should we re-enable other Omnibox tests and see what happens? Thanks for the new tests!! I'll take a look shortly. Any other significant changes to look at?
The change to the tab helper is the most interesting, I think. The logic there is "subtle". On Nov 16, 2016 3:45 PM, <pasko@chromium.org> wrote: > On 2016/11/16 09:38:37, mattcary wrote: > > > Notes on the tests: > > > * I think I got your tests 1-3. I'm not sure how to do browser > initiated > > > navigation; the omnibox browser tests are all disabled so I suspect > they're > > > flakey or something. Anyone know anything? > > > > > Seems to be crbug/294592. I don't know if the long command line is > omnibox > > specific, but it seems other omnibox tests unrelated to prerender were > also > > disabled in the same CL (c4c0c0a16). > > I did not know the status of omnibox prerender tests until now. One test > was > re-enabled after the massive disable, should we re-enable other Omnibox > tests > and see what happens? > > Thanks for the new tests!! I'll take a look shortly. Any other significant > changes to look at? > > https://codereview.chromium.org/2423383002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
The change to the tab helper is the most interesting, I think. The logic there is "subtle". On Nov 16, 2016 3:45 PM, <pasko@chromium.org> wrote: > On 2016/11/16 09:38:37, mattcary wrote: > > > Notes on the tests: > > > * I think I got your tests 1-3. I'm not sure how to do browser > initiated > > > navigation; the omnibox browser tests are all disabled so I suspect > they're > > > flakey or something. Anyone know anything? > > > > > Seems to be crbug/294592. I don't know if the long command line is > omnibox > > specific, but it seems other omnibox tests unrelated to prerender were > also > > disabled in the same CL (c4c0c0a16). > > I did not know the status of omnibox prerender tests until now. One test > was > re-enabled after the massive disable, should we re-enable other Omnibox > tests > and see what happens? > > Thanks for the new tests!! I'll take a look shortly. Any other significant > changes to look at? > > https://codereview.chromium.org/2423383002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/16 15:26:34, chromium-reviews wrote: > The change to the tab helper is the most interesting, I think. The logic > there is "subtle". who is this?
On 2016/11/16 16:58:40, pasko wrote: > On 2016/11/16 15:26:34, chromium-reviews wrote: > > The change to the tab helper is the most interesting, I think. The logic > > there is "subtle". > > who is this? ok, this was mattcary@
On 2016/11/16 17:01:09, pasko wrote: > On 2016/11/16 16:58:40, pasko wrote: > > On 2016/11/16 15:26:34, chromium-reviews wrote: > > > The change to the tab helper is the most interesting, I think. The logic > > > there is "subtle". > > > > who is this? > > ok, this was mattcary@ Yeah, sorry, I replied from my phone.
(note that the test failure from the trybot seems totally unrelated to my CL).
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org
Hm part of me wants the big if statement to be moved to the ObservePolicy of PageLoadMetricsObserver. (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo...) bmcquade and I were thinking of doing something like this earlier, where each PLMO defers to a "default" policy in the parent class that would exclude things like prerender. Bryan, WDYT? This is arguably more reasonable than changing every observer to call a parent class method. Perhaps we should allow custom policy in the initializer.
On 2016/11/17 15:00:28, Charlie Harrison wrote: > Hm part of me wants the big if statement to be moved to the ObservePolicy of > PageLoadMetricsObserver. > (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo...) > > bmcquade and I were thinking of doing something like this earlier, where each > PLMO defers to a "default" policy in the parent class that would exclude things > like prerender. > > Bryan, WDYT? This is arguably more reasonable than changing every observer to > call a parent class method. Perhaps we should allow custom policy in the > initializer. My $0.02: The ObservePolicy used by OnStart seemed a little clunky to use to me, we'd have to have a "observe on prerender" value, and interpret "continue_observing" as "continue_observing_if_not_prerendering". But if you're suggesting a new InitializePolicy that could be {PRERENDER, NORMAL} with a default of NORMAL, that gets called on AddObserver or whereever, that does seems a lot nicer than the big if statement which is ugly. Doesn't much matter to me either way, though; I'm probably missing something.
On 2016/11/17 15:11:27, mattcary wrote: > On 2016/11/17 15:00:28, Charlie Harrison wrote: > > Hm part of me wants the big if statement to be moved to the ObservePolicy of > > PageLoadMetricsObserver. > > > (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo...) > > > > bmcquade and I were thinking of doing something like this earlier, where each > > PLMO defers to a "default" policy in the parent class that would exclude > things > > like prerender. > > > > Bryan, WDYT? This is arguably more reasonable than changing every observer to > > call a parent class method. Perhaps we should allow custom policy in the > > initializer. > > My $0.02: > > The ObservePolicy used by OnStart seemed a little clunky to use to me, we'd have > to have a "observe on prerender" value, and interpret "continue_observing" as > "continue_observing_if_not_prerendering". I was thinking of a default policy that would stop observing if we're prerendering, and the prerender observer would explicitly override this policy. This requires making existing observers use a default policy which is non-ideal. > > But if you're suggesting a new InitializePolicy that could be {PRERENDER, > NORMAL} with a default of NORMAL, that gets called on AddObserver or whereever, > that does seems a lot nicer than the big if statement which is ugly. Yeah something like that could work. > > Doesn't much matter to me either way, though; I'm probably missing something. The main thing we want to do is make this extensible, so we can use this pattern in similar ways in the future. As it is now, I'm pretty ok with the big if statement but let's hear what bmcquade has to say.
> > The ObservePolicy used by OnStart seemed a little clunky to use to me, we'd > have > > to have a "observe on prerender" value, and interpret "continue_observing" as > > "continue_observing_if_not_prerendering". > I was thinking of a default policy that would stop observing if we're > prerendering, and the prerender observer would explicitly override this policy. > This requires making existing observers use a default policy which is non-ideal. > Currently, we have the following: DataReductionProxyMetricsObserver and ProtocolPageLoadMetricsObserver both override OnStart to avoid capturing metrics if not in the foreground. That would actually fall under prerender, I guess, but that feels too much like implicit behavior to me. FromGWSPageLoadMetricsObserver and HttpsEngagementPLMO override OnStart only to set some state. Regardless, I'll wait for Bryan's feedback.
looked only at class interfaces, and only occasionally implementations, got a few notes already, so decided to share early, without actually understanding how it works, hence a few dumb questions potentially https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:720: "page_load_metrics/observers/prerender_page_load_metrics_observer.cc", .h as well, dunno why, mostly for consistency https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (left): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:29: bool IsPrerendering(content::WebContents* web_contents) override { just wondering if this IsPrerendering refactoring in metrics would be faster to review separately from the new FCP histograms. Up to you though. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_histograms.h (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.h:56: // analogous to the page load time above. let's copy the relevant parts of the comment from the above because the above function is likely to go away soon, and when it happens this comment won't be updated. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.h:151: static std::string GetNoStateFirstContentfulPaintHistogramName( It would be better to make this method private and expand the full histogram names in tests. This would also protect from accidentally changing what the function does with all tests unaffected. (may be better as a separate change) https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:629: if (!tab_helper) { DCHECK(tab_helper) is shorter, why not use it? https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:107: class Observer { A toplevel class PrerenderManagerObserver is preferable. Two reasonas I can see: 1. grep-able more easily 2. This class does not need to have access to internal state of the PrerenderManager, but at the place where it is extended it creates a confusion that the base class needs access to the private state of PrerenderManager. Isolating it out is more explicit and hence easier to read (modulo 'friend', which is discouraged). https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:109: virtual void OnFirstContentfulPaint() = 0; should have a comment saying on which thread all the methods of this class are called. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:228: // Records the time to first contentful paint for no state prefetch loads. is it rather for loads that were recently prefetched with nostate prefetch in the same browsing session? https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:343: void DisablePageLoadMetricsObserverForTesting() { I see a few tests calling this, does something break if they don't do it? https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:434: auto fcp_waiter = base::WrapUnique(new FirstContentfulPaintManagerWaiter()); WrapUnique is discouraged, is MakeUnique somehow not suitable here? https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:448: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, Confused. I assumed this class lives on UI thread (please add a comment in the .h to clarify this). If so, why posting a task into UI thread from UI thread? (I know it can be necessary, but would like to know whether it was actually necessary) https://codereview.chromium.org/2423383002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:1631: "../browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc", this file is now referenced in 2 places, can it be moved to some common library instead? like "page_load_metrics_test_support"?
https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:720: "page_load_metrics/observers/prerender_page_load_metrics_observer.cc", On 2016/11/17 19:52:16, pasko wrote: > .h as well, dunno why, mostly for consistency done. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (left): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:29: bool IsPrerendering(content::WebContents* web_contents) override { On 2016/11/17 19:52:16, pasko wrote: > just wondering if this IsPrerendering refactoring in metrics would be faster to > review separately from the new FCP histograms. Up to you though. We can't get the FCP metrics with the IsPrerendering refactor, so may as well do them both together. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_histograms.h (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.h:56: // analogous to the page load time above. On 2016/11/17 19:52:16, pasko wrote: > let's copy the relevant parts of the comment from the above because the above > function is likely to go away soon, and when it happens this comment won't be > updated. Done. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.h:151: static std::string GetNoStateFirstContentfulPaintHistogramName( On 2016/11/17 19:52:16, pasko wrote: > It would be better to make this method private and expand the full histogram > names in tests. This would also protect from accidentally changing what the > function does with all tests unaffected. > > (may be better as a separate change) What I've done here is consistent with how the other tests work, so this is best left to another change. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:629: if (!tab_helper) { On 2016/11/17 19:52:16, pasko wrote: > DCHECK(tab_helper) is shorter, why not use it? Copied from cases like PrerenderManager::SwapInternal :/ https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:107: class Observer { On 2016/11/17 19:52:16, pasko wrote: > A toplevel class PrerenderManagerObserver is preferable. Two reasonas I can see: > 1. grep-able more easily > 2. This class does not need to have access to internal state of the > PrerenderManager, but at the place where it is extended it creates a confusion > that the base class needs access to the private state of PrerenderManager. > Isolating it out is more explicit and hence easier to read (modulo 'friend', > which is discouraged). Done. I was copying what PrerenderContents was doing. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:109: virtual void OnFirstContentfulPaint() = 0; On 2016/11/17 19:52:16, pasko wrote: > should have a comment saying on which thread all the methods of this class are > called. Done. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:228: // Records the time to first contentful paint for no state prefetch loads. On 2016/11/17 19:52:16, pasko wrote: > is it rather for loads that were recently prefetched with nostate prefetch in > the same browsing session? Done. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:343: void DisablePageLoadMetricsObserverForTesting() { On 2016/11/17 19:52:16, pasko wrote: > I see a few tests calling this, does something break if they don't do it? Yes, if it's not disabled there will be a race between the mock timing in the test, and the actual timings recorded from the page_load_metrics. The tests instantiate a PrerenderPageLoadMetricsObserver which fires of the OnFirstContentfulPaint message manually. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:434: auto fcp_waiter = base::WrapUnique(new FirstContentfulPaintManagerWaiter()); On 2016/11/17 19:52:16, pasko wrote: > WrapUnique is discouraged, is MakeUnique somehow not suitable here? The constructor for FirstContentfulPaintManagerWaiter is private in order to ensure that it's only created via this factory function, which means it's not visible to the unique_ptr<FCPMW> class. IIRC the unique_ptr discussions, I think this is the most common reason for WrapUnique. The other may be some sort of polymorphism issue. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:448: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, On 2016/11/17 19:52:16, pasko wrote: > Confused. > > I assumed this class lives on UI thread (please add a comment in the .h to > clarify this). If so, why posting a task into UI thread from UI thread? (I know > it can be necessary, but would like to know whether it was actually necessary) This is how I think things are happening. I may very well be wrong, so please correct/improve the idiom: on UI thread, browser test calls NavigateToDestURL ->renderer starts doing its thing in another process fcp_waiter->Wait() called on UI thread ->a message loop is created which blocks In renderer, the FCP happens. A timing update is sent to the browser, which then runs OnFirstContentfulPaint in the UI thread -> we post the quit closure to our spinning message loop in the same thread Is it possible to just call waiter_->Quit() directly? I don't have a solid understanding of Chrome's message loops. https://codereview.chromium.org/2423383002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:1631: "../browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc", On 2016/11/17 19:52:16, pasko wrote: > this file is now referenced in 2 places, can it be moved to some common library > instead? like "page_load_metrics_test_support"? I have no idea, the build file is not very comprehensible. I could put it into :test_support directly. I could make a new page_load_metrics_test_support which I could then include in browser_tests and unit_tests. Could this new library could reasonably go in either browser/BUILD.gn or tests/BUILD.gn? I could put this new page_load_metrics_test_support library either as a dependence of test_support, or put it as dependencies of unit_tests and browser_tests. There are probably some other options as well... At any rate, point me in the right direction and I'll implement it.
Thanks! Glad to see this change. This basically looks good, but I wanted to get your thoughts on whether we can approach the tests a bit differently or not. https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:343: void DisablePageLoadMetricsObserverForTesting() { On 2016/11/18 at 09:21:11, mattcary wrote: > On 2016/11/17 19:52:16, pasko wrote: > > I see a few tests calling this, does something break if they don't do it? > > Yes, if it's not disabled there will be a race between the mock timing in the test, and the actual timings recorded from the page_load_metrics. > > The tests instantiate a PrerenderPageLoadMetricsObserver which fires of the OnFirstContentfulPaint message manually. You do get a bit less complete test coverage this way - by instantiating your own PrerenderPageLoadMetricsObserver you don't verify that the production code would have instantiated a PrerenderPageLoadMetricsObserver for this page load, for example. You also aren't verifying that the expected timing updates are dispatched to your observer for a page with a contentful paint, though this is reasonably well tested elsewhere. Is there something we could do to enable you to rely on the default observer being instantiated, and not have to create a custom observer? One thing our other browser tests have done in these cases is assert not that a specific value of FCP was logged, but just that the FCP histogram did get logged. We then have separate unit tests to provide specific values and assert that they get logged. This gives good coverage in both areas: in the browser test we verify the expected end to end logic as best we can, while in the unit test we verify the expected behavior of the observer when certain timing events are passed to it. I don't feel strongly here - it's up to you - but if there's something we can do to enable you to avoid needing this method, I'd be happy to try to support that, and I think it'd lead to slightly better coverage as well, as you'd be testing that the observer instantiated in production code has the expected value, rather than creating a fake observer. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:40: web_contents_, *timing.first_contentful_paint); why is the * needed here when passing timing.first_contentful_paint? https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:28: explicit PrerenderPageLoadMetricsObserver( given the static createifneeded method. does this need to also be public? if public only for tests, let's add a comment to note that. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:29: prerender::PrerenderManager* manager, just want to make sure PrerenderManager's lifetime is guaranteed to exceed the lifetime of the observer - is that right? what's the lifetime of PrerenderManager? Maybe add a short comment to note it. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3306: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingSimple) { looking at these tests, they don't feel like browser/end to end tests as much as unit tests of the PrerenderPageLoadMetricsObserver. But you end up having to bring up a lot of machinery just to unit test this class. Is it possible to instead add a prerender_page_load_metrics_observer_unittest and use the existing PageLoadMetricsObserverTestHarness to write these same tests?
Thanks for your comments, Bryan. Any opinion on the best way to avoid the ugly if statement in PageLoadMetricsEmbedder::RegisterObservers? https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:343: void DisablePageLoadMetricsObserverForTesting() { > > One thing our other browser tests have done in these cases is assert not that a > specific value of FCP was logged, but just that the FCP histogram did get > logged. We then have separate unit tests to provide specific values and assert > that they get logged. This gives good coverage in both areas: in the browser > We already have logging of the histogram counts elsewhere (or will have it; there's other CLs in flight). The point of these tests is specifically the timing, as Egor pointed out earlier on in this CL, so that we can confirm, eg, when there are two prefetches the perceived contentful paint is attributed to the correct load and so the timed interval is correct. > I don't feel strongly here - it's up to you - but if there's something we can do > to enable you to avoid needing this method, I'd be happy to try to support that, > and I think it'd lead to slightly better coverage as well, as you'd be testing > that the observer instantiated in production code has the expected value, rather > than creating a fake observer. > I basically agree with you; I'd prefer to test the actual observer chain too. However, it seems very difficult to initialize the observer with a custom class that could override the timings (and I think it's necessary to replace the observer, otherwise we race with actual timing updates). The only solution I can see would involve creating some global singleton for PrerenderPLMO to use in CreateIfNeeded for injection, and that approach has been rejected elsewhere. I'm very possibly missing a solution, though, so any suggestions are welcome! Note also that it's not feasible to test this sort of prerender other than in a browser test. I tried for a couple of days to get a unittest together, in the style of those found in page_load_metrics/observers/, and one ends up on a path to mocking out all of chrome. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:40: web_contents_, *timing.first_contentful_paint); On 2016/11/18 14:52:37, Bryan McQuade wrote: > why is the * needed here when passing timing.first_contentful_paint? timing.first_contentful_paint is a base::Optional<base::TimeDelta>. The * burrows to the inside of the option. Someone in base likes C++ overloading :) https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:28: explicit PrerenderPageLoadMetricsObserver( On 2016/11/18 14:52:37, Bryan McQuade wrote: > given the static createifneeded method. does this need to also be public? if > public only for tests, let's add a comment to note that. Done. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:29: prerender::PrerenderManager* manager, On 2016/11/18 14:52:37, Bryan McQuade wrote: > just want to make sure PrerenderManager's lifetime is guaranteed to exceed the > lifetime of the observer - is that right? what's the lifetime of > PrerenderManager? Maybe add a short comment to note it. Done. The PrerenderManager lifetime is like that of the browser process; it has to outlive all the renderers and web contents as it manages who gets swapped in where. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3306: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingSimple) { On 2016/11/18 14:52:37, Bryan McQuade wrote: > looking at these tests, they don't feel like browser/end to end tests as much as > unit tests of the PrerenderPageLoadMetricsObserver. But you end up having to > bring up a lot of machinery just to unit test this class. Is it possible to > instead add a prerender_page_load_metrics_observer_unittest and use the existing > PageLoadMetricsObserverTestHarness to write these same tests? As mentioned in the other comment, no, apparently it isn't possible. I tried very hard.
On 2016/11/18 at 15:14:14, mattcary wrote: > Thanks for your comments, Bryan. Any opinion on the best way to avoid the ugly if statement in PageLoadMetricsEmbedder::RegisterObservers? I think this is ok for now. We could delegate to each observer to decide if it wants to observe or not, but I like the simplicity of doing this in RegisterObservers for now. I'll think about whether there's a nicer way to do this going forward. > > https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_manager.h (right): > > https://codereview.chromium.org/2423383002/diff/80001/chrome/browser/prerende... > chrome/browser/prerender/prerender_manager.h:343: void DisablePageLoadMetricsObserverForTesting() { > > > > One thing our other browser tests have done in these cases is assert not that a > > specific value of FCP was logged, but just that the FCP histogram did get > > logged. We then have separate unit tests to provide specific values and assert > > that they get logged. This gives good coverage in both areas: in the browser > > > We already have logging of the histogram counts elsewhere (or will have it; there's other CLs in flight). > > The point of these tests is specifically the timing, as Egor pointed out earlier on in this CL, so that we can confirm, eg, when there are two prefetches the perceived contentful paint is attributed to the correct load and so the timed interval is correct. Ah, ok, makes sense, thanks! > > > > I don't feel strongly here - it's up to you - but if there's something we can do > > to enable you to avoid needing this method, I'd be happy to try to support that, > > and I think it'd lead to slightly better coverage as well, as you'd be testing > > that the observer instantiated in production code has the expected value, rather > > than creating a fake observer. > > > I basically agree with you; I'd prefer to test the actual observer chain too. However, it seems very difficult to initialize the observer with a custom class that could override the timings (and I think it's necessary to replace the observer, otherwise we race with actual timing updates). The only solution I can see would involve creating some global singleton for PrerenderPLMO to use in CreateIfNeeded for injection, and that approach has been rejected elsewhere. > > I'm very possibly missing a solution, though, so any suggestions are welcome! > > Note also that it's not feasible to test this sort of prerender other than in a browser test. I tried for a couple of days to get a unittest together, in the style of those found in page_load_metrics/observers/, and one ends up on a path to mocking out all of chrome. > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:40: web_contents_, *timing.first_contentful_paint); > On 2016/11/18 14:52:37, Bryan McQuade wrote: > > why is the * needed here when passing timing.first_contentful_paint? > > timing.first_contentful_paint is a base::Optional<base::TimeDelta>. The * burrows to the inside of the option. Someone in base likes C++ overloading :) > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h (right): > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:28: explicit PrerenderPageLoadMetricsObserver( > On 2016/11/18 14:52:37, Bryan McQuade wrote: > > given the static createifneeded method. does this need to also be public? if > > public only for tests, let's add a comment to note that. > > Done. > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:29: prerender::PrerenderManager* manager, > On 2016/11/18 14:52:37, Bryan McQuade wrote: > > just want to make sure PrerenderManager's lifetime is guaranteed to exceed the > > lifetime of the observer - is that right? what's the lifetime of > > PrerenderManager? Maybe add a short comment to note it. > > Done. > > The PrerenderManager lifetime is like that of the browser process; it has to outlive all the renderers and web contents as it manages who gets swapped in where. > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... > chrome/browser/prerender/prerender_browsertest.cc:3306: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingSimple) { > On 2016/11/18 14:52:37, Bryan McQuade wrote: > > looking at these tests, they don't feel like browser/end to end tests as much as > > unit tests of the PrerenderPageLoadMetricsObserver. But you end up having to > > bring up a lot of machinery just to unit test this class. Is it possible to > > instead add a prerender_page_load_metrics_observer_unittest and use the existing > > PageLoadMetricsObserverTestHarness to write these same tests? > > As mentioned in the other comment, no, apparently it isn't possible. I tried very hard. Makes sense. I think the approach you're taking is the best option overall. Thanks!
https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: prerender_manager_->RecordPerceivedFirstContentfulPaint( one thing we do for paint-based timings is only track metrics for pages that are foregrounded for the entire duration from nav_start to the given paint timing. The reason for this is that the rendering pipeline is disabled when a page is backgrounded, so if you include timings for pages that spent some time in the background, what you may end up measuring is the time until the user foregrounded the tab, a long time after it had been ready to paint in the background. So I'd suggest one of a few options: 1. only log metrics for page loads that spend all of their time in the foreground 2. log metrics that spend time between start and FCP in the background in a separate histogram with a '.Background' suffix - this is what we do elsewhere, though in practice these histograms haven't been very useful other than to have a sense for the % of page loads that get backgrounded before FCP. Your implementation is made slightly more challenging by the fact that prerenders do start in the background. I think you want to track whether the page load spends any time from the time the prerender is 'used' to the given paint event in the background. To do this you probably need to implement the PLMO OnHidden callback, to track if you end up being hidden after the prerender is used but before FCP fires. One simple thing you could do is just return STOP_OBSERVING in your OnHidden impl, the first time it gets called after the prerender is used. Else you can have a boolean member or a timestamp for first_background_after_use_ that allows you to know if you were backgrounded before a given metric event.
https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: prerender_manager_->RecordPerceivedFirstContentfulPaint( On 2016/11/18 15:23:28, Bryan McQuade wrote: > one thing we do for paint-based timings is only track metrics for pages that are > foregrounded for the entire duration from nav_start to the given paint timing. > The reason for this is that the rendering pipeline is disabled when a page is > backgrounded, so if you include timings for pages that spent some time in the > background, what you may end up measuring is the time until the user > foregrounded the tab, a long time after it had been ready to paint in the > background. > > So I'd suggest one of a few options: > 1. only log metrics for page loads that spend all of their time in the > foreground > 2. log metrics that spend time between start and FCP in the background in a > separate histogram with a '.Background' suffix - this is what we do elsewhere, > though in practice these histograms haven't been very useful other than to have > a sense for the % of page loads that get backgrounded before FCP. > > Your implementation is made slightly more challenging by the fact that > prerenders do start in the background. I think you want to track whether the > page load spends any time from the time the prerender is 'used' to the given > paint event in the background. To do this you probably need to implement the > PLMO OnHidden callback, to track if you end up being hidden after the prerender > is used but before FCP fires. One simple thing you could do is just return > STOP_OBSERVING in your OnHidden impl, the first time it gets called after the > prerender is used. Else you can have a boolean member or a timestamp for > first_background_after_use_ that allows you to know if you were backgrounded > before a given metric event. Interesting. I designed this to be as parallel as possible to PerceivedPLT, but as we're getting rid of PLT I guess that may not matter much. In case it wasn't clear from the other parts of this CL, what we're doing is reporting the time from prerender swap in until FCP (by keeping track of load start time, prerender swap time, and subtracting appropriately). Okay, with that out of the way, let me see if I understand your suggestion. You're concerned about when the (pre)renderer gets hidden between swap and FCP. This can be detected via the PLMO OnHidden callback, and you suggest just splitting our histogram in two depending on whether it was backgrounded or not, but otherwise not adjusting any of the timings. If I return STOP_OBSERVING from OnHidden, I won't fire on the FCP, and the effect will be to not record any backgrounded pages (and so I just keep the single histogram that I currently have). This seems pretty reasonable to me, I'll see if I can make that work. Let me know if I misinterpreted anything.
Great, yes, either returning STOP_OBSERVING or segmenting by foreground/background metrics should work. One more suggestion about taking the reference time that all page load metric timedeltas are deltas from. https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_tab_helper.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.cc:115: last_load_ = GetTimeTicksFromPrerenderManager(); the reference start time that all page load metrics time deltas are measured from is NavigationHandle::NavigationStart() - can we use that instead? Perhaps you can override OnStart and record the navigation start time in your observer, and then instead of passing the timedelta for FCP to prerendermanager, you can instead pass navigation_start + first_contentful_paint TimeTicks, so you have the absolute time that the FCP happened, which you can then subtract last_swap_ from to get the perceived delta.
On 2016/11/18 15:46:42, Bryan McQuade wrote: > Great, yes, either returning STOP_OBSERVING or segmenting by > foreground/background metrics should work. > Okay, thanks. I ended up segmenting the metrics under the theory that it's best to know how much this is happening. I suspect in practice we'll just ignore the *.Hidden histograms due to the increased variance, but at least we'll know how common an occurrence it is by the counts.
https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_tab_helper.cc (right): https://codereview.chromium.org/2423383002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.cc:115: last_load_ = GetTimeTicksFromPrerenderManager(); On 2016/11/18 15:46:42, Bryan McQuade wrote: > the reference start time that all page load metrics time deltas are measured > from is NavigationHandle::NavigationStart() - can we use that instead? > > Perhaps you can override OnStart and record the navigation start time in your > observer, and then instead of passing the timedelta for FCP to prerendermanager, > you can instead pass navigation_start + first_contentful_paint TimeTicks, so you > have the absolute time that the FCP happened, which you can then subtract > last_swap_ from to get the perceived delta. Done, although there seems no nice way to test due to the need to mock out the NavigationHandle. I instead allow one to set the start time on the observer directly, which means ::OnStart is not directly tested. The logic there is very simple, though, so I think that's the best we can do. Contradiction welcome :)
Looks good! https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (left): https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:325: TEST_F(MetricsWebContentsObserverTest, DontLogPrerender) { It would be nice if we could keep a test like this in some form, maybe as a browser test. https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:43: if (!started_in_foreground) I would have thought all prerenders start in the background. Don't we want was_hidden_ to be true only after we've been swapped in and then backgrounded? https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { When will this happen?
I haven't looked at the tests yet, but already accumulated enough misunderstanding of the logic to ask plenty of questions. Please help me understand those edge cases. Disabling the whole metrics recorder for testing still looks alarming, but I am postponing this discussion, I do not have any suggestions for that yet. .. And various semi-random fly-by nit-ish things. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: PrerenderPageLoadMetricsObserver::OnStart( I am reading the comment at PageLoadMetricsObserver::OnStart that says: // Implementers of OnStart that only want // to process non-same-page navigations should also check to see that the page // load committed via OnCommit or committed_url in // PageLoadExtraInfo. I think same-page navigations can happen in prerendered webcontents (after commit - via client redirects, but before first paint), so we might have multiple calls to OnStart() here, and would get start_ticks_ later than we wanted, right? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:43: if (!started_in_foreground) From my reading .. |started_in_foreground| is true iff the WebContents was in foreground when navigation started. This seems to be about the prerendered webcontents, which makes me wonder .. is it ever (started_in_foreground == true)? Seems that the test that checks PerceivedTTFCP.Visible does not execute this codepath, is it true? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { is it null only in tests? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:68: return CONTINUE_OBSERVING; I see other observers early give up on seeing something going to background, would it simplify to do similarly here? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:25: static std::unique_ptr<page_load_metrics::PageLoadMetricsObserver> nit: Add #include <memory> for unique_ptr<> [build/include_what_you_use] [4] https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.h:52: prerender::PrerenderManager* const prerender_manager_; that const is nice, I did not know that the style encourages emphasizing constness of members after construction. Thanks. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3393: observer.SetNavigationStartFromZero(time_out_delta + This is the only callsite for this function, and can be replaced with: observer.SetNavigationStartMilliseconds(1000); less methods, simpler .. does any other change use it more extensively? I also find the 'FromZero' part confusing, the 'Millisectonds' method is similarly 'from zero', no? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_tab_helper.cc (left): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.cc:162: DCHECK_EQ(NAVIGATION_TYPE_NORMAL, navigation_type_); we should not call PrerenderSwappedIn() before DidStartProvisionalLoadForFrame(), so the fact that determining navigation time moved to the latter method, should not affect this DCHECK. Should we still have this DCHECK then? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.cc:193: navigation_type_ = NAVIGATION_TYPE_NORMAL; why removing this?
Thanks! A couple more comments. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: PrerenderPageLoadMetricsObserver::OnStart( On 2016/11/21 at 14:44:36, pasko wrote: > I am reading the comment at PageLoadMetricsObserver::OnStart that says: > > // Implementers of OnStart that only want > // to process non-same-page navigations should also check to see that the page > // load committed via OnCommit or committed_url in > // PageLoadExtraInfo. > > I think same-page navigations can happen in prerendered webcontents (after commit - via client redirects, but before first paint), so we might have multiple calls to OnStart() here, and would get start_ticks_ later than we wanted, right? Good point, yes, same page navs are something to be aware of when implementing OnStart. Fortunately, OnStart will only be called once per instance - a same page nav will have its own new instance of a PrerenderPageLoadMetricsObserver. Given that OnStart doesn't do any logging and only records state that would be used later, and given that no additional callbacks will be invoked for same-page navs in the current impl, we're ok here. That said, I'll do 2 things: * I'll update the OnStart comment to be clearer about this * I'll reach out to the navigation team to see if we can get NavigationHandle to report that it's a same page nav at construction time, so we can avoid this complexity. I don't think any changes are needed to this implementation currently, though. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:43: if (!started_in_foreground) On 2016/11/21 at 14:44:36, pasko wrote: > From my reading .. |started_in_foreground| is true iff the WebContents was in foreground when navigation started. This seems to be about the prerendered webcontents, which makes me wonder .. is it ever (started_in_foreground == true)? > > Seems that the test that checks PerceivedTTFCP.Visible does not execute this codepath, is it true? A prerender should always have started_in_foreground = false, so I think we could just DCHECK(!started_in_foreground); here - that's what the code in PageLoadTracker has been doing: if (embedder_interface_->IsPrerendering( navigation_handle->GetWebContents())) { DCHECK(!started_in_foreground_); ... } https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 at 14:44:36, pasko wrote: > is it null only in tests? Yeah - I'd like to avoid this special casing if at all possible - can we force an invocation to OnStart in all tests? Having test-only code in the production codepath seems like something we should strive to avoid if possible.
https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: PrerenderPageLoadMetricsObserver::OnStart( On 2016/11/21 14:56:54, Bryan McQuade wrote: > On 2016/11/21 at 14:44:36, pasko wrote: > > I am reading the comment at PageLoadMetricsObserver::OnStart that says: > > > > // Implementers of OnStart that only want > > // to process non-same-page navigations should also check to see that the > page > > // load committed via OnCommit or committed_url in > > // PageLoadExtraInfo. > > > > I think same-page navigations can happen in prerendered webcontents (after > commit - via client redirects, but before first paint), so we might have > multiple calls to OnStart() here, and would get start_ticks_ later than we > wanted, right? > > Good point, yes, same page navs are something to be aware of when implementing > OnStart. Fortunately, OnStart will only be called once per instance - a same > page nav will have its own new instance of a PrerenderPageLoadMetricsObserver. > Given that OnStart doesn't do any logging and only records state that would be > used later, and given that no additional callbacks will be invoked for same-page > navs in the current impl, we're ok here. > > That said, I'll do 2 things: > * I'll update the OnStart comment to be clearer about this > * I'll reach out to the navigation team to see if we can get NavigationHandle to > report that it's a same page nav at construction time, so we can avoid this > complexity. > > I don't think any changes are needed to this implementation currently, though. NavigationHandle has IsSamePage() which is constant throughout its lifetime. That should be good enough for the MWCO. Additionally, I'm pretty sure same page navs don't go through the NavigationThrottle stack, so we will never call OnStart for those.
On 2016/11/21 at 15:06:56, csharrison wrote: > https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: PrerenderPageLoadMetricsObserver::OnStart( > On 2016/11/21 14:56:54, Bryan McQuade wrote: > > On 2016/11/21 at 14:44:36, pasko wrote: > > > I am reading the comment at PageLoadMetricsObserver::OnStart that says: > > > > > > // Implementers of OnStart that only want > > > // to process non-same-page navigations should also check to see that the > > page > > > // load committed via OnCommit or committed_url in > > > // PageLoadExtraInfo. > > > > > > I think same-page navigations can happen in prerendered webcontents (after > > commit - via client redirects, but before first paint), so we might have > > multiple calls to OnStart() here, and would get start_ticks_ later than we > > wanted, right? > > > > Good point, yes, same page navs are something to be aware of when implementing > > OnStart. Fortunately, OnStart will only be called once per instance - a same > > page nav will have its own new instance of a PrerenderPageLoadMetricsObserver. > > Given that OnStart doesn't do any logging and only records state that would be > > used later, and given that no additional callbacks will be invoked for same-page > > navs in the current impl, we're ok here. > > > > That said, I'll do 2 things: > > * I'll update the OnStart comment to be clearer about this > > * I'll reach out to the navigation team to see if we can get NavigationHandle to > > report that it's a same page nav at construction time, so we can avoid this > > complexity. > > > > I don't think any changes are needed to this implementation currently, though. > > NavigationHandle has IsSamePage() which is constant throughout its lifetime. That should be good enough for the MWCO. > > Additionally, I'm pretty sure same page navs don't go through the NavigationThrottle stack, so we will never call OnStart for those. Ah, you're right! This is great as it means none of our callbacks should be invoked for same page navs. I'll add a test and update the comment.
https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:43: if (!started_in_foreground) On 2016/11/21 14:56:54, Bryan McQuade wrote: > On 2016/11/21 at 14:44:36, pasko wrote: > > From my reading .. |started_in_foreground| is true iff the WebContents was in > foreground when navigation started. This seems to be about the prerendered > webcontents, which makes me wonder .. is it ever (started_in_foreground == > true)? > > > > Seems that the test that checks PerceivedTTFCP.Visible does not execute this > codepath, is it true? > > A prerender should always have started_in_foreground = false, so I think we > could just DCHECK(!started_in_foreground); here - that's what the code in > PageLoadTracker has been doing: > if (embedder_interface_->IsPrerendering( > navigation_handle->GetWebContents())) { > DCHECK(!started_in_foreground_); > ... > } Done. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 14:56:54, Bryan McQuade wrote: > On 2016/11/21 at 14:44:36, pasko wrote: > > is it null only in tests? > > Yeah - I'd like to avoid this special casing if at all possible - can we force > an invocation to OnStart in all tests? Having test-only code in the production > codepath seems like something we should strive to avoid if possible. It wasn't clear to me that OnStart is always going to be invoked. We have to do something if it's not, for whatever reason, now or in the future. Check-failing seems a little extreme as this is just histogram code. I could also not record anything in this case; the point here, though, is that PrerenderManager is able to do something sensible with invalid timings (recording a failed status). https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:68: return CONTINUE_OBSERVING; On 2016/11/21 14:44:36, pasko wrote: > I see other observers early give up on seeing something going to background, > would it simplify to do similarly here? I thought it would be a good idea to know how often we get hidden; if it's a lot, but we just drop observations, we're going to have skewed results. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3393: observer.SetNavigationStartFromZero(time_out_delta + On 2016/11/21 14:44:36, pasko wrote: > This is the only callsite for this function, and can be replaced with: > observer.SetNavigationStartMilliseconds(1000); > > less methods, simpler .. does any other change use it more extensively? > > I also find the 'FromZero' part confusing, the 'Millisectonds' method is > similarly 'from zero', no? No, it needs to add in the time_out_delta to the 1000 milliseconds. I agree it's all very confusing. The problem is that one can't set a TimeTicks to an arbitrary value, one can only add TimeDeltas to it. That's what I'm trying to hide with these methods. This method takes a TimeDelta. Possibly this should be the only method, and the Milliseconds method should be replaced with SetNavigationStart(base::TimeDelta::FromSeconds(1)), even though that's more cumbersome. WDYT? https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_tab_helper.cc (left): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.cc:162: DCHECK_EQ(NAVIGATION_TYPE_NORMAL, navigation_type_); On 2016/11/21 14:44:36, pasko wrote: > we should not call PrerenderSwappedIn() before > DidStartProvisionalLoadForFrame(), so the fact that determining navigation time > moved to the latter method, should not affect this DCHECK. Should we still have > this DCHECK then? No. The FCP can occur after the page load, and we can't reset the navigation type else we lose the histograms. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.cc:193: navigation_type_ = NAVIGATION_TYPE_NORMAL; On 2016/11/21 14:44:36, pasko wrote: > why removing this? It's moved up to provisional start. This is the confusing state I mentioned that prerender_tab_helper has to keep track of; it was a bad idea for it to originally set it in the constructor. https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:43: if (!started_in_foreground) On 2016/11/21 14:30:17, Charlie Harrison wrote: > I would have thought all prerenders start in the background. Don't we want > was_hidden_ to be true only after we've been swapped in and then backgrounded? That's a good point. Done. https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 14:30:17, Charlie Harrison wrote: > When will this happen? If OnStart() doesn't get called. When could that happen? Probably never, now, but maybe in the future? At any rate, I don't want to pollute our histograms with strange data, and it seems extreme to check-fail.
https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 at 16:13:37, mattcary wrote: > On 2016/11/21 14:56:54, Bryan McQuade wrote: > > On 2016/11/21 at 14:44:36, pasko wrote: > > > is it null only in tests? > > > > Yeah - I'd like to avoid this special casing if at all possible - can we force > > an invocation to OnStart in all tests? Having test-only code in the production > > codepath seems like something we should strive to avoid if possible. > > It wasn't clear to me that OnStart is always going to be invoked. We have to do something if it's not, for whatever reason, now or in the future. Check-failing seems a little extreme as this is just histogram code. > > I could also not record anything in this case; the point here, though, is that PrerenderManager is able to do something sensible with invalid timings (recording a failed status). OnStart will always be invoked for every instantiated observer - it's a bug if that ever doesn't happen. You can see that we instantiate them then immediately call OnStart on them here: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... So I think you can DCHECK or even CHECK that start_ticks_ is non-null here. https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 at 16:13:37, mattcary wrote: > On 2016/11/21 14:30:17, Charlie Harrison wrote: > > When will this happen? > > If OnStart() doesn't get called. When could that happen? Probably never, now, but maybe in the future? At any rate, I don't want to pollute our histograms with strange data, and it seems extreme to check-fail. We'll always call OnStart once, for all observers. I'd suggest at least DCHECKing that this can't happen. Perhaps also log an UMA error counter if you want to know how often this happens (should be zero times).
https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3393: observer.SetNavigationStartFromZero(time_out_delta + On 2016/11/21 16:13:37, mattcary wrote: > On 2016/11/21 14:44:36, pasko wrote: > > This is the only callsite for this function, and can be replaced with: > > observer.SetNavigationStartMilliseconds(1000); > > > > less methods, simpler .. does any other change use it more extensively? > > > > I also find the 'FromZero' part confusing, the 'Millisectonds' method is > > similarly 'from zero', no? > > No, it needs to add in the time_out_delta to the 1000 milliseconds. > > I agree it's all very confusing. The problem is that one can't set a TimeTicks > to an arbitrary value, one can only add TimeDeltas to it. That's what I'm trying > to hide with these methods. This method takes a TimeDelta. Possibly this should > be the only method, and the Milliseconds method should be replaced with > SetNavigationStart(base::TimeDelta::FromSeconds(1)), even though that's more > cumbersome. WDYT? Actually, it occurred to me that I should just set the NavigationStart ticks from the test clock directly. Upload forthcoming.
https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:39: PrerenderPageLoadMetricsObserver::OnStart( On 2016/11/21 15:06:56, Charlie Harrison wrote: > On 2016/11/21 14:56:54, Bryan McQuade wrote: > > On 2016/11/21 at 14:44:36, pasko wrote: > > > I am reading the comment at PageLoadMetricsObserver::OnStart that says: > > > > > > // Implementers of OnStart that only want > > > // to process non-same-page navigations should also check to see that the > > page > > > // load committed via OnCommit or committed_url in > > > // PageLoadExtraInfo. > > > > > > I think same-page navigations can happen in prerendered webcontents (after > > commit - via client redirects, but before first paint), so we might have > > multiple calls to OnStart() here, and would get start_ticks_ later than we > > wanted, right? > > > > Good point, yes, same page navs are something to be aware of when implementing > > OnStart. Fortunately, OnStart will only be called once per instance - a same > > page nav will have its own new instance of a PrerenderPageLoadMetricsObserver. > > Given that OnStart doesn't do any logging and only records state that would be > > used later, and given that no additional callbacks will be invoked for > same-page > > navs in the current impl, we're ok here. > > > > That said, I'll do 2 things: > > * I'll update the OnStart comment to be clearer about this > > * I'll reach out to the navigation team to see if we can get NavigationHandle > to > > report that it's a same page nav at construction time, so we can avoid this > > complexity. > > > > I don't think any changes are needed to this implementation currently, though. > > NavigationHandle has IsSamePage() which is constant throughout its lifetime. > That should be good enough for the MWCO. > > Additionally, I'm pretty sure same page navs don't go through the > NavigationThrottle stack, so we will never call OnStart for those. Acknowledged. https://codereview.chromium.org/2423383002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 16:26:47, Bryan McQuade wrote: > On 2016/11/21 at 16:13:37, mattcary wrote: > > On 2016/11/21 14:56:54, Bryan McQuade wrote: > > > On 2016/11/21 at 14:44:36, pasko wrote: > > > > is it null only in tests? > > > > > > Yeah - I'd like to avoid this special casing if at all possible - can we > force > > > an invocation to OnStart in all tests? Having test-only code in the > production > > > codepath seems like something we should strive to avoid if possible. > > > > It wasn't clear to me that OnStart is always going to be invoked. We have to > do something if it's not, for whatever reason, now or in the future. > Check-failing seems a little extreme as this is just histogram code. > > > > I could also not record anything in this case; the point here, though, is that > PrerenderManager is able to do something sensible with invalid timings > (recording a failed status). > > OnStart will always be invoked for every instantiated observer - it's a bug if > that ever doesn't happen. You can see that we instantiate them then immediately > call OnStart on them here: > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... > > So I think you can DCHECK or even CHECK that start_ticks_ is non-null here. Done. https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: if (start_ticks_.is_null()) { On 2016/11/21 16:26:47, Bryan McQuade wrote: > On 2016/11/21 at 16:13:37, mattcary wrote: > > On 2016/11/21 14:30:17, Charlie Harrison wrote: > > > When will this happen? > > > > If OnStart() doesn't get called. When could that happen? Probably never, now, > but maybe in the future? At any rate, I don't want to pollute our histograms > with strange data, and it seems extreme to check-fail. > > We'll always call OnStart once, for all observers. I'd suggest at least > DCHECKing that this can't happen. Perhaps also log an UMA error counter if you > want to know how often this happens (should be zero times). DCHECK'd
LGTM for page_load_metrics, thanks! Just one optional suggestion. https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: prerender_manager_->RecordPerceivedFirstContentfulPaint( optional: it turns out that, given your implementation, you can get away without implementing OnHidden, and instead doing the following bool was_hidden = extra_info.first_background_time && timing.first_contentful_paint.value() <= extra_info.first_background_time.value();
On 2016/11/23 13:51:23, Bryan McQuade wrote: > LGTM for page_load_metrics, thanks! Just one optional suggestion. > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > File > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: > prerender_manager_->RecordPerceivedFirstContentfulPaint( > optional: it turns out that, given your implementation, you can get away without > implementing OnHidden, and instead doing the following > > bool was_hidden = extra_info.first_background_time && > timing.first_contentful_paint.value() <= > extra_info.first_background_time.value(); Thanks. I don't quite understand that logic: from your earlier comment, I thought that the paint would wait until the page was foregrounded again, in which case the FCP time would be larger than the background time?
On 2016/11/23 at 14:53:15, mattcary wrote: > On 2016/11/23 13:51:23, Bryan McQuade wrote: > > LGTM for page_load_metrics, thanks! Just one optional suggestion. > > > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > > File > > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc > > (right): > > > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: > > prerender_manager_->RecordPerceivedFirstContentfulPaint( > > optional: it turns out that, given your implementation, you can get away without > > implementing OnHidden, and instead doing the following > > > > bool was_hidden = extra_info.first_background_time && > > timing.first_contentful_paint.value() <= > > extra_info.first_background_time.value(); > > Thanks. I don't quite understand that logic: from your earlier comment, I thought that the paint would wait until the page was foregrounded again, in which case the FCP time would be larger than the background time? I'm sorry, my code snippet is totally wrong. Should read: bool was_hidden_before_fcp = extra_info.first_background_time && extra_info.first_background_time.value() < timing.first_contentful_paint.value(); The first_background_time is intended to track the first time the page was background after being in the foreground, so for prerender this would only get set of the prerender is used (foregrounded), and then backgrounded. I haven't tested this with prerender though so it might be worth veryfing. I'm also fine leaving the implementation as you have it currently.
On 2016/11/23 15:00:19, Bryan McQuade wrote: > On 2016/11/23 at 14:53:15, mattcary wrote: > > On 2016/11/23 13:51:23, Bryan McQuade wrote: > > > LGTM for page_load_metrics, thanks! Just one optional suggestion. > > > > > > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > > > File > > > > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc > > > (right): > > > > > > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > > > > chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:53: > > > prerender_manager_->RecordPerceivedFirstContentfulPaint( > > > optional: it turns out that, given your implementation, you can get away > without > > > implementing OnHidden, and instead doing the following > > > > > > bool was_hidden = extra_info.first_background_time && > > > timing.first_contentful_paint.value() <= > > > extra_info.first_background_time.value(); > > > > Thanks. I don't quite understand that logic: from your earlier comment, I > thought that the paint would wait until the page was foregrounded again, in > which case the FCP time would be larger than the background time? > > I'm sorry, my code snippet is totally wrong. > > Should read: > > bool was_hidden_before_fcp = extra_info.first_background_time && > extra_info.first_background_time.value() < > timing.first_contentful_paint.value(); > > The first_background_time is intended to track the first time the page was > background after being in the foreground, so for prerender this would only get > set of the prerender is used (foregrounded), and then backgrounded. I haven't > tested this with prerender though so it might be worth veryfing. I'm also fine > leaving the implementation as you have it currently. Good, I'm not crazy ;) I think I'll leave it as-is, as testing with time-related comparisons seems to be hard (as the times are set manually, it's hard to verify that the real world actually works as the test does).
https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (left): https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:325: TEST_F(MetricsWebContentsObserverTest, DontLogPrerender) { Do you think we could maintain a test like this (maybe as a browsertest), to ensure we aren't logging metrics for our core metrics during prerender?
https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (left): https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:325: TEST_F(MetricsWebContentsObserverTest, DontLogPrerender) { On 2016/11/23 18:25:46, Charlie Harrison(OOO Nov23-28) wrote: > Do you think we could maintain a test like this (maybe as a browsertest), to > ensure we aren't logging metrics for our core metrics during prerender? Sorry, dropped this. I started looking in, it's a little tricky, especially since the prerender browsertests need to be done over in a different directory. I could have a test that spot-checks some of the core metrics, but I think it would be difficult to guarantee coverage without some explicit registry scheme. Would that be enough?
https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:329: GetHistogramName(origin, IsOriginWash(), "PerceivedTTFCPRecorded") + If we want to compare NoStatePrefetch vs Prerender vs Control using a UMA experiment, we need to report all the TTFCP in a single histogram, and then use Finch to compare. So in practice I think we should merge this histogram and the "NoStatePrefetchTTFCP" histograms. Another option would be to keep these histograms if they have some value, and add another one that would be the merge of all of them. This could then be done in another CL.
https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (left): https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:325: TEST_F(MetricsWebContentsObserverTest, DontLogPrerender) { On 2016/11/24 09:07:42, mattcary wrote: > On 2016/11/23 18:25:46, Charlie Harrison(OOO Nov23-28) wrote: > > Do you think we could maintain a test like this (maybe as a browsertest), to > > ensure we aren't logging metrics for our core metrics during prerender? > > Sorry, dropped this. I started looking in, it's a little tricky, especially > since the prerender browsertests need to be done over in a different directory. > I could have a test that spot-checks some of the core metrics, but I think it > would be difficult to guarantee coverage without some explicit registry scheme. > Would that be enough? Done.
A major update based on our finalized experiment plan. We have decided to unify the nostate prefetch, prerender, and control histograms into a single FCP histogram so that we can compare performance in finch during our experiment. This CL does that, as well as updating all histograms with the visibility information. Note that I have not deleted the NoStateTTFCP histograms; we should probably do that as they have not been used yet, but I didn't know if this was the appropriate CL.
Looks great, only nits. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:6: #include "base/logging.h" for the DCHECK https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:26: return base::WrapUnique<page_load_metrics::PageLoadMetricsObserver>( Nit: I think MakeUnique is recommended over WrapUnique https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:37: DCHECK(prerender_manager_); DCHECK(web_contents_) as well? https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:105: return prerender::PrerenderContents::FromWebContents(web_contents_) != optional nit, just to fit on 80 columns: remove "!= nullptr" https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3285: "Prerender.websame_PrefetchTTFCP.Warm.Cacheable.Visible", 1654, 1); It may be nice to have base::TimeDelta::FromMilliseconds(2654) and base::Time::FromDoubleT(1) defined as constants, and compute 1654 as the difference between the two. Here and in other tests. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:72: virtual void OnFirstContentfulPaint() = 0; Leak: Add a empty virtual destructor (or pure). https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:642: std::vector<std::unique_ptr<PrerenderManagerObserver>> observers_; Nit: consider using base::ObserverList. Note it has a slightly different behavior because it does not take ownership though.
https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:6: On 2016/11/29 15:54:39, droger wrote: > #include "base/logging.h" > for the DCHECK Done. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:26: return base::WrapUnique<page_load_metrics::PageLoadMetricsObserver>( On 2016/11/29 15:54:39, droger wrote: > Nit: I think MakeUnique is recommended over WrapUnique I thought you needed to do this to change the type from unique_ptr<PrerenderPageLoadMetricsObserver> to unique_ptr<PageLoadMetricsObserver>, but apparently you don't. Maybe I have gone crazy. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/prerender_page_load_metrics_observer.cc:37: DCHECK(prerender_manager_); On 2016/11/29 15:54:39, droger wrote: > DCHECK(web_contents_) as well? Done. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:105: return prerender::PrerenderContents::FromWebContents(web_contents_) != On 2016/11/29 15:54:39, droger wrote: > optional nit, just to fit on 80 columns: remove "!= nullptr" Done. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3285: "Prerender.websame_PrefetchTTFCP.Warm.Cacheable.Visible", 1654, 1); On 2016/11/29 15:54:39, droger wrote: > It may be nice to have base::TimeDelta::FromMilliseconds(2654) and > base::Time::FromDoubleT(1) defined as constants, and compute 1654 as the > difference between the two. > > Here and in other tests. That's not quite true, however: this is the lower edge of the bucket the timestamp ends up in. I set the times so that we end up right on the edge, under the theory that would catch more errors, but it seems misleading to claim that bucket = paint - start time. There seems to be no good GetBucketLowerBound function here which would make the calculation you suggest accurate. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:72: virtual void OnFirstContentfulPaint() = 0; On 2016/11/29 15:54:39, droger wrote: > Leak: Add a empty virtual destructor (or pure). Done. https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:642: std::vector<std::unique_ptr<PrerenderManagerObserver>> observers_; On 2016/11/29 15:54:40, droger wrote: > Nit: consider using base::ObserverList. > > Note it has a slightly different behavior because it does not take ownership > though. Yeah, the ownership issue seemed more convenient to me to do this way. If there's a hard rule for observer lists, thought, I can work around it.
thanks, lgtm https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:642: std::vector<std::unique_ptr<PrerenderManagerObserver>> observers_; On 2016/11/29 16:39:45, mattcary wrote: > On 2016/11/29 15:54:40, droger wrote: > > Nit: consider using base::ObserverList. > > > > Note it has a slightly different behavior because it does not take ownership > > though. > > Yeah, the ownership issue seemed more convenient to me to do this way. If > there's a hard rule for observer lists, thought, I can work around it. I don't think there is a hard rule. ObserverList has just some nice features like Observers being able to remove themselves from within an observer call and such. This is not needed here (for now at least), I'm fine with the current code.
just responding to a thread, looking closer soon https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:642: std::vector<std::unique_ptr<PrerenderManagerObserver>> observers_; On 2016/11/29 16:46:18, droger wrote: > On 2016/11/29 16:39:45, mattcary wrote: > > On 2016/11/29 15:54:40, droger wrote: > > > Nit: consider using base::ObserverList. > > > > > > Note it has a slightly different behavior because it does not take ownership > > > though. > > > > Yeah, the ownership issue seemed more convenient to me to do this way. If > > there's a hard rule for observer lists, thought, I can work around it. > > I don't think there is a hard rule. ObserverList has just some nice features > like Observers being able to remove themselves from within an observer call and > such. > > This is not needed here (for now at least), I'm fine with the current code. I think it is good to not use the ObserverList here, because it makes it clear observers do *not* remove themselves. The tricky lifetime issues are .1% less tricky?
https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/280001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:642: std::vector<std::unique_ptr<PrerenderManagerObserver>> observers_; On 2016/11/29 16:51:17, pasko wrote: > On 2016/11/29 16:46:18, droger wrote: > > On 2016/11/29 16:39:45, mattcary wrote: > > > On 2016/11/29 15:54:40, droger wrote: > > > > Nit: consider using base::ObserverList. > > > > > > > > Note it has a slightly different behavior because it does not take > ownership > > > > though. > > > > > > Yeah, the ownership issue seemed more convenient to me to do this way. If > > > there's a hard rule for observer lists, thought, I can work around it. > > > > I don't think there is a hard rule. ObserverList has just some nice features > > like Observers being able to remove themselves from within an observer call > and > > such. > > > > This is not needed here (for now at least), I'm fine with the current code. > > I think it is good to not use the ObserverList here, because it makes it clear > observers do *not* remove themselves. The tricky lifetime issues are .1% less > tricky? Good point, sgtm
toplevel: can you update the issue description to be more acceptable to a general chrome hacker? here are good recommendations: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
Description was changed from ========== Perceived FCP for prerender. BUG=657762 ========== to ========== Adds first contentful paint histograms for prerender, using perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762 ==========
On 2016/11/29 16:54:50, pasko wrote: > toplevel: can you update the issue description to be more acceptable to a > general chrome hacker? > > here are good recommendations: > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... Oops, done.
On 2016/11/29 17:00:54, mattcary wrote: > On 2016/11/29 16:54:50, pasko wrote: > > toplevel: can you update the issue description to be more acceptable to a > > general chrome hacker? > > > > here are good recommendations: > > > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... > > Oops, done. ... and nit: "Prerender: Add first contentful paint histograms" would be a better title line than "Adds first contentful paint histograms for prerender, using perceived" sorry ;)
Description was changed from ========== Adds first contentful paint histograms for prerender, using perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762 ========== to ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762 ==========
On 2016/11/29 19:53:04, pasko wrote: > On 2016/11/29 17:00:54, mattcary wrote: > > On 2016/11/29 16:54:50, pasko wrote: > > > toplevel: can you update the issue description to be more acceptable to a > > > general chrome hacker? > > > > > > here are good recommendations: > > > > > > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... > > > > Oops, done. > > ... and nit: "Prerender: Add first contentful paint histograms" would be a > better title line than "Adds first contentful paint histograms for prerender, > using perceived" > > sorry ;) done
Description was changed from ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762 ========== to ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762,667243 ==========
https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:318: GetHistogramName(origin, IsOriginWash(), "PerceivedTTFCPRecorded") + this suffix is not mentioned in histograms.xml https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. does this sentence refer to some actions being done by the code below to mitigate the situation? If so, which is it? https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:1198: if (it->url == url) { This matches only the start URL, while Prerender swap-in also takes care of matching session_storage_namespace (roughly for multi-profile), and alias_urls_. The effect of the former is probably small, but the latter may introduce bias, since it may record metrics only for a subset of prerendered+used pages. https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:242: base::TimeTicks ticks); Perhaps it is worth explaining in the comment why this is not a TimeDelta as the two methods above have. https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:542: // Get infromation associated with a possible prefetch of |url|. nit: s/infromation/information/ https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48582: +<histogram name="Prerender.PrefetchTTFCP" units="ms"> I think "PrefetchTTFCP for Prerender" is a more confusing concept than "PerceivedTTFCP for Prerender". Did you consider "PerceivedTTFCP" as a name to be sending a more confusing message? https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48587: + Time to first contentful paint for navigations related to prefetch Please expand this summary more on where the timer starts. https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48588: + (including prerender, no-state prefetch, and associated experiments). Please expand what a TTFCP of a prefetch is. That's the only place where it is going to be documented in code, right?
https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:318: GetHistogramName(origin, IsOriginWash(), "PerceivedTTFCPRecorded") + On 2016/12/12 17:32:06, pasko wrote: > this suffix is not mentioned in histograms.xml Oops, done. What happens if the histogram is missing in histograms.xml? This should have been generated by the tests, and nothing failed---it seems like a bug that histograms.xml isn't validated. https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. On 2016/12/12 17:32:06, pasko wrote: > does this sentence refer to some actions being done by the code below to > mitigate the situation? If so, which is it? It explains the following line, where the origin retreived from GetPrefetchInformation is not used (because it is not relevant to the prerender). https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:1198: if (it->url == url) { On 2016/12/12 17:32:06, pasko wrote: > This matches only the start URL, while Prerender swap-in also takes care of > matching session_storage_namespace (roughly for multi-profile), and alias_urls_. > The effect of the former is probably small, but the latter may introduce bias, > since it may record metrics only for a subset of prerendered+used pages. Hmm. For prefetch (which is the context this was originally written), wouldn't that also be an issue, because the cache is per-profile? alias_urls_ seem to be only tracked in the PrerenderContents? So it will not be possible to deal with alias_urls_ for nostate prefetches? If that's true, since the point of these histograms is to compare for the experiment, we will want the prerender and nostate prefetch bias to be the same. WDYT? https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:242: base::TimeTicks ticks); On 2016/12/12 17:32:06, pasko wrote: > Perhaps it is worth explaining in the comment why this is not a TimeDelta as the > two methods above have. Isn't that what "The FCP ticks is in absolute time..." explains? https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:542: // Get infromation associated with a possible prefetch of |url|. On 2016/12/12 17:32:06, pasko wrote: > nit: s/infromation/information/ Done. https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48582: +<histogram name="Prerender.PrefetchTTFCP" units="ms"> On 2016/12/12 17:32:06, pasko wrote: > I think "PrefetchTTFCP for Prerender" is a more confusing concept than > "PerceivedTTFCP for Prerender". Did you consider "PerceivedTTFCP" as a name to > be sending a more confusing message? PerceivedTTFPC isn't accurate when it's used for NoStatePrefetch. Since the semantics are very subtle, it seems most straightforward to make it explicit that these are histograms tuned mostly for the prefetch experiments. https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48587: + Time to first contentful paint for navigations related to prefetch On 2016/12/12 17:32:06, pasko wrote: > Please expand this summary more on where the timer starts. Done. https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48588: + (including prerender, no-state prefetch, and associated experiments). On 2016/12/12 17:32:06, pasko wrote: > Please expand what a TTFCP of a prefetch is. That's the only place where it is > going to be documented in code, right? That should be done in the PrerenderPrefetchAge histogram_suffixes I believe---can you confirm that that documentation propagates up to what's visible in UMA?
I still do not fully understand the structure of the new histograms. What could help speeding up this review: * more detailed issue description on what histograms are being introduced and how they are implemented with a high-level description * more detailed section in the design doc how the histograms will be used to make the study conclusive * talk with a whiteboard https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:318: GetHistogramName(origin, IsOriginWash(), "PerceivedTTFCPRecorded") + On 2016/12/12 21:14:54, mattcary wrote: > On 2016/12/12 17:32:06, pasko wrote: > > this suffix is not mentioned in histograms.xml > > Oops, done. > > What happens if the histogram is missing in histograms.xml? This should have > been generated by the tests, and nothing failed---it seems like a bug that > histograms.xml isn't validated. Yeah, I guess we could get some histograms verified against histograms.xml in debug mode, but it was not ever done afaict. So defining metrics that we rely on to make major decisions is like shooting in the dark. https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. On 2016/12/12 21:14:54, mattcary wrote: > On 2016/12/12 17:32:06, pasko wrote: > > does this sentence refer to some actions being done by the code below to > > mitigate the situation? If so, which is it? > > It explains the following line, where the origin retreived from > GetPrefetchInformation is not used (because it is not relevant to the > prerender). Makes sense now, not sure why it was not clear before. Thanks. https://codereview.chromium.org/2423383002/diff/300001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:1198: if (it->url == url) { On 2016/12/12 21:14:54, mattcary wrote: > On 2016/12/12 17:32:06, pasko wrote: > > This matches only the start URL, while Prerender swap-in also takes care of > > matching session_storage_namespace (roughly for multi-profile), and > alias_urls_. > > The effect of the former is probably small, but the latter may introduce bias, > > since it may record metrics only for a subset of prerendered+used pages. > > Hmm. For prefetch (which is the context this was originally written), wouldn't > that also be an issue, because the cache is per-profile? > > alias_urls_ seem to be only tracked in the PrerenderContents? So it will not be > possible to deal with alias_urls_ for nostate prefetches? > > If that's true, since the point of these histograms is to compare for the > experiment, we will want the prerender and nostate prefetch bias to be the same. > WDYT? Oh, all prefetches_ are biased indeed. Well, one way is to say that multi-profile is small (pointing at some metric I did not find yet). If that's easier than proper prefetches_ (I think it is), then let's go for it. https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48582: +<histogram name="Prerender.PrefetchTTFCP" units="ms"> On 2016/12/12 21:14:55, mattcary wrote: > On 2016/12/12 17:32:06, pasko wrote: > > I think "PrefetchTTFCP for Prerender" is a more confusing concept than > > "PerceivedTTFCP for Prerender". Did you consider "PerceivedTTFCP" as a name to > > be sending a more confusing message? > > PerceivedTTFPC isn't accurate when it's used for NoStatePrefetch. Since the > semantics are very subtle, it seems most straightforward to make it explicit > that these are histograms tuned mostly for the prefetch experiments. So you are saying that it isn't accurate for NoStatePrefetch, but at the same time it will be used for prefetch experiments? Did you mean "it is not accurate for non-prefetch uses"? Then how are we supposed to compare it with Prerender? I am puzzled. https://codereview.chromium.org/2423383002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48588: + (including prerender, no-state prefetch, and associated experiments). On 2016/12/12 21:14:55, mattcary wrote: > On 2016/12/12 17:32:06, pasko wrote: > > Please expand what a TTFCP of a prefetch is. That's the only place where it is > > going to be documented in code, right? > > That should be done in the PrerenderPrefetchAge histogram_suffixes I > believe---can you confirm that that documentation propagates up to what's > visible in UMA? The documentation for the suffix would be concatenated with the base documentation. However, descriptions of those suffixes are pretty slim. https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49328: +<histogram name="Prerender.PerceivedTTFCPRecorded" enum="BooleanEnabled"> please make another boolean https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49372: + navigation-to-FCP; for prerender this is the swapped-in prerender navigation please explain what "swapped-in prerender navigation" means
https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49328: +<histogram name="Prerender.PerceivedTTFCPRecorded" enum="BooleanEnabled"> On 2016/12/15 19:37:51, pasko wrote: > please make another boolean Can you be more specific? https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49372: + navigation-to-FCP; for prerender this is the swapped-in prerender navigation On 2016/12/15 19:37:51, pasko wrote: > please explain what "swapped-in prerender navigation" means Done.
In response to an offline conversation: If a prerender is canceled, then we will still record a prefetch-appropriate TTFCP for a subsequent load to the page, but in sort of a weird way. First, by "prefetch-appropriate" I mean we will emit a PrefetchTTFCP.*.* histogram with all the variations (prefetch age, hidden/visible, etc) correct. Second, by weird way, I mean that if a web contents is created that is not a prerender, then we attach a NoStatePrefetchPageLoadMetricsObserver and (since prerender_manager->prefetches_ is updated appropriately whether or not NoState is enabled), this observer will correctly record the FCP. I say that's weird because NoState is not actually active here, so having an observer prefixed "NoState" is unobvious to me. In summary: it seems to work, and (in a future CL rather than now?) we should probably tweak the naming of the observer.
I started looking at tests, a few first ones make total sense, but the infrastructure for them seemed more verbose than necessary. Hence a few proposals to refactor it. No functionality change proposals in this iteration, will continue looking at the tests in parallel. https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49328: +<histogram name="Prerender.PerceivedTTFCPRecorded" enum="BooleanEnabled"> On 2016/12/16 10:03:03, mattcary wrote: > On 2016/12/15 19:37:51, pasko wrote: > > please make another boolean > > Can you be more specific? Oh, oops, that was indeed a bit cryptic. This is BooleanEnabled, which is defined as: <enum name="BooleanEnabled" type="int"> <int value="0" label="Disabled"/> <int value="1" label="Enabled"/> </enum> I proposed another kind of boolean: <enum name="BooleanRecorded" type="int"> <int value="0" label="Not recorded"/> <int value="1" label="Recorded"/> </enum> https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:44: bool was_hidden_; // The page went to background while rendering. I am wondering whether there is a mechanism to detect that chrome itself went to background while prerender was in progress. Do we want to add these as .Hidden.? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:903: void SetMidLoadClockAdvance(base::SimpleTestTickClock* clock, This is used only in one test, which is not a good excuse to increase the amount of state in the fixture. If Advance absolutely needs to be done inside PrerenderTestURLImpl (which does not seem obviously necessary), then something like SetAfterPrerenderLoadCallback() would do it and keep the state controlled enough in the test. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:1087: // Prerendered load. The Simple/Prerender tests have little in common, any reason not to make it two separate tests? maybe it tries to cover some tricky state during the transition and renderer reuse? Would be nice to add a comment in such case. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3308: DisableJavascriptCalls(); We are not expecting swap to succeed on this prerender, so it is not obvious why checking DidPrerenderPass() should be disabled. Please add a comment to explain it. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.h:144: static std::string FirstContentfulPaintHiddenName(bool was_hidden); this is not used outside prerender_histograms.cc, please move into the .cc and anonymous namespace https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:352: void AddObserver(std::unique_ptr<PrerenderManagerObserver> observer); This method is used only for testing afaict. If it is the case, then better to make it explicit that the observer logic is not necessary for production use. Unless there is a plan of using the observer for something else, let me know if this is the case. Possibilities, not mutually exclusive: 1. AddObserverForTesting and correspondingly testing_observers_ 2. Avoid managing ownership of the observers in PrerenderManager. Observers seem to be needed only when we wait for OnFirstContentfulPaint() in the observers, after that wait the observer is not needed, so the ownership could be managed fully in tests, avoiding complications in the critical pieces of code. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:260: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:392: using PrerenderPageLoadMetricsObserver::PrerenderPageLoadMetricsObserver; thanks for letting me know about inherited constructors :) https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:395: SetNavigationStartTicksForTesting(ticks); The only thing this class does is forwarding to this protected method, which complicates reading the callsites. Can we make PrerenderPageLoadMetricsObserver::SetNavigationStartTicksForTesting() public instead? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:397: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49380: + Whether perceived TTFCP was recorded successfully. If true, there is an It is still a little confusing to see a reference to "perceived TTFCP" without having Perceived.*TTFCP anywhere. Should we rename it to Prerender.PrefetchTTFCPRecorded and update the description accordingly? Or is it more like Prerender.TTFCPRecorded? Both of these variants sound better to me. P.S.: yeah, pasko@ nitpicky about naming things, and not mentioning it during offline conversation, sorry about that last part :/
https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49328: +<histogram name="Prerender.PerceivedTTFCPRecorded" enum="BooleanEnabled"> On 2016/12/20 19:33:33, pasko wrote: > On 2016/12/16 10:03:03, mattcary wrote: > > On 2016/12/15 19:37:51, pasko wrote: > > > please make another boolean > > > > Can you be more specific? > > Oh, oops, that was indeed a bit cryptic. > > This is BooleanEnabled, which is defined as: > > <enum name="BooleanEnabled" type="int"> > <int value="0" label="Disabled"/> > <int value="1" label="Enabled"/> > </enum> > > I proposed another kind of boolean: > > <enum name="BooleanRecorded" type="int"> > <int value="0" label="Not recorded"/> > <int value="1" label="Recorded"/> > </enum> Done. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:44: bool was_hidden_; // The page went to background while rendering. On 2016/12/20 19:33:34, pasko wrote: > I am wondering whether there is a mechanism to detect that chrome itself went to > background while prerender was in progress. Do we want to add these as .Hidden.? No idea. I'll ping Bryan. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:903: void SetMidLoadClockAdvance(base::SimpleTestTickClock* clock, On 2016/12/20 19:33:34, pasko wrote: > This is used only in one test, which is not a good excuse to increase the amount > of state in the fixture. If Advance absolutely needs to be done inside > PrerenderTestURLImpl (which does not seem obviously necessary), then something > like SetAfterPrerenderLoadCallback() would do it and keep the state controlled > enough in the test. This is a text fixture only used in this file (ie, not in prerender_test_utils). Callbacks seem to have unnecessary generality and are much more difficult to read, which especially for tests is the wrong way to go. We need to be able to control the clock while in the middle of PrerenderTestURLImpl in order to time out the prerender, as the timeout occurs during the periodic cleanup that happens in the WaitForStop; otherwise the prerender will continue and we'll get a first contentful paint which we don't want for this test. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:1087: // Prerendered load. On 2016/12/20 19:33:34, pasko wrote: > The Simple/Prerender tests have little in common, any reason not to make it two > separate tests? maybe it tries to cover some tricky state during the transition > and renderer reuse? Would be nice to add a comment in such case. Done. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3308: DisableJavascriptCalls(); On 2016/12/20 19:33:34, pasko wrote: > We are not expecting swap to succeed on this prerender, so it is not obvious why > checking DidPrerenderPass() should be disabled. Please add a comment to explain > it. Done, here and below. It's not the swap bit, it's the fact that we don't wait for the load. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.h:144: static std::string FirstContentfulPaintHiddenName(bool was_hidden); On 2016/12/20 19:33:34, pasko wrote: > this is not used outside prerender_histograms.cc, please move into the .cc and > anonymous namespace Done. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:260: }; On 2016/12/20 19:33:34, pasko wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:392: using PrerenderPageLoadMetricsObserver::PrerenderPageLoadMetricsObserver; On 2016/12/20 19:33:34, pasko wrote: > thanks for letting me know about inherited constructors :) Ack I think it was actually from digit@ and notes from his C++ class... https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:395: SetNavigationStartTicksForTesting(ticks); On 2016/12/20 19:33:34, pasko wrote: > The only thing this class does is forwarding to this protected method, which > complicates reading the callsites. Can we make > PrerenderPageLoadMetricsObserver::SetNavigationStartTicksForTesting() public > instead? It seems to be common to hide testing code with protected, but it doesn't matter much to me. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:397: }; On 2016/12/20 19:33:34, pasko wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49380: + Whether perceived TTFCP was recorded successfully. If true, there is an On 2016/12/20 19:33:34, pasko wrote: > It is still a little confusing to see a reference to "perceived TTFCP" without > having Perceived.*TTFCP anywhere. Should we rename it to > Prerender.PrefetchTTFCPRecorded and update the description accordingly? Or is it > more like Prerender.TTFCPRecorded? Both of these variants sound better to me. > > P.S.: yeah, pasko@ nitpicky about naming things, and not mentioning it during > offline conversation, sorry about that last part :/ This is only active for perceived FCP. Calling it PrefetchTTFCPRecorded would seem to imply that it should be incremented for all PrefetchTTFCP, which is not true.
https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:44: bool was_hidden_; // The page went to background while rendering. On 2016/12/21 at 17:08:58, mattcary wrote: > On 2016/12/20 19:33:34, pasko wrote: > > I am wondering whether there is a mechanism to detect that chrome itself went to > > background while prerender was in progress. Do we want to add these as .Hidden.? > > No idea. I'll ping Bryan. OnHidden is a per-tab signal. We don't really have an overall browser signal indicating whether the browser gets foregrounded / backgrounded, but we could add one. We do have the FlushMetricsOnAppEnterBackground callback, which is invoked on Android when the app goes into the background. This is mostly provided so we can persist any buffered metrics in this callback, since Chrome may be killed at any point without subsequent notification after this method is invoked (this gets invoked as part of the Activity.onPause flow when we don't have any more activities in the foreground).
still looking at histogram naming and tests https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:44: bool was_hidden_; // The page went to background while rendering. On 2016/12/21 17:56:31, Bryan McQuade wrote: > On 2016/12/21 at 17:08:58, mattcary wrote: > > On 2016/12/20 19:33:34, pasko wrote: > > > I am wondering whether there is a mechanism to detect that chrome itself > went to > > > background while prerender was in progress. Do we want to add these as > .Hidden.? > > > > No idea. I'll ping Bryan. > > OnHidden is a per-tab signal. > > We don't really have an overall browser signal indicating whether the browser > gets foregrounded / backgrounded, but we could add one. > > We do have the FlushMetricsOnAppEnterBackground callback, which is invoked on > Android when the app goes into the background. This is mostly provided so we can > persist any buffered metrics in this callback, since Chrome may be killed at any > point without subsequent notification after this method is invoked (this gets > invoked as part of the Activity.onPause flow when we don't have any more > activities in the foreground). To me this suggests that we do not worry about onPause in metrics for Android, hence we probably should not worry here for Prerender TTFCP. Is that a close enough interpretation? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:903: void SetMidLoadClockAdvance(base::SimpleTestTickClock* clock, On 2016/12/21 17:08:58, mattcary wrote: > On 2016/12/20 19:33:34, pasko wrote: > > This is used only in one test, which is not a good excuse to increase the > amount > > of state in the fixture. If Advance absolutely needs to be done inside > > PrerenderTestURLImpl (which does not seem obviously necessary), then something > > like SetAfterPrerenderLoadCallback() would do it and keep the state controlled > > enough in the test. > > This is a text fixture only used in this file (ie, not in prerender_test_utils). Yes, but the class is also about 500 lines of code, which makes it preferable to push the state out of it into the individual tests that need it. The PrerenderTestURLImpl() is already a bit too complex, hard to always keep in mind all the things it does. > Callbacks seem to have unnecessary generality and are much more difficult to > read, which especially for tests is the wrong way to go. I agree that callbacks are not good here, I was hoping that it was not necessary to advance time in the middle. > We need to be able to control the clock while in the middle of > PrerenderTestURLImpl in order to time out the prerender, as the timeout occurs > during the periodic cleanup that happens in the WaitForStop; otherwise the > prerender will continue and we'll get a first contentful paint which we don't > want for this test. Ah, this was to be able to timeout without FCP! Makes sense. Thanks! Hm, is the ultimate goal to test with FINAL_STATUS_TIMED_OUT? If the goal is to test what happens if a prerender does not reach FCP, then other final statuses should work without complications of intercepting the clock in the middle of the test, like: FINAL_STATUS_UNSUPPORTED_SCHEME. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3291: timing.navigation_start = base::Time::FromDoubleT(1); this number does not contribute to any of the expectations in the test, right? It was not obvious, and I was wondering of the reasons to initialize it from such a distinctive floating point value. Maybe make it clear that it is something like kUnusedTime? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3363: page_load_metrics::PageLoadTiming timing; this part seems to repeat what the previous test did. The name of the test suggests that we are testing here that timed out prerenders would not record the histograms. But we cannot check it because we are not setting the observer. Then idea #2: we might be testing that histograms are OK on after-timeout prerenders. Is there a reason to suspect that the mechanics would be significantly different from the ones for CANCELLED? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3423: // perceived time is not set and so update the following histogram. I was confused by this comment. The notion of perceived FCP is internal to PrerenderManager, while it receives request to record FCP from outside. So it should not 'detect' that 'perceived time' is set or not, it records it or avoids recording it depending on the situation, right? I think it would be clearer to say that the PrerenderManager observes that FCP is recorded without webcontents swap (since it is done by observer), so it records that it did not record perceived time. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. s/may not be/is not/ https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:612: false, was_hidden); this repetition with changing one magic boolean can be explained better like this: base::TimeTicks last_swap = tab_helper->last_swap(); bool perceived_fcp_recorded = false; if (!last_swap.is_null() && !first_contentful_paint.is_null()) { perceived_fcp_recorded = true; histograms_->RecordPrefetchFirstContentfulPaintTime( tab_helper->origin(), is_no_store, was_hidden, first_contentful_paint - last_swap, prefetch_age); } histograms_->RecordPerceivedFirstContentfulPaintStatus( tab_helper->origin(), perceived_fcp_recorded, was_hidden); https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_tab_helper.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.h:105: base::TimeTicks last_swap_; My reading of the code suggests that: 1. Instances of this class do not travel from one WebContents to another, and have only one instance per WebContents. 2. PrerenderTabHelper::PrerenderSwappedIn() gets recorded on the tab helper for the _new_ WebContents. Hence, a given PrerenderTabHelper can observe a swap only once. This makes last_swap_ is confusing and is overly alarming. How about swap_time_ticks_? https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:395: SetNavigationStartTicksForTesting(ticks); On 2016/12/21 17:08:59, mattcary wrote: > On 2016/12/20 19:33:34, pasko wrote: > > The only thing this class does is forwarding to this protected method, which > > complicates reading the callsites. Can we make > > PrerenderPageLoadMetricsObserver::SetNavigationStartTicksForTesting() public > > instead? > > It seems to be common to hide testing code with protected, but it doesn't matter > much to me. Ah, now I see where it is coming from. I think it is an important feature of the suffix "ForTesting" to avoid extra overhead of subclassing. https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49380: + Whether perceived TTFCP was recorded successfully. If true, there is an On 2016/12/21 17:08:59, mattcary wrote: > On 2016/12/20 19:33:34, pasko wrote: > > It is still a little confusing to see a reference to "perceived TTFCP" without > > having Perceived.*TTFCP anywhere. Should we rename it to > > Prerender.PrefetchTTFCPRecorded and update the description accordingly? Or is > it > > more like Prerender.TTFCPRecorded? Both of these variants sound better to me. > > > > P.S.: yeah, pasko@ nitpicky about naming things, and not mentioning it during > > offline conversation, sorry about that last part :/ > > This is only active for perceived FCP. Calling it PrefetchTTFCPRecorded would > seem to imply that it should be incremented for all PrefetchTTFCP, which is not > true. The word "perceived" does not obviously translate to something equivalent to "For Prerender, but also only if swap-in happened". I had to read the code to figure this out. For people not familiar with prerender, it will be all cryptic. How about "Recorded for each Prerendered WebContents that reaches the First Contentful Paint. Records True for prerenders that were _used_, False otherwise.", which would be named like "Prerender.IsSwappedInWhenReachedTTFCP". It could be I am still missing a few possibilities. Please correct me. https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:611: if (!last_swap.is_null() && !first_contentful_paint.is_null()) { this gets recorded from a notification for FCP. In which cases do we hit first_contentful_paint.is_null()? Is it only for testing?
https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:44: bool was_hidden_; // The page went to background while rendering. On 2016/12/21 18:39:08, pasko wrote: > On 2016/12/21 17:56:31, Bryan McQuade wrote: > > On 2016/12/21 at 17:08:58, mattcary wrote: > > > On 2016/12/20 19:33:34, pasko wrote: > > > > I am wondering whether there is a mechanism to detect that chrome itself > > went to > > > > background while prerender was in progress. Do we want to add these as > > .Hidden.? > > > > > > No idea. I'll ping Bryan. > > > > OnHidden is a per-tab signal. > > > > We don't really have an overall browser signal indicating whether the browser > > gets foregrounded / backgrounded, but we could add one. > > > > We do have the FlushMetricsOnAppEnterBackground callback, which is invoked on > > Android when the app goes into the background. This is mostly provided so we > can > > persist any buffered metrics in this callback, since Chrome may be killed at > any > > point without subsequent notification after this method is invoked (this gets > > invoked as part of the Activity.onPause flow when we don't have any more > > activities in the foreground). > > To me this suggests that we do not worry about onPause in metrics for Android, > hence we probably should not worry here for Prerender TTFCP. Is that a close > enough interpretation? sgtm https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:903: void SetMidLoadClockAdvance(base::SimpleTestTickClock* clock, > Hm, is the ultimate goal to test with FINAL_STATUS_TIMED_OUT? If the goal is to > test what happens if a prerender does not reach FCP, then other final statuses > should work without complications of intercepting the clock in the middle of the > test, like: FINAL_STATUS_UNSUPPORTED_SCHEME. yes, exactly, hence the test name. It seems that the case of a page starting a prefetch, and then timing out due to the main resource or redirects or whatever stalling on a flakey mobile network, is practical and likely, and thus a test for this case specifically is a good idea. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3291: timing.navigation_start = base::Time::FromDoubleT(1); On 2016/12/21 18:39:08, pasko wrote: > this number does not contribute to any of the expectations in the test, right? > It was not obvious, and I was wondering of the reasons to initialize it from > such a distinctive floating point value. Maybe make it clear that it is > something like kUnusedTime? Actually, it's just to initialize it to a non-null time. Added comment. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3363: page_load_metrics::PageLoadTiming timing; On 2016/12/21 18:39:08, pasko wrote: > this part seems to repeat what the previous test did. The name of the test > suggests that we are testing here that timed out prerenders would not record the > histograms. But we cannot check it because we are not setting the observer. > > Then idea #2: we might be testing that histograms are OK on after-timeout > prerenders. Is there a reason to suspect that the mechanics would be > significantly different from the ones for CANCELLED? Yes. The point is that parts of the page load timing are set in different stages of the prerenderer lifetime, and we want to make sure that we correctly detect & ignore those partial loads. Timing out and cancelling are different mechanisms. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:3423: // perceived time is not set and so update the following histogram. On 2016/12/21 18:39:09, pasko wrote: > I was confused by this comment. The notion of perceived FCP is internal to > PrerenderManager, while it receives request to record FCP from outside. So it > should not 'detect' that 'perceived time' is set or not, it records it or avoids > recording it depending on the situation, right? > > I think it would be clearer to say that the PrerenderManager observes that FCP > is recorded without webcontents swap (since it is done by observer), so it > records that it did not record perceived time. Done. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. On 2016/12/21 18:39:09, pasko wrote: > s/may not be/is not/ No, may not be. Sometimes it is the same. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:612: false, was_hidden); On 2016/12/21 18:39:09, pasko wrote: > this repetition with changing one magic boolean can be explained better like > this: > > base::TimeTicks last_swap = tab_helper->last_swap(); > bool perceived_fcp_recorded = false; > if (!last_swap.is_null() && !first_contentful_paint.is_null()) { > perceived_fcp_recorded = true; > histograms_->RecordPrefetchFirstContentfulPaintTime( > tab_helper->origin(), is_no_store, was_hidden, > first_contentful_paint - last_swap, prefetch_age); > } > histograms_->RecordPerceivedFirstContentfulPaintStatus( > tab_helper->origin(), perceived_fcp_recorded, was_hidden); Done. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_tab_helper.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_tab_helper.h:105: base::TimeTicks last_swap_; On 2016/12/21 18:39:09, pasko wrote: > My reading of the code suggests that: > > 1. Instances of this class do not travel from one WebContents to another, and > have only one instance per WebContents. > > 2. PrerenderTabHelper::PrerenderSwappedIn() gets recorded on the tab helper for > the _new_ WebContents. > > Hence, a given PrerenderTabHelper can observe a swap only once. This makes > last_swap_ is confusing and is overly alarming. How about swap_time_ticks_? "Alarming" seems a bit of a stretch, but sure. https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_test_utils.h:395: SetNavigationStartTicksForTesting(ticks); On 2016/12/21 18:39:09, pasko wrote: > On 2016/12/21 17:08:59, mattcary wrote: > > On 2016/12/20 19:33:34, pasko wrote: > > > The only thing this class does is forwarding to this protected method, which > > > complicates reading the callsites. Can we make > > > PrerenderPageLoadMetricsObserver::SetNavigationStartTicksForTesting() public > > > instead? > > > > It seems to be common to hide testing code with protected, but it doesn't > matter > > much to me. > > Ah, now I see where it is coming from. I think it is an important feature of the > suffix "ForTesting" to avoid extra overhead of subclassing. Acknowledged. https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49380: + Whether perceived TTFCP was recorded successfully. If true, there is an On 2016/12/21 18:39:09, pasko wrote: > On 2016/12/21 17:08:59, mattcary wrote: > > On 2016/12/20 19:33:34, pasko wrote: > > > It is still a little confusing to see a reference to "perceived TTFCP" > without > > > having Perceived.*TTFCP anywhere. Should we rename it to > > > Prerender.PrefetchTTFCPRecorded and update the description accordingly? Or > is > > it > > > more like Prerender.TTFCPRecorded? Both of these variants sound better to > me. > > > > > > P.S.: yeah, pasko@ nitpicky about naming things, and not mentioning it > during > > > offline conversation, sorry about that last part :/ > > > > This is only active for perceived FCP. Calling it PrefetchTTFCPRecorded would > > seem to imply that it should be incremented for all PrefetchTTFCP, which is > not > > true. > > The word "perceived" does not obviously translate to something equivalent to > "For Prerender, but also only if swap-in happened". I had to read the code to > figure this out. For people not familiar with prerender, it will be all cryptic. > > How about "Recorded for each Prerendered WebContents that reaches the First > Contentful Paint. Records True for prerenders that were _used_, False > otherwise.", which would be named like "Prerender.IsSwappedInWhenReachedTTFCP". > > It could be I am still missing a few possibilities. Please correct me. ptal https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:611: if (!last_swap.is_null() && !first_contentful_paint.is_null()) { On 2016/12/21 18:39:09, pasko wrote: > this gets recorded from a notification for FCP. In which cases do we hit > first_contentful_paint.is_null()? Is it only for testing? Maybe something goes wrong with the PageLoadMetricsObserver timing? Who knows, data can go bad in many ways. Even if we DCHECK'd this, we'd still want to avoid putting corrupt data into histogram: note that a null FCP time would mean that we'd record a negative bucket into the histogram which would cause all sorts of hilarity.
lgtm, thank you for addressing all the requests with patience, special thanks for tests, etc etc, you rock! Remaining are nits and questsions. I am still slightly unhappy about having "Perceived" in the histogram name, and the sloppy definition in the design doc, but since the definition in the histograms.xml is detailed enough now, I can tolerate those other slight disagreements. Thanks again! https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. On 2016/12/22 10:49:07, mattcary wrote: > On 2016/12/21 18:39:09, pasko wrote: > > s/may not be/is not/ > > No, may not be. Sometimes it is the same. Then extracting this bit of information from the code and the comment makes the comment not worth it IMO. It could be that I asked to put a clarifying comment to explain a little about |unused_origin|. Apologies in this case, it did not work well. https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:611: if (!last_swap.is_null() && !first_contentful_paint.is_null()) { On 2016/12/22 10:49:07, mattcary wrote: > On 2016/12/21 18:39:09, pasko wrote: > > this gets recorded from a notification for FCP. In which cases do we hit > > first_contentful_paint.is_null()? Is it only for testing? > > Maybe something goes wrong with the PageLoadMetricsObserver timing? Who knows, > data can go bad in many ways. Even if we DCHECK'd this, we'd still want to avoid > putting corrupt data into histogram: note that a null FCP time would mean that > we'd record a negative bucket into the histogram which would cause all sorts of > hilarity. OK, I started suspecting that I am crazy. Glad that we don't have the evidence coming from here. Then this lg. Lemme suggest to add DCHECK(!first_contentful_paint.is_null()) without removing the test for it before recording the histogram. Adds potential to catch it in testing, also some readability about us being unsure about this thing we depend on not going out of control. The effect would be small, up to you. https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:116: std::string FirstContentfulPaintHiddenName(bool was_hidden) { nit: const char* may be a tiny bit faster/economical, or may not be so https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:619: tab_helper->origin(), fcp_recorded, was_hidden); probably asking it the second time (sorry if so): Why do we have reasons to think that prior-to-swap FCP would be rare for prerenders? Does something in page load metrics guarantee that invisible webcontents do not register FCP? Is it a guarantee that is going to be maintained going forward or happens to be true today?
https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc (left): https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:325: TEST_F(MetricsWebContentsObserverTest, DontLogPrerender) { On 2016/11/24 09:07:42, mattcary wrote: > On 2016/11/23 18:25:46, Charlie Harrison(OOO Nov23-28) wrote: > > Do you think we could maintain a test like this (maybe as a browsertest), to > > ensure we aren't logging metrics for our core metrics during prerender? > > Sorry, dropped this. I started looking in, it's a little tricky, especially > since the prerender browsertests need to be done over in a different directory. > I could have a test that spot-checks some of the core metrics, but I think it > would be difficult to guarantee coverage without some explicit registry scheme. > Would that be enough? Sorry I missed this. I would be happy to test just a single critical metric like NavigationToFirstContentfulPaint. I think it would be fine for prerender browsertests to check this, or you could e.g. have a test which has <link rel='prerender'> in it.
https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/340001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:599: // The prefetch origin may not be relevant to the prerender. On 2016/12/22 18:22:00, pasko wrote: > On 2016/12/22 10:49:07, mattcary wrote: > > On 2016/12/21 18:39:09, pasko wrote: > > > s/may not be/is not/ > > > > No, may not be. Sometimes it is the same. > > Then extracting this bit of information from the code and the comment makes the > comment not worth it IMO. It could be that I asked to put a clarifying comment > to explain a little about |unused_origin|. Apologies in this case, it did not > work well. Oh, I see. Done. https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/360001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:611: if (!last_swap.is_null() && !first_contentful_paint.is_null()) { On 2016/12/22 18:22:00, pasko wrote: > On 2016/12/22 10:49:07, mattcary wrote: > > On 2016/12/21 18:39:09, pasko wrote: > > > this gets recorded from a notification for FCP. In which cases do we hit > > > first_contentful_paint.is_null()? Is it only for testing? > > > > Maybe something goes wrong with the PageLoadMetricsObserver timing? Who knows, > > data can go bad in many ways. Even if we DCHECK'd this, we'd still want to > avoid > > putting corrupt data into histogram: note that a null FCP time would mean that > > we'd record a negative bucket into the histogram which would cause all sorts > of > > hilarity. > > OK, I started suspecting that I am crazy. Glad that we don't have the evidence > coming from here. > > Then this lg. Lemme suggest to add DCHECK(!first_contentful_paint.is_null()) > without removing the test for it before recording the histogram. Adds potential > to catch it in testing, also some readability about us being unsure about this > thing we depend on not going out of control. The effect would be small, up to > you. Done. https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:116: std::string FirstContentfulPaintHiddenName(bool was_hidden) { On 2016/12/22 18:22:01, pasko wrote: > nit: const char* may be a tiny bit faster/economical, or may not be so Done. https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2423383002/diff/380001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:619: tab_helper->origin(), fcp_recorded, was_hidden); On 2016/12/22 18:22:01, pasko wrote: > probably asking it the second time (sorry if so): > > Why do we have reasons to think that prior-to-swap FCP would be rare for > prerenders? Does something in page load metrics guarantee that invisible > webcontents do not register FCP? Is it a guarantee that is going to be > maintained going forward or happens to be true today? Yes, according to Bryan, if a page is hidden then the FCP is delayed until visible--the rendering pipeline is suppressed, which means that when the page becomes visible again there's some amount of rendering that still needs to get done before the FCP. I have no idea about the future guarantee. At any rate, we expected Recorded=false to be a very small number; if it's not one of these assumptions is wrong.
On 2016/12/22 19:51:25, Charlie Harrison OOO Dec 15-21 wrote: > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc > (left): > > https://codereview.chromium.org/2423383002/diff/240001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:325: > TEST_F(MetricsWebContentsObserverTest, DontLogPrerender) { > On 2016/11/24 09:07:42, mattcary wrote: > > On 2016/11/23 18:25:46, Charlie Harrison(OOO Nov23-28) wrote: > > > Do you think we could maintain a test like this (maybe as a browsertest), to > > > ensure we aren't logging metrics for our core metrics during prerender? > > > > Sorry, dropped this. I started looking in, it's a little tricky, especially > > since the prerender browsertests need to be done over in a different > directory. > > I could have a test that spot-checks some of the core metrics, but I think it > > would be difficult to guarantee coverage without some explicit registry > scheme. > > Would that be enough? > > Sorry I missed this. I would be happy to test just a single critical metric like > NavigationToFirstContentfulPaint. I think it would be fine for prerender > browsertests to check this, or you could e.g. have a test which has <link > rel='prerender'> in it. Okay, I added this metric to the PrerenderBrowserTest.PageLoadMetrics* tests (previously I just had NavigationToFirstLayout). LMK.
mattcary@chromium.org changed reviewers: + isherman@chromium.org
isherman: please look at another prerender-related histograms.xml change (adding new histograms for upcoming experiment). Thanks
Thanks very much! Looks good just one question https://codereview.chromium.org/2423383002/diff/420001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/420001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:1104: "PageLoad.DocumentTiming.NavigationToFirstLayout", 1); Why is layout calculated for this page? A prerendered page should not log layout or paint metrics.
https://codereview.chromium.org/2423383002/diff/420001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/420001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:1104: "PageLoad.DocumentTiming.NavigationToFirstLayout", 1); On 2016/12/23 14:33:18, Charlie Harrison OOO Dec 15-21 wrote: > Why is layout calculated for this page? A prerendered page should not log layout > or paint metrics. s/calculated/logged
https://codereview.chromium.org/2423383002/diff/420001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2423383002/diff/420001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:1104: "PageLoad.DocumentTiming.NavigationToFirstLayout", 1); On 2016/12/23 14:33:18, Charlie Harrison OOO Dec 15-21 wrote: > Why is layout calculated for this page? A prerendered page should not log layout > or paint metrics. It's for the page that starts off the prerender: PrerenderTestURL loads a page containing a link rel=prerender and an empty body (chrome/test/data/prerender/prefetch_loader.html); this fires a NavigationToFirstLayout event but not a contentful paint event as the body is empty. There would be two layouts if the prerender logged one. That's confusing, I'll add a comment.
Thanks, LGTM
This CL adds quite a large number of histograms. That might be okay if you really need all of them, but please do keep in mind that histograms have costs associated with them, and consider whether you might not be able to measure this information in a more efficient way. https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (left): https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:486: : "NoStatePrefetchTTFCP.Cold"; It looks like you dropped "NoState" from this name. Could you please mark the corresponding histograms in histograms.xml as <obsolete>? https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:315: successful ? 1 : 0, 2); nit: Please use UmaHistogramBoolean from //base/histogram_functions.h. (In a separate CL, you might want to switch other uses of RecordHistogramEnum over to these shared functions as well.) https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:471: GetHistogramName(origin, IsOriginWash(), "Prerender.PrefetchAge"), Why did you drop "NoState" from this histogram name? I don't see a corresponding update in histograms.xml. https://codereview.chromium.org/2423383002/diff/440001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/440001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49688: + Whether the perceived TTFCP was recorded successfully for a prerendered It's probably helpful to clarify what TTFCP means, as you do in the other histogram description, below.
Thanks for your review. Sorry for not being clear about the context. For a more detailed description of our use of the histograms, see our doc (http://goo.gl/EJjTCM), especially the "Measurements" sections at https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5E... https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (left): https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:486: : "NoStatePrefetchTTFCP.Cold"; On 2016/12/27 22:46:06, Ilya Sherman (Away De.29-Ja.4) wrote: > It looks like you dropped "NoState" from this name. Could you please mark the > corresponding histograms in histograms.xml as <obsolete>? Done. I have left the obsolete histogram (both this and NoStatePrefetchAge) in histogram_suffixes, is that appropriate? https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:315: successful ? 1 : 0, 2); On 2016/12/27 22:46:06, Ilya Sherman (Away De.29-Ja.4) wrote: > nit: Please use UmaHistogramBoolean from //base/histogram_functions.h. (In a > separate CL, you might want to switch other uses of RecordHistogramEnum over to > these shared functions as well.) Done; created crbug/677257 so I don't drop switching over the other uses. https://codereview.chromium.org/2423383002/diff/440001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:471: GetHistogramName(origin, IsOriginWash(), "Prerender.PrefetchAge"), On 2016/12/27 22:46:06, Ilya Sherman (Away De.29-Ja.4) wrote: > Why did you drop "NoState" from this histogram name? I don't see a > corresponding update in histograms.xml. Oops, thanks. Updated histograms.xml to obsolete the old one and add this one. This histogram is no recording more than just NoState prefetch ages; it will also record effective prefetch age for prerender and non-prefetched loads as well (depending on the experiment branch). https://codereview.chromium.org/2423383002/diff/440001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2423383002/diff/440001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49688: + Whether the perceived TTFCP was recorded successfully for a prerendered On 2016/12/27 22:46:06, Ilya Sherman (Away De.29-Ja.4) wrote: > It's probably helpful to clarify what TTFCP means, as you do in the other > histogram description, below. Done.
Metrics LGTM, thanks.
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, droger@chromium.org, pasko@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2423383002/#ps460001 (title: "histogram comments")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, bmcquade@chromium.org, pasko@chromium.org, isherman@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2423383002/#ps480001 (title: "rebase")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, bmcquade@chromium.org, pasko@chromium.org, isherman@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2423383002/#ps500001 (title: "MSVC compilation 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": 500001, "attempt_start_ts": 1483351226623770,
"parent_rev": "6f9e55685c087686357596d726362e2b52d75cb0", "commit_rev":
"cde59ae2c6b26a97edefecf7dfc1e8fc90bd8896"}
Message was sent while issue was closed.
Description was changed from ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762,667243 ========== to ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762,667243 Review-Url: https://codereview.chromium.org/2423383002 ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762,667243 Review-Url: https://codereview.chromium.org/2423383002 ========== to ========== [Prerender] Add first contentful paint histograms Uses perceived FCP measured from prerender swap time when appropriate. Changes no state prefetch to use the same FCP histogram for easier experimentation. If a page is hidden during prerendering, the paint may be delayed in an unpredictable manner; therefore the histograms are split between hidden and visible. BUG=657762,667243 Committed: https://crrev.com/f564b5a80caa74b761951b2d9a6772eb80c473b0 Cr-Commit-Position: refs/heads/master@{#441056} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/f564b5a80caa74b761951b2d9a6772eb80c473b0 Cr-Commit-Position: refs/heads/master@{#441056} |
