New issue
Advanced search Search tips

Issue 880829 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 6
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Ad filtering Smart UI broken in M69

Project Member Reported by csharrison@chromium.org, Sep 5

Issue description

To reproduce:
1. Visit a site with ad filtering enabled on Android e.g. testsafebrowsing.appspot.com/s/phishing.html

2. See ads are filtered, with the "ad block" UI

3. Refresh the page. See the "ad block" UI again when it should be hidden.

 
Labels: Merge-Request-70
This landed in https://chromium-review.googlesource.com/c/chromium/src/+/1206674

Requesting a merge to M70 for this after a day of baking in canary. It is a trivial CL with +7/-2 delta in non-test files.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 5

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

commit d27345d5783756e83e65687da9ed5ec2c356f7b6
Author: Charlie Harrison <csharrison@chromium.org>
Date: Wed Sep 05 17:55:54 2018

[subresource_filter] Fix smart UI on Android

This regressed in crrev.com/c/1081370 due to a combination of
1. Over-eager refactoring
2. Phantom tests that didn't actually test the functionality.

The regression is reverted here and the test for the feature is
fixed so it actually triggers the behavior.

Bug:  880829 
Change-Id: Ic27a0718de685acea00af0b5d88e249b5fb0a6c2
Reviewed-on: https://chromium-review.googlesource.com/1206674
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588930}
[modify] https://crrev.com/d27345d5783756e83e65687da9ed5ec2c356f7b6/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc
[modify] https://crrev.com/d27345d5783756e83e65687da9ed5ec2c356f7b6/chrome/browser/subresource_filter/subresource_filter_settings_browsertest.cc

Status: Fixed (was: Assigned)
Project Member

Comment 4 by sheriffbot@chromium.org, Sep 6

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
I confirmed in the latest canary that this is fixed. Proceeding with merge.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 6

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e25f5dd92581bba905d7336d7466ff556b59fd97

commit e25f5dd92581bba905d7336d7466ff556b59fd97
Author: Charlie Harrison <csharrison@chromium.org>
Date: Thu Sep 06 19:53:13 2018

[subresource_filter] Fix smart UI on Android

This regressed in crrev.com/c/1081370 due to a combination of
1. Over-eager refactoring
2. Phantom tests that didn't actually test the functionality.

The regression is reverted here and the test for the feature is
fixed so it actually triggers the behavior.

TBR=jkarlin@chromium.org

Bug:  880829 
Change-Id: Ic27a0718de685acea00af0b5d88e249b5fb0a6c2
Reviewed-on: https://chromium-review.googlesource.com/1206674
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588930}(cherry picked from commit d27345d5783756e83e65687da9ed5ec2c356f7b6)
Reviewed-on: https://chromium-review.googlesource.com/1211544
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#94}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/e25f5dd92581bba905d7336d7466ff556b59fd97/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc
[modify] https://crrev.com/e25f5dd92581bba905d7336d7466ff556b59fd97/chrome/browser/subresource_filter/subresource_filter_settings_browsertest.cc

Sign in to add a comment