New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 818666 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Measure the number of cross-origin vs same-origin ads

Project Member Reported by jkarlin@chromium.org, Mar 5 2018

Issue description

Let's create an enum histogram: PageLoad.Clients.Ads.FrameCounts.AnyParentFrame.Origin with enum types of SAME_ORIGIN and CROSS_ORIGIN.

The idea is that if an ad is in a same-origin frame to the page's main frame, we'll label it as SAME_ORIGIN else cross.



 
Cc: jkarlin@chromium.org
Owner: ericrobinson@chromium.org
So keep in mind I'm a new Chromium developer, but I'm wondering why we'd choose to use a Histogram representation when there are exactly two values.  Given the enums, it doesn't seem to leave room for future values.

I'm happy to do this as a Histogram if that's how this sort of thing is typically done (or "histogram" is really the measurement type, but it can be displayed in different formats), but it seems like a different representation (such as a pie chart or straight percentage) would make more sense.
Okay, figured out the previous part myself, though a couple of things:
1. Right now I'm planning on using UMA_HISTOGRAM_BOOLEAN rather than an enum (with "...AnyParentFrame.CrossOrigin"), as I can't see this being more than two values.  I can switch this to an enum if you do think there's a need.
2. I'm noticing most (all?) of our metrics have an extra prefix: "Pageloads.Clients.Ads.(Google/SubresourceFiler/All).FrameCounts...".  Just wanted to make sure this was left out on purpose.
A boolean makes sense to me, so perhaps PageLoad.Clients.Ads.FrameCounts.AnyParentFrame.SameOrigin?

Please do add the prefixes. The ADS_HISTOGRAM macro should take care of that for you.
Sorry, I improperly associated the CL to bug number.  This bug is closed by:

Change has been successfully rebased and submitted as 81bbb440c7bd6f449b4f79b6a3db129fd98a14ae by Commit Bot
https://chromium-review.googlesource.com/c/chromium/src/+/970908
I think we might be counting about:srcdoc, about:blank, and data scheme urls as cross-origin to the main frame, when they should be same-origin.
I can definitely check that out.  Do you happen to know of an example of a page that uses this for an ad?
Forbes articles tend to have them. 

https://chrome.google.com/webstore/detail/framesexplorer/imijdbpfemdegalijeojlkhiamfcgklp is a useful extension to show you the frame layout of a page.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8356f1ef9a79c036134c7a7344f3871b530f0b8

commit e8356f1ef9a79c036134c7a7344f3871b530f0b8
Author: Eric Robinson <ericrobinson@chromium.org>
Date: Mon Apr 30 11:32:59 2018

Fixing the origin for the ad frame.

This fixes the origin for the ad iframe to properly account for srcdoc contents
by using the Renderer's origin rather than building one ourselves.

Bug:  818666 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I00953f8147e46a097099225b8a15d8248ce1f352
Reviewed-on: https://chromium-review.googlesource.com/986673
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554721}
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
[add] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/test/data/ads_observer/same_origin_ad.html
[add] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/test/data/ads_observer/srcdoc_embedded_ad.html
[add] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/chrome/test/data/ads_observer/srcdoc_embedded_ad_empty.html
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/e8356f1ef9a79c036134c7a7344f3871b530f0b8/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Assigned)

Sign in to add a comment