Cosmetic Filter Implementation #48

Snuupy · 2018-07-13T00:24:19Z

At the point where I can take code feedback.

Testing notes: this does not work for any elements that are in-line. It will only work when we do a .removeElement() with the MutationObserver implementation

Done:

Add a context menu which shows on right click / cmd + click
Have the element blocked on the page (display:none or ~~remove from DOM~~)
Persist rules to store (ex: utilize the existing local storage used elsewhere)
Add a context menu item for removing rules on a given domain
Add a context menu item for removing rules on all domains
Fix webpack dev environment

To do for this PR:

Unit tests
- Pass existing tests
Remove logging/unused context menus
Clean up / refactor code
- Remove .vscode/settings.json, build-dev.js
- Remove console.log statements
- Reset prod.config.js to original version

For next PR(s):

Highlight corresponding element with candidate selector
DOM observer
Remove via JS
No flicker on load
Put right click context menu into redux workflow

bsclifton · 2018-08-03T15:54:53Z

@cezaraugusto @NejcZdovc @bbondy @jonathansampson @petemill ready for review 😄

cezaraugusto

comments left. noticed you added the cosmeticFilterEventsTest.ts but file is empty. let's remove it and add in a follow-up when we start writing tests for it

cezaraugusto · 2018-08-03T16:26:40Z

app/background/api/cosmeticFilterAPI.ts

