New issue
Advanced search Search tips

Issue 884233 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 11
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Cleanup: Remove the Google Ad Metrics

Project Member Reported by jkarlin@chromium.org, Sep 14

Issue description

Now that we have AdTagging ready to launch, it's time to remove AD_TYPE_GOOGE and AD_TYPE_ANY and just use subresource filter. That will remove lots of metrics, code, and histogram prefix complexity.
 
Owner: jkarlin@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 20

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

commit c7f0aa6a54816da89a476e117dcdb157f4cbd066
Author: Josh Karlin <jkarlin@chromium.org>
Date: Thu Sep 20 20:35:22 2018

[AdsMetrics] Remove unused metric, PageLoad.Clients.Ads.All.ResourceTypeWhenNoFrameFound

Removes a metric that showed the resources loaded in non-ad
frames. This was never referenced so it's time to remove it.

Bug:  884233 
Change-Id: Icc3522b515d757f4b58c30edcf7373da98453423
Reviewed-on: https://chromium-review.googlesource.com/1233937
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592931}
[modify] https://crrev.com/c7f0aa6a54816da89a476e117dcdb157f4cbd066/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
[modify] https://crrev.com/c7f0aa6a54816da89a476e117dcdb157f4cbd066/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/c7f0aa6a54816da89a476e117dcdb157f4cbd066/tools/metrics/histograms/histograms.xml

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 21

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

commit f803b6cba3eba98d3af74671134a2a60d2d55f1c
Author: Josh Karlin <jkarlin@chromium.org>
Date: Fri Sep 21 15:55:14 2018

[AdMetrics] Remove unnecessary metric, PageLoad.Clients.Ads.All.ParentExistsForSubFrame

Removing ParentExistsForSubFrame metric as it was only briefly
necessary to verify that the code worked as intended.

Bug:  884233 
Change-Id: I6934f92d295e03bf2715fc3460333dca63f6f53f
Reviewed-on: https://chromium-review.googlesource.com/1234224
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593201}
[modify] https://crrev.com/f803b6cba3eba98d3af74671134a2a60d2d55f1c/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
[modify] https://crrev.com/f803b6cba3eba98d3af74671134a2a60d2d55f1c/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/f803b6cba3eba98d3af74671134a2a60d2d55f1c/tools/metrics/histograms/histograms.xml

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 9

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 10

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

commit 267afb8ee6a93a9e89239997c27989737aee1119
Author: Josh Karlin <jkarlin@chromium.org>
Date: Thu Jan 10 21:52:05 2019

[AdTagging] Remove .Google. ad histograms

What:
Remove the .Google. Ads histograms, leaving only .SubresourceFilter.
John will rename those in an upcoming CL.

Why:
We're focusing on the ads detected by AdTagging at this point and no
longer need the Google histograms.

How:
Most of the changes for this are in tests. The browser tests used to
use the iframe's name as a means of ad detection for cases where the
iframe doesn't have a url (e.g., doc.written frames). Now, doc.written
ad iframes are detected because they're created by known ad scripts.

Bug:  884233 
Change-Id: I08cbecc9f70f698d24529e50aa1cde8db1de0981
Reviewed-on: https://chromium-review.googlesource.com/c/1403092
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621755}
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/browser/subresource_filter/ad_tagging_browsertest.cc
[add] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/test/data/ads_observer/ad_iframe_writer.js
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/test/data/ads_observer/docwrite_blank_frame.html
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/test/data/ads_observer/docwrite_provisional_frame.html
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/test/data/ads_observer/same_origin_ad.html
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/test/data/ads_observer/srcdoc_embedded_ad.html
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/chrome/test/data/ads_observer/srcdoc_embedded_ad_empty.html
[modify] https://crrev.com/267afb8ee6a93a9e89239997c27989737aee1119/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)
I've removed .Google. and .All. The SubresourceFilter type remains, which John will remove in an upcoming CL.

Sign in to add a comment