Skip to content

/ brave-core

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement cosmetic filtering #3303

Merged
merged 9 commits into from Dec 14, 2019
+807 −13
Merged

Implement cosmetic filtering #3303

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1715046
expose cosmetic filtering javascript API
antonok-edm Aug 8, 2019
0b93419
adaptations of @Snuupy's MutationObserver work
Snuupy Jun 18, 2019
7512da5
use cosmetic filtering api
antonok-edm Aug 20, 2019
c45be3c
work around browser-internal race condition
antonok-edm Aug 27, 2019
3aca80f
add browser tests for cosmetic filtering
antonok-edm Aug 27, 2019
5640c42
add brave://flags feature to enable cosmetic filtering
antonok-edm Nov 22, 2019
6a9fa30
connect feature flag to cosmetic filtering mechanism
antonok-edm Nov 22, 2019
e7af497
only use MutationObserver when cosmetic filtering is enabled
antonok-edm Dec 5, 2019
ec80ce7
store generic rule exceptions in the Shields Panel state
antonok-edm Dec 11, 2019
File filter...
Filter file types
Clear filters
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Prev Next

add browser tests for cosmetic filtering

  • Loading branch information
@antonok-edm
antonok-edm committed Aug 27, 2019
Verified
This commit was signed with a verified signature.
antonok-edm Anton Lazarev
GPG key ID: FBD20243E0CD9104 Learn about signing commits
commit 3aca80f9dcfb64fae40e283d34803a46219ec2dd
1 components/brave_extension/extension/brave_extension/background/api/shieldsAPI.ts
View file
Open in desktop
@@ -49,6 +49,7 @@ export const getShieldSettingsForTabData = (tabData?: chrome.tabs.Tab) => {
hostname,
id: tabData.id,
braveShields: 'block',
cosmeticBlocking: 0,
ads: 0,
trackers: 0,
httpUpgradableResources: 0,
3 ...ents/brave_extension/extension/brave_extension/background/reducers/shieldsPanelReducer.ts
View file
Open in desktop
@@ -108,6 +108,9 @@ export default function shieldsPanelReducer (
case shieldsPanelTypes.SHIELDS_PANEL_DATA_UPDATED: {
state = shieldsPanelState.updateTabShieldsData(state, action.details.id, action.details)
shieldsPanelState.updateShieldsIcon(state)
if (chrome.test && shieldsPanelState.getActiveTabData(state)) {
chrome.test.sendMessage('brave-extension-shields-data-ready')
}
break
}
case shieldsPanelTypes.SHIELDS_TOGGLED: {
1 components/brave_extension/extension/brave_extension/types/actions/shieldsPanelActions.ts
View file
Open in desktop
@@ -7,6 +7,7 @@ import { BlockTypes, BlockOptions, BlockFPOptions, BlockJSOptions, BlockCookiesO

export interface ShieldDetails {
id: number
cosmeticBlocking: BlockOptions
ads: BlockOptions
trackers: BlockOptions
httpUpgradableResources: BlockOptions
129 components/brave_shields/browser/ad_block_service_browsertest.cc
View file
Open in desktop
@@ -24,6 +24,7 @@
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/test/browser_test_utils.h"
#include "extensions/test/extension_test_message_listener.h"
#include "net/dns/mock_host_resolver.h"

using content::BrowserThread;
@@ -218,6 +219,15 @@ class AdBlockServiceTest : public ExtensionBrowserTest {
base::CreateSingleThreadTaskRunner({BrowserThread::IO}).get()));
ASSERT_TRUE(io_helper->Run());
}

void WaitForBraveExtensionShieldsDataReady() {
// Sometimes, the page can start loading before the Shields panel has
// received information about the window and tab it's loaded in.
ExtensionTestMessageListener extension_listener(
"brave-extension-shields-data-ready",
false);
ASSERT_TRUE(extension_listener.WaitUntilSatisfied());
}
};

// Load a page with an ad image, and make sure it is blocked.
@@ -774,3 +784,122 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectRulesAreRespected) {
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);
}

