Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512953002:
Prerender: Add FCP histograms to nostate browsertests. (Closed)
DescriptionPrerender: Add FCP histograms to nostate browsertests.
This adds a browsertest in a nostate prerender context to cover histograms added in browser/page_load_metrics.
BUG=657764
Committed: https://crrev.com/bfde2fdb4910e7628b949bc39afb3340d249ddd7
Cr-Commit-Position: refs/heads/master@{#441059}
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments #Patch Set 3 : rebase #
Depends on Patchset: Messages
Total messages: 17 (9 generated)
mattcary@chromium.org changed reviewers: + droger@chromium.org, pasko@chromium.org
I think the only possibly controversial thing in this CL is the firing of PrerenderManagerObserver::OnFirstContentfulPaint on both NoStatePrefetch FCP and Prerender PerceivedFCP, with no way to tell the difference between them. I don't think anything is misleading anywhere, and I'm loathe to add extra complexity for distinguishing when we don't actually have a use case for it.
the test code looks good, a couple of questions on prerender_manager https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:620: observer->OnFirstContentfulPaint(); can we notify observers just after recording histograms? The manipulations of prefetches_ above might be potentially non-instant, so in case observers ever get sensitive to precision, we would do our best here. I think this does not matter in practice, but looks safer and raises less questions. https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:642: for (auto& observer : observers_) { this moved too, does anything depend on firing it later? https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:142: PrefetchFromFile(kPrefetchPage, FINAL_STATUS_NOSTATE_PREFETCH_FINISHED); PrefetchFromFile waits for Stop (i.e. when all prefetch requests are already made), so request counters are not needed.
https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:620: observer->OnFirstContentfulPaint(); On 2016/11/18 15:56:56, pasko wrote: > can we notify observers just after recording histograms? The manipulations of > prefetches_ above might be potentially non-instant, so in case observers ever > get sensitive to precision, we would do our best here. I think this does not > matter in practice, but looks safer and raises less questions. SGTM. I moved it to the end of the function for a similar reasons, but your suggestion is more precise. Note that it now occurs to me that what we are really using these observers for is to wait for the FCP histograms, not actually the FCP. That only affects tests, though, and it seems like it might be confusing to emphasize that detail. https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:642: for (auto& observer : observers_) { On 2016/11/18 15:56:56, pasko wrote: > this moved too, does anything depend on firing it later? I moved this to be consistent with the other FCP (ie, at the end). At any rate, it happily matches your other suggestion. https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2512953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:142: PrefetchFromFile(kPrefetchPage, FINAL_STATUS_NOSTATE_PREFETCH_FINISHED); On 2016/11/18 15:56:56, pasko wrote: > PrefetchFromFile waits for Stop (i.e. when all prefetch requests are already > made), so request counters are not needed. Done.
lgtm
The CQ bit was checked by mattcary@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2423383002 Patch 120001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Description was changed from ========== Prerender: Add FCP histograms to nostate browsertests. BUG=657764 ========== to ========== Prerender: Add FCP histograms to nostate browsertests. This adds a browsertest in a nostate prerender context to cover histograms added in browser/page_load_metrics. BUG=657764 ==========
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2512953002/#ps40001 (title: "rebase")
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": 40001, "attempt_start_ts": 1483356755264550,
"parent_rev": "aee3b6b8f6737d75e59e21cb8678d9f27f98b8cf", "commit_rev":
"49c9211b9c2523e8d24e1e2e16fbee82c829453b"}
Message was sent while issue was closed.
Description was changed from ========== Prerender: Add FCP histograms to nostate browsertests. This adds a browsertest in a nostate prerender context to cover histograms added in browser/page_load_metrics. BUG=657764 ========== to ========== Prerender: Add FCP histograms to nostate browsertests. This adds a browsertest in a nostate prerender context to cover histograms added in browser/page_load_metrics. BUG=657764 Review-Url: https://codereview.chromium.org/2512953002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Prerender: Add FCP histograms to nostate browsertests. This adds a browsertest in a nostate prerender context to cover histograms added in browser/page_load_metrics. BUG=657764 Review-Url: https://codereview.chromium.org/2512953002 ========== to ========== Prerender: Add FCP histograms to nostate browsertests. This adds a browsertest in a nostate prerender context to cover histograms added in browser/page_load_metrics. BUG=657764 Committed: https://crrev.com/bfde2fdb4910e7628b949bc39afb3340d249ddd7 Cr-Commit-Position: refs/heads/master@{#441059} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bfde2fdb4910e7628b949bc39afb3340d249ddd7 Cr-Commit-Position: refs/heads/master@{#441059} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
