[LCP] Add UKM indicating that the largest candidate is animated. [chromium/src : main]
Yoav Weiss (Gerrit)
Attention is currently required from: Nicolás Peña Moreno, Daniel Cheng, Annie Sullivan, Robert Kaplow, Mike West.
Patch set 6:Commit-Queue +1
2 comments:
Patchset:
Hey folks!
This is adding a UKM to tell apart cases where the LCP is an animated image.
Feel free to just review the bits you own:
dcheng@ - for page_load_metrics.mojom
rkaplow@ - ukm.xml
sullivan@ and npm@ - everything else :)
File components/page_load_metrics/browser/observers/core/largest_contentful_paint_handler.h:
Patch Set #4, Line 42:
ContentfulPaintTimingInfo(const ContentfulPaintTimingInfo& other);Changed this to appease Tricium..
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Nicolás Peña Moreno, Daniel Cheng, Annie Sullivan, Robert Kaplow.
Yoav Weiss removed Mike West from this change.
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.
Nicolás Peña Moreno (Gerrit)
Attention is currently required from: Daniel Cheng, Yoav Weiss, Annie Sullivan, Robert Kaplow.
1 comment:
Patchset:
Seems this depends on the CL that got reverted, so no review needed yet?
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.
Annie Sullivan (Gerrit)
Attention is currently required from: Daniel Cheng, Yoav Weiss, Robert Kaplow.
1 comment:
Patchset:
Overall I think this is great, but do you think we could change it to a set of flags about the LCP? Some I can think of that we may want:
IsText
IsImage
IsVideo
IsAnimated
IsFullViewport
Is(SVG|GIF|JPG|PNG|Whatever)We don't have to implement every possibility right away, but using a flag would make adding features easy.
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Yoav Weiss, Robert Kaplow.
Patch set 7:Code-Review +1
2 comments:
Patchset:
LGTM from the Mojo perspective. I glanced over the Blink portions as well, but I don't have much insight to add there—it looks reasonable to me anyway :)
(If there are significant changes that would affect IPC, please ping me and I will re-review)
File components/page_load_metrics/browser/observers/core/largest_contentful_paint_handler.h:
Patch Set #4, Line 42:
ContentfulPaintTimingInfo(const ContentfulPaintTimingInfo& other);Changed this to appease Tricium..
Makes sense to me.
(Tricium in general warns on diffs, but unfortunately, your change touched lines in close proximity)
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kaplow (Gerrit)
Attention is currently required from: Yoav Weiss.
Patch set 7:Code-Review +1
1 comment:
Patchset:
lg, but make sure privacy has reviewed
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Yoav Weiss.
Patch set 12:Commit-Queue +1
2 comments:
File chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.cc:
Patch Set #12, Line 643:
builder.SetPaintTiming_LargestContentfulPaintIsAnimatedImage(sullivan@ - Do we want to report each type separately to UKM, or do we want to just report the uint bit-set of all the flags?
Patch Set #12, Line 837:
.SetPaintTiming_ExperimentalLargestContentfulPaint_ContentType(RE the "content type", I renamed the methods (to avoid confusion), but not sure what renaming the metric entails. Do we need to expose an alternative one and deprecate? Can we just rename?
Also, in the doc, you mentioned we'd want to include this info in the flag bit-set. The same questions apply if we'd want to take this route.
To view, visit change 3245540. To unsubscribe, or for help writing mail filters, visit settings.