Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782503004:
Prerender: clarify how FCP works in non-prerendered cases (Closed)
|
Created:
3 years, 8 months ago by pasko Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+prer_chromium.org, tburkard+watch_chromium.org, speed-metrics-reviews_chromium.org, loading-reviews+metrics_chromium.org, csharrison+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrerender: clarify how FCP works in non-prerendered cases
The only changes are in comments.
BUG=704911
TBR=bmcquade@chromium.org
Review-Url: https://codereview.chromium.org/2782503004
Cr-Commit-Position: refs/heads/master@{#466907}
Committed: https://chromium.googlesource.com/chromium/src/+/fbecc55ae2f4923fd2731391e33af6c37ff7ee2a
Patch Set 1 #
Total comments: 2
Patch Set 2 : s/it // #
Total comments: 2
Patch Set 3 : clarified the comment a little #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Prerender: clarify how FCP works in non-prerendered cases The only changes are in comments. BUG=704911 ========== to ========== Prerender: clarify how FCP works in non-prerendered cases The only changes are in comments. BUG=704911 ==========
pasko@chromium.org changed reviewers: + droger@chromium.org, mattcary@chromium.org
lgtm https://codereview.chromium.org/2782503004/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2782503004/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.h:206: // experimental group it such triggering makes the page prerendered, while in remove "it"
thank, David. Will wait for ack from Matt, and the other change this one depends on to avoid resolving conflicts, and a rubberstamp from Bryan (or .. I guess we are now encouraged to TBR documentation?) https://codereview.chromium.org/2782503004/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2782503004/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.h:206: // experimental group it such triggering makes the page prerendered, while in On 2017/03/28 16:50:51, droger wrote: > remove "it" Done.
https://codereview.chromium.org/2782503004/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2782503004/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:24: // bit to tell whether the page was recently eligible for prefetching with Which boolean bits in the metrics? The "bits" in the metrics are the origin, which is not boolean, the prefetching age (which is a tristate) and the main resource type and visibility which are bits but say nothing about prefetching eligibility, and are collected in this observer and not in the PrerenderManager. Anyway I think I see what you're getting at here, but this comment seems to confuse things, as well as being overly dependent on the current state of prerender. Perhaps something like the following is more accurate (I'm not totally happy with the wording but I think it's more accurate than your suggestion). This observer records metrics that track the eligibility of prefetch for a page. It combines information from the PrerenderManager about the prefetch origin and time between possible prefetch and page load with main resource cachability and page visibility at load time. This information is used no matter what prefetching mode is enabled, so that different experimental groups can be compared. This observer should not be attached to a prerendering WebContents, see PrerenderPageLoadMetricsObserver for that case. This observer will be used if prerendering is enabled, but the particular page load is not prerendered.
https://codereview.chromium.org/2782503004/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h (right): https://codereview.chromium.org/2782503004/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h:24: // bit to tell whether the page was recently eligible for prefetching with On 2017/03/29 11:12:17, mattcary wrote: > Which boolean bits in the metrics? The "bits" in the metrics are the origin, > which is not boolean, the prefetching age (which is a tristate) and the main > resource type and visibility which are bits but say nothing about prefetching > eligibility, and are collected in this observer and not in the PrerenderManager. Yes, there are these details. The sentence is only explaining why this observer should be attached to all webcontents except prerenders. It is because we are asking about the eligibility bit from PrerenderManager. We could ask it from somewhere else, but then the classes would look differently. I think this explanation is pretty important because the instantiation of this class has been my top source of confusion. It just explains one aspect of how to properly use this class. > Anyway I think I see what you're getting at here, but this comment seems to > confuse things, as well as being overly dependent on the current state of > prerender. Perhaps something like the following is more accurate (I'm not > totally happy with the wording but I think it's more accurate than your > suggestion). > > This observer records metrics that track the eligibility of prefetch for a page. > It combines information from the PrerenderManager about the prefetch origin and > time between possible prefetch and page load with main resource cachability and > page visibility at load time. This information is used no matter what > prefetching mode is enabled, so that different experimental groups can be > compared. I would like to clarify what it means to be enabled for both "prefetching" and "prerendering", that's why I mentioned specific group behaviors in the original comment. Things can be enabled, disabled at various levels, and when we say "enabled" it is not clear what level the phrase is referring to. I like your wording in the first part where it explains what information is combined, I'd like to take that. Can you rephrase without referring to "enabled" and "disabled"? > This observer should not be attached to a prerendering WebContents, see > PrerenderPageLoadMetricsObserver for that case. This observer will be used if > prerendering is enabled, but the particular page load is not prerendered. aren't we using this observer for simple load as well? I think this sentence can be interpreted in contradictory ways.
friendly ping :)
On 2017/04/12 12:50:15, pasko wrote: > friendly ping :) Sorry, I was waiting for you to come up with an alternate wording. It seemed like you understood the gist of my comment.
On 2017/04/12 12:59:52, mattcary wrote: > On 2017/04/12 12:50:15, pasko wrote: > > friendly ping :) > > Sorry, I was waiting for you to come up with an alternate wording. It seemed > like you understood the gist of my comment. Provided a new wording, PTAL.
The CQ bit was checked by pasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks!
lgtm
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org Link to the patchset: https://codereview.chromium.org/2782503004/#ps40001 (title: "clarified the comment a little")
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 pasko@chromium.org
Description was changed from ========== Prerender: clarify how FCP works in non-prerendered cases The only changes are in comments. BUG=704911 ========== to ========== Prerender: clarify how FCP works in non-prerendered cases The only changes are in comments. BUG=704911 TBR=bmcquade@chromium.org ==========
The CQ bit was checked by pasko@chromium.org
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": 1493103222378070,
"parent_rev": "8bb84356d2e79de6c4a88e323dc9c5d6a0df40c4", "commit_rev":
"fbecc55ae2f4923fd2731391e33af6c37ff7ee2a"}
Message was sent while issue was closed.
Description was changed from ========== Prerender: clarify how FCP works in non-prerendered cases The only changes are in comments. BUG=704911 TBR=bmcquade@chromium.org ========== to ========== Prerender: clarify how FCP works in non-prerendered cases The only changes are in comments. BUG=704911 TBR=bmcquade@chromium.org Review-Url: https://codereview.chromium.org/2782503004 Cr-Commit-Position: refs/heads/master@{#466907} Committed: https://chromium.googlesource.com/chromium/src/+/fbecc55ae2f4923fd2731391e33a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/fbecc55ae2f4923fd2731391e33a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