// Test simple cosmetic filtering
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringSimple) {
UpdateAdBlockInstanceWithRules(
"b.com###ad-banner\n"
"##.ad");

WaitForBraveExtensionShieldsDataReady();

GURL tab_url = embedded_test_server()->GetURL("b.com",
"/cosmetic_filtering.html");
ui_test_utils::NavigateToURL(browser(), tab_url);

content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('#ad-banner', 'display', 'none')",
&as_expected));
EXPECT_TRUE(as_expected);

as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('.ad-banner', 'display', 'block')",
&as_expected));
EXPECT_TRUE(as_expected);

as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('.ad', 'display', 'none')",
&as_expected));
EXPECT_TRUE(as_expected);
}

// Test cosmetic filtering on elements added dynamically
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringDynamic) {
UpdateAdBlockInstanceWithRules("##.blockme");

WaitForBraveExtensionShieldsDataReady();

GURL tab_url = embedded_test_server()->GetURL("b.com",
"/cosmetic_filtering.html");
ui_test_utils::NavigateToURL(browser(), tab_url);

content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"addElementsDynamically();\n"
"checkSelector('.blockme', 'display', 'none')",
&as_expected));
EXPECT_TRUE(as_expected);

as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('.dontblockme', 'display', 'block')",
&as_expected));
EXPECT_TRUE(as_expected);
}

// Test custom style rules
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringCustomStyle) {
UpdateAdBlockInstanceWithRules("b.com##.ad:style(padding-bottom: 10px)");

WaitForBraveExtensionShieldsDataReady();

GURL tab_url = embedded_test_server()->GetURL("b.com",
"/cosmetic_filtering.html");
ui_test_utils::NavigateToURL(browser(), tab_url);

content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('.ad', 'padding-bottom', '10px')",
&as_expected));
EXPECT_TRUE(as_expected);
}

// Test rules overridden by hostname-specific exception rules
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringUnhide) {
UpdateAdBlockInstanceWithRules(
"##.ad\n"
"b.com#@#.ad\n"
"###ad-banner\n"
"a.com#@##ad-banner");

WaitForBraveExtensionShieldsDataReady();

GURL tab_url = embedded_test_server()->GetURL("b.com",
"/cosmetic_filtering.html");
ui_test_utils::NavigateToURL(browser(), tab_url);

content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('.ad', 'display', 'block')",
&as_expected));
EXPECT_TRUE(as_expected);

as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
"checkSelector('#ad-banner', 'display', 'none')",
&as_expected));
EXPECT_TRUE(as_expected);
This conversation was marked as resolved by bbondy

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy Nov 18, 2019

Member

Good job on these tests 👍

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy Nov 18, 2019

Member

Could you add a test for when cosmetic filters are off that it doesn't actually do any work?
Either by preference or by calling this explicitly:

brave_shields::SetCosmeticFilteringControlType(profile_,
                                  value ? ControlType::BLOCK
                                        : ControlType::ALLOW,
                                  GURL());

I think setting just the pref won't work currently because this is done in the callback and the pref isn't actually checked anywhere.

This comment has been minimized.

Sign in to view
Copy link
@antonok-edm

antonok-edm Nov 19, 2019

Author Collaborator

For the reason I mentioned above, I couldn't actually get the value of the preference when running tests, so I defaulted to having cosmetic filtering on. This would be a good thing to fix and test, but I don't know where to go from here.

This comment has been minimized.

Sign in to view
Copy link
@antonok-edm

antonok-edm Dec 7, 2019

Author Collaborator

