New issue
Advanced search Search tips

Issue 856273 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 8
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

AdTracker should only be created if the AdTagging is enabled

Project Member Reported by jkarlin@chromium.org, Jun 25 2018

Issue description

AdTracker may have noticable impact on performance and we need to track it. So let's gate the creation of it on the AdTagging field trial.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 19

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

commit a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791
Author: John Delaney <johnidel@chromium.org>
Date: Thu Jul 19 15:03:21 2018

Require adtagging enabled for AdTracker creation

Only provision an AdTracker to a local frame if AdTagging is
enabled. Useful to measure the performance impact of the AdTracker.
AdTagging feature declaration moved to support RuntimeEnabledFeature

Bug:  856273 
Change-Id: I898c78d5be916583744b445afad885d445b440bf
Reviewed-on: https://chromium-review.googlesource.com/1115374
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: John Delaney <johnidel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576493}
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/chrome/renderer/DEPS
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/third_party/blink/public/platform/web_runtime_features.h
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/third_party/blink/renderer/core/frame/ad_tracker_test.cc
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/third_party/blink/renderer/core/frame/local_frame.cc
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/third_party/blink/renderer/platform/exported/web_runtime_features.cc
[modify] https://crrev.com/a80e0326ebc7fe4b2dd2d26ae570f68da3fb6791/third_party/blink/renderer/platform/runtime_enabled_features.json5

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 17

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

commit 438caa21caa4c4ded95c0d6ab70d58d70fbb154a
Author: Josh Karlin <jkarlin@chromium.org>
Date: Wed Oct 17 18:06:08 2018

[AdTagging] Fix disabled test

The test was supposed to verify that, when AdTagging is disabled,
frames with ad urls aren't tagged as ads. This wasn't working as
intended. It passed regardless of whether or not ad tagging was
disabled. What the test wanted to do isn't really possible when an
AdTracker isn't present, and it's not when AdTagging is disabled.

Changed the test to verify that the AdTracker is not present when
AdTagging is disabled.

Bug:  856273 
Change-Id: I6e31444774ed5c1d1a9156a2a16e92896909d8fa
Reviewed-on: https://chromium-review.googlesource.com/c/1273816
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600477}
[modify] https://crrev.com/438caa21caa4c4ded95c0d6ab70d58d70fbb154a/third_party/blink/renderer/core/frame/ad_tracker_test.cc

Sign in to add a comment