+      result.list[hostname] !== null &&
+      result.list[hostname].length !== 0) {
+      result.list[hostname].map((rule: string) => {
+        console.log('applying rule', rule)


ok we can remove this

cezaraugusto · 2018-08-03T16:27:15Z

app/background/events/cosmeticFilterEvents.ts

+    console.log('report broken page sub-context menu created')
+  })
+
+  // log storage


let's remove this code block as we don't require it

cezaraugusto · 2018-08-03T16:27:31Z

app/background/events/cosmeticFilterEvents.ts

+    case 'addBlockElement':
+      {
+        console.log(rule)
+        console.log('blockElement clicked in cosmeticFilterEvents.ts', rule.host, rule.selector)


cezaraugusto · 2018-08-03T16:27:41Z

app/background/events/cosmeticFilterEvents.ts

+      }
+    case 'resetSiteFilterSettings':
+      {
+        console.log('dispatching cosmeticFilterAction to cosmeticFilterReducer from events')


cezaraugusto · 2018-08-03T16:28:14Z

app/background/events/cosmeticFilterEvents.ts

+        cosmeticFilterActions.siteCosmeticFilterRemoved(rule.host)
+        break
+      }
+    case 'reportBrokenPage':


not sure if this is needed we are only logging the result inside the action handler

cezaraugusto · 2018-08-03T16:41:03Z

app/background/api/cosmeticFilterAPI.ts

@@ -0,0 +1,41 @@
+// import { Tab } from '../../types/state/shieldsPannelState'
+// import { getShieldSettingsForTabData, getTabData } from './shieldsAPI'


let's remove these imports as we don't need them

cezaraugusto · 2018-08-03T16:41:38Z

app/background/api/cosmeticFilterAPI.ts

+
+export const addSiteCosmeticFilter = async (origin: string, cssfilter: string) => {
+  chrome.storage.local.get('list', (result = {}) => {
+    let l = Object.assign({}, result.list)


nit: can we have a more descriptive variable name here?

are you referring to list, l, result, or any combination of those 3?

ya I mean the l

cezaraugusto · 2018-08-03T16:41:49Z

app/background/api/cosmeticFilterAPI.ts

+
+export const removeSiteFilter = (origin: string) => {
+  chrome.storage.local.get('list', (result = {}) => {
+    let l = Object.assign({}, result.list)


same as above

cezaraugusto · 2018-08-03T16:42:33Z

app/background/events/cosmeticFilterEvents.ts

+// import tabActions from '../actions/tabActions'
+
+// import * as cosmeticFilterAPI from '../api/cosmeticFilterAPI'
+// background


let's remove the imports above as we don't use them

cezaraugusto · 2018-08-03T16:43:39Z

app/background/events/cosmeticFilterEvents.ts

+
+// add context menu
+chrome.runtime.onInstalled.addListener(function () {
+


nit: can we remove this empty line? standardjs would complain but I'm not sure why it doesn't

diracdeltas · 2018-08-06T22:10:29Z

app/background/events/cosmeticFilterEvents.ts

+
+// content script listener for right click DOM selection event
+chrome.runtime.onMessage.addListener((msg, sender, sendResponse) => {
+  rule.host = msg.baseURI


should the rule host include port number?

I propose filing that as a separate issue and keeping it as-is for now as there are several other features that have higher priority (mutation observer, UI > port for now).

petemill

Great job getting this all setup and working with typescript and integrated in to the extension.

I know that we're going to work on some better UI than a simple text input prompt, but until we have that I think we can make a slight improvement here: I find the context menu titles a confusing UI since the first refers to Blocking an 'ad' but the others refer to filtering 'sites'. Secondly, the UI that pops up asks for a 'rule' but gives no explanation as to what to type.
I propose the following new strings for these menu item titles whilst the UI is in this state:

- Block element vis CSS rule
- Clear CSS rules for this site
- Clear CSS rules for all sites

Alternatively, instead of CSS Rule we could use Selector, since that's what we're asking for (we create the display: none rule internally, the user does not provide it).

I've left more specific code comments inline.

We're also going to need to squash this down to 1 or 2 commits since this adds 1 feature. However I did notice at least 1 thing that should remain a separate commit / PR that this can rebase on.

petemill · 2018-08-06T21:24:45Z

app/background/api/cosmeticFilterAPI.ts

@@ -0,0 +1,36 @@
+export const addSiteCosmeticFilter = async (origin: string, cssfilter: string) => {
+  chrome.storage.local.get('list', (result = {}) => {
+    let l = Object.assign({}, result.list)


A more descriptive variable name here could be storeData instead of result and storeList instead of l.

petemill · 2018-08-06T21:27:42Z

app/background/events/cosmeticFilterEvents.ts

+    parentId: 'brave',
+    contexts: ['all']
+  }, () => {
+    console.log('block custom ad sub-context menu created')


We shouldn't have all the menu items automatically log for all users. If we want to keep it in, please put behind a debug flag, even if it's simply a const at the top of the module.

petemill · 2018-08-06T21:29:25Z

app/background/events/cosmeticFilterEvents.ts

+        break
+      }
+    default: {
+      console.log('invalid context menu option - cosmeticFilterEvents.ts')


If you're going to log an invalid option then put what the invalid option was as part of the log string, e.g: [cosmeticFilterEvents] invalid context menu option: ${info.menuItemId}

petemill · 2018-08-06T21:31:42Z

app/background/reducers/cosmeticFilterReducer.ts

+} from '../api/cosmeticFilterAPI'
+
+const focusedWindowChanged = (state: State, windowId: number): State => {
+  if (windowId !== -1) {


Might be nicer to return early here, instead of multiple levels of nesting conditionals, e.g.

if (windowId === -1) { console.warn(...) return state } ...

petemill · 2018-08-06T21:33:14Z

app/background/reducers/cosmeticFilterReducer.ts

+        setAllowBraveShields(tabData.origin, action.setting)
+          .then(() => {
+            reloadTab(tabId, true).catch(() => {
+              console.error('Tab reload was not successful')


Shouldn't we log the error too? .catch(e => console.error('Tab reload was not successful', e))

petemill · 2018-08-06T21:34:56Z

app/content.js

+    selector: selector,
+    baseURI: baseURI
+  }, ((response) => {
+    console.log('content.js - response received: ', response)


Should remove console.log by default

petemill · 2018-08-06T21:42:04Z

test/app/background/api/cosmeticFilterAPITest.ts

+    const filter = '#cssFilter'
+
+    let sampleValue: any
+    console.log(sampleValue)


Well, that error is correct: you are only declaring or assigning the variable, but never reading it, so it doesn't seem necessary. Why do you need to assign it inside callsFake( via sampleValue = value if you're never using that value in any test, or in the stub for chrome.storage.local.get? Looks like you can just use empty functions inside callsFake, just have an empty stub, or use that value in the stub for get (only if it needs it).

petemill · 2018-08-06T21:44:14Z

test/app/background/api/shieldsAPITest.ts

@@ -374,8 +374,8 @@ describe('Shields API', () => {
    it('resolves the returned promise', function (cb) {
      this.p
      .then(function () {


Don't need to create a function to call a function, can just pass in cb as the argument. Alternatively, you can just return the whole promise instead of using a callback.

I only fixed the typo, would you prefer if I changed all the shieldAPITests to conform to passing in cb instead?

ok, no that's ok - thanks anyway @Snuupy

petemill · 2018-08-06T21:53:15Z

.gitignore

@@ -64,3 +64,7 @@ build/

 # Ignore python compiled files
 *.pyc
+
+# VSCode config


This should really be done in a separate PR, or at least a separate commit once all of these commits are squashed.

petemill · 2018-08-06T22:10:43Z

app/background/events/cosmeticFilterEvents.ts

+
+  // block ad child menu
+  chrome.contextMenus.create({
+    title: 'Block ad',


I have an opinion on these context menu titles that I'll describe in the overall review comment

petemill · 2018-08-09T22:03:52Z

app/background/reducers/cosmeticFilterReducer.ts

@@ -29,6 +29,7 @@ const focusedWindowChanged = (state: State, windowId: number): State => {
      requestShieldPanelData(shieldsPanelState.getActiveTabId(state))
    } else {
      console.warn('no tab id so cannot request shield data from window focus change!')
+      return state


I don't think you need the extra return state here. The time to use that pattern is if you return early because of some condition, e.g.

if (condition) { return state } ...continue processing...

petemill

Thanks @Snuupy for a great start to the feature, and attention to all the review comments.

From a code / functionality POV, this lgtm and can be merged 👍

However, I did just notice that we're pointing to a github repo (although a Brave one) for a new package.json dependency - we may want to put this on npm as without that we won't benefit from semver or caching.

dri94 · 2018-08-30T12:18:34Z

There aren't clear instructions in how to merge brave-extension and brave-browser. Is this in the dev builds of brave browser now?

Snuupy · 2018-08-31T00:31:45Z

@dri94 Thanks for following up on the feature!

There aren't clear instructions in how to merge brave-extension and brave-browser.

Your observation is correct.

The way it works is the DEPS file in brave-core references a specific commit from each repository it pulls from to setup the development environment (or to create builds).

As you can see from line 8 in the DEPS file:

"vendor/brave-extension": "https://github.com/brave/brave-extension.git@2ea4ff3d9c9cbc0ad70c3ad3f3335a8d1d39ce1f"

If you look at the merge/commit log in brave-extension, you'll see that my commit (d192d23) is before the one referenced (2ea4ff3) which means by default the build process will include the cosmetic filtering feature.

Because of the way the build process works, I also had to update the brave-core/browser/resources/brave_extension.grd file to include the content script as well.

I know this is not exactly intuitive, but I hope this clears things up.

Is this in the dev builds of brave browser now?

Yes.

Snuupy force-pushed the cosmeticFilter branch from 0623d99 to 5cf0f81 Compare July 18, 2018 23:32

NejcZdovc added the PR/work-in-progress ⚒ label Jul 24, 2018

Snuupy mentioned this pull request Aug 2, 2018

Add a block this element feature to the brave-extension (cosmetic adblock filters) brave/brave-browser#94

Closed

32 tasks

bsclifton requested review from jonathansampson, bbondy, cezaraugusto, bsclifton and NejcZdovc August 2, 2018 21:12

bsclifton assigned Snuupy Aug 2, 2018

bsclifton requested a review from petemill August 2, 2018 22:28

bsclifton changed the title ~~WIP: Cosmetic Filter Implementation~~ Cosmetic Filter Implementation Aug 3, 2018

bsclifton removed the PR/work-in-progress ⚒ label Aug 3, 2018

cezaraugusto reviewed Aug 3, 2018

View reviewed changes

diracdeltas reviewed Aug 6, 2018

View reviewed changes

petemill suggested changes Aug 6, 2018

View reviewed changes

bsclifton mentioned this pull request Aug 7, 2018

Block native ads brave/browser-laptop#527

Closed

petemill reviewed Aug 9, 2018

View reviewed changes

Snuupy force-pushed the cosmeticFilter branch from f68bb56 to bf275d0 Compare August 9, 2018 23:12

add cosmetic filter feature

b914102

Snuupy force-pushed the cosmeticFilter branch from bf275d0 to b914102 Compare August 10, 2018 05:08

petemill approved these changes Aug 10, 2018

View reviewed changes

Snuupy merged commit 85aa24a into brave:master Aug 10, 2018

bsclifton deleted the cosmeticFilter branch August 10, 2018 21:19

Snuupy mentioned this pull request Aug 17, 2018

put dependency package on npm Snuupy/brave-extension#10

Open

		@@ -0,0 +1,41 @@
		// import { Tab } from '../../types/state/shieldsPannelState'
		// import { getShieldSettingsForTabData, getTabData } from './shieldsAPI'


		// add context menu
		chrome.runtime.onInstalled.addListener(function () {

Cosmetic Filter Implementation #48

Cosmetic Filter Implementation #48

Conversation

Snuupy commented Jul 13, 2018 • edited Loading

bsclifton commented Aug 3, 2018

cezaraugusto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

dri94 commented Aug 30, 2018

Snuupy commented Aug 31, 2018

Snuupy commented Jul 13, 2018 •

edited

Loading

petemill left a comment •

edited

Loading