Since moving to a feature flag I was able to make a test for this.
}
10 components/test/brave_extension/background/api/cosmeticFilterAPI_test.ts
View file
Open in desktop
@@ -177,7 +177,7 @@ describe('cosmeticFilter API', () => {
})
})
})
describe('applySiteFilters', () => {
describe('applyCSSCosmeticFilters', () => {
const filter = '#cssFilter'
const filter2 = '#cssFilter2'

@@ -206,7 +206,7 @@ describe('cosmeticFilter API', () => {
'brave.com': [filter]
}
})
cosmeticFilterAPI.applySiteFilters(1, 'brave.com')
cosmeticFilterAPI.applyCSSCosmeticFilters(1, 'brave.com')
expect(insertCSSStub.getCall(0).args[0]).toEqual(1)
expect(insertCSSStub.getCall(0).args[1]).toEqual({
code: `${filter} {display: none !important;}`,
@@ -220,7 +220,7 @@ describe('cosmeticFilter API', () => {
'brave.com': [filter, filter2]
}
})
cosmeticFilterAPI.applySiteFilters(1, 'brave.com')
cosmeticFilterAPI.applyCSSCosmeticFilters(1, 'brave.com')
expect(insertCSSStub.getCall(0).args[0]).toEqual(1)
expect(insertCSSStub.getCall(0).args[1]).toEqual({
code: `${filter } {display: none !important;}`,
@@ -239,7 +239,7 @@ describe('cosmeticFilter API', () => {
getStorageStub.yields({
cosmeticFilterList: {}
})
cosmeticFilterAPI.applySiteFilters(1, 'brave.com')
cosmeticFilterAPI.applyCSSCosmeticFilters(1, 'brave.com')
expect(insertCSSStub.called).toBe(false)
})
it('doesn\'t apply filters if storage is explicitly undefined', () => {
@@ -248,7 +248,7 @@ describe('cosmeticFilter API', () => {
'brave.com': undefined
}
})
cosmeticFilterAPI.applySiteFilters(1, 'brave.com')
cosmeticFilterAPI.applyCSSCosmeticFilters(1, 'brave.com')
expect(insertCSSStub.called).toBe(false)
})
})
2 components/test/brave_extension/background/api/shieldsAPI_test.ts
View file
Open in desktop
@@ -38,6 +38,7 @@ describe('Shields API', () => {
origin: 'https://www.brave.com',
hostname: 'www.brave.com',
braveShields: 'block',
cosmeticBlocking: true,
ads: 'block',
trackers: 'block',
httpUpgradableResources: 'block',
@@ -94,6 +95,7 @@ describe('Shields API', () => {
})
it('resolves and calls requestShieldPanelData', (cb) => {
const details: ShieldDetails = {
cosmeticBlocking: true,
ads: 'block',
trackers: 'block',
httpUpgradableResources: 'block',
3 components/test/testData.ts
View file
Open in desktop
@@ -217,6 +217,9 @@ export const getMockChrome = () => {
getBraveShieldsEnabledAsync: function (url: string) {
return Promise.resolve(false)
},
getCosmeticFilteringEnabledAsync: function (url: string) {
return Promise.resolve(true)
},
getAdControlTypeAsync: function (url: string) {
return Promise.resolve('block')
},
41 test/data/cosmetic_filtering.html
View file
Open in desktop
@@ -0,0 +1,41 @@
<html>
<head>
<script>

function addElementsDynamically() {
let root = document.documentElement;
for (let i = 0; i < 10; i++) {
const e = document.createElement('div')
e.className = 'blockme'
root.appendChild(e);
}

for (let i = 0; i < 10; i++) {
const e = document.createElement('div')
e.className = 'dontblockme'
root.appendChild(e);
}
}

function checkSelector(selector, property, expected) {
setTimeout(() => {
let elements = [].slice.call(document.querySelectorAll(selector));
let result = elements.every(e => {
let style = window.getComputedStyle(e);
return style[property] === expected;
})
window.domAutomationController.send(result)
}, 1500)
}

</script>
</head>
<body>
<div id="ad-banner"></div>
<div class="ad-banner">
<div class="ad"></div>
</div>
<div class="ad"></div>
<div class="ad"></div>
</body>
</html>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.