[brave-extension] add settings option to hide Shields activity count #3150

cezaraugusto · 2019-08-12T09:50:36Z

fix brave/brave-browser#3121

Test Plan:

Visit a website with ads/trackers
Assert resources blocked count is visible
In a new tab, go to brave://settings/shields/
Set the option to show activity count to false
Go back to the website
Assert resources blocked count is not visible. Refresh page
Assert count is still not visible
Go back to brave://settings/shields/ and toggle the option on
Repeat the same operation. Badge count should be visible again
Go back to brave://settings/shields/ and toggle the option off again
Activate the website tab
Quit Brave (cancel npm start script)
Open Brave (npm start)
Ensure counter does not show

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

petemill

👍 but just a couple of flow changes would be nice if possible

petemill · 2019-08-12T16:13:37Z

components/brave_extension/extension/brave_extension/background/api/browserActionAPI.ts

-    chrome.browserAction.setBadgeText({
-      tabId,
-      text: String(text)
+  getViewPreferences()


Calling in to another API from this API doesn't seem like great code organization to me for the following reasons:

It's retrieving the data again, adding a (small) delay.

The app decision logic is now in multiple places.

It's complicating the otherwise simple module, perhaps also complicating testability. This means the API no longer just does what it says setBadgeText.

Instead, since we store this data in the redux state or some other main decision making area, can we simply read current value of the state before we call setBadgeText and decide what to pass in?

that one is tricky because change happens with shields closed so there's no state since the component isn't mounted. I moved the data to the persistent state (shields localStorage). lmk if you have a better option

petemill · 2019-08-12T16:21:47Z

components/brave_extension/extension/brave_extension/background/api/shieldsAPI.ts

 }
 export async function getViewPreferences (): Promise<GetViewPreferencesData> {
  const showAdvancedViewPref = await SettingsPrivate.getPreference(settingsKeys.showAdvancedView.key)
+  const statsBadgeVisiblePref = await SettingsPrivate.getPreference(settingsKeys.statsBadgeVisible.key)


We can use promise.all here instead of waiting for each setting. As an extra, since we store the neccessary config in one object, this code also lends itself to avoiding repetition with something like:

let settings = {} await Promise.all(Object.keys(settingsKeys).map(async (name) => { // Get setting by internal key const pref = await SettingsPrivate.getPreference(settingsKeys[name].key) // Validate setting type if (pref.type !== settingsKeys[name].type) { throw new Error(`Unexpected settings type received for "${settingsKeys[name].key}". Expected: ${settingsKeys[name].type}, Received: ${pref.type}`) } // Valid settings[name] = pref.value }) return settings

wdyt?

sounds good, updated with the above

petemill · 2019-08-12T16:25:49Z

components/brave_extension/extension/brave_extension/background/api/shieldsAPI.ts

 }

 const settingsKeys = {
-  showAdvancedView: { key: 'brave.shields.advanced_view_enabled', type: chrome.settingsPrivate.PrefType.BOOLEAN }
+  showAdvancedView: { key: 'brave.shields.advanced_view_enabled', type: chrome.settingsPrivate.PrefType.BOOLEAN },
+  statsBadgeVisible: { key: 'brave.shields.stats_badge_visible', type: chrome.settingsPrivate.PrefType.BOOLEAN },


lint: comma (even though I personally like this style :-( )

thought our linter would spot that. updated

petemill · 2019-08-12T16:28:14Z

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html

+      <div class="label shields-primary-title">$i18n{lookFeelTitle}</div>
+    </div>
+    <settings-toggle-button
+        class="first-after-text"


There is the .continuation chromium settings css class, so you shouldn't need to make the .first-after-text class.

petemill · 2019-08-14T18:27:20Z

components/brave_extension/extension/brave_extension/background/reducers/shieldsPanelReducer.ts

+        getViewPreferences()
+          .then((settings: chrome.braveShields.BraveShieldsViewPreferences) => {
+            if (state.persistentData.statsBadgeVisible === settings.statsBadgeVisible) {
+              state = shieldsPanelState.updatePersistentData(state, { statsBadgeVisible: settings.statsBadgeVisible })


Setting state in a callback won't affect the state since the reducer will have already returned the state before this .then callback is run, e.g.

console.log('1'); Promise.resolve().then(() => console.log('2')); console.log('3'); > 1 > 3 > 2

To solve all your issues with state, perhaps you should have a redux action called VIEW_PREFERENCES_CHANGED, and use chrome.settingsPrivate.onPrefsChanged.addListener to call this when the relevant items change.

Perhaps in shieldsAPI.ts you would have something line:

export function addViewPreferencesChangedListener(callback) { chrome.settingsPrivate.onPrefsChanged.addListener(changedPrefs => { // Reverse settings key / name lookup (can be done outside of this function) const settingsNameByKey = {} for (const name of Object.keys(settingsKeys)) { settingsNameByKey[settingsKeys[name].key] = { ...settingsKeys[name], name } } // Get changed settings that are relevant const changedSettings: SetViewPreferencesData = {} for (const pref of changedPrefs) { const setting = settingsNameByKey[pref.key] if (setting) { // Validate setting type if (pref.type !== setting.type) { throw new Error(`Unexpected settings type received for "${pref.key}". Expected: ${setting.type}, Received: ${pref.type}`) } // Valid changedSettings[setting.name] = pref.value } } if (Object.keys(changedSettings).length) { callback(changedSettings) } }) }

and then in your redux store setup you could use:

shieldsAPI.addViewPreferencesChangedListener(actions.viewPreferencesChanged) shieldsAPI.getViewPreferences.then(actions.viewPreferencesChanged)

and in the reducer store the value of these preferences in the state wherever you like.
wdyt?

your proposal looks good but I think the call order doesn't matter since localStorage would update anyway, which is what is read by the reducer? also, shouldn't we avoid calling actions inside the store?

I mean, I can't think of a case where the store would receive the wrong value as-is

As-is, you are calling an async API inside the reducer, and then bypassing the action -> reducer flow to attempt to directly set the state inside that async callback. So that's the anti-pattern, but also the state = isn't actually doing anything here, so all this code can be removed, right? Or IMO better, is to move shieldsAPI.addViewPreferencesChangedListener(actions.viewPreferencesChanged) to your init function, whether that's the app init or store init (Setting up async events -> redux actions in the store init is a fine pattern IMO. The anti-pattern is calling actions inside the reducer function, which that wouldn't neccessarily be doing).

e.g in background.ts you could setup the calls for the initial data which fires an action to set the initial data, as well as subscribe to events to update that data by firing an action again on change, unless I'm mistaking the architecture? I just don't think we want to exemplify a pattern where we go to an API for the data every time. Also when we receive an action that the preferences have changed, we can check the state to see if the statsBadgeVisible changed and if it did, then go and update the badge text for every tab. That way other windows will be updated instantly when the setting changes.

petemill · 2019-08-14T18:28:49Z

components/brave_extension/extension/brave_extension/state/shieldsPanelState.ts

    // do not show any badge if there are no blocked items
-    setBadgeText(tabId, total > 99 ? '99+' : total > 0 ? total.toString() : '')
+    setBadgeText(tabId, text, state.persistentData.statsBadgeVisible)


Why pass in this boolean to a generic API and not just only pass text if statsBadgeVisible is true?

we need to pass empty strings to override the previous call. otherwise we will need to update the page to have the change visible

Suggestion is to pass empty string and keep the decision in the caller layer and keep setBadgeText a simple API. setBadgeText(tabId, statsBadgeVisible ? '' : text)

petemill · 2019-08-14T22:35:48Z

components/definitions/chromel.d.ts

  }
  type BraveShieldsSetViewPreferencesData = {
    showAdvancedView?: boolean
+    statsBadgeVisible?: boolean
  }


I think both these types can be removed since we use chrome.settingsPrivate API now. Must have left this in by accident.

petemill · 2019-08-14T22:37:33Z

components/brave_extension/extension/brave_extension/background/api/shieldsAPI.ts

@@ -163,24 +163,34 @@ export const setAllowScriptOriginsOnce = (origins: Array<string>, tabId: number)
  })

 export type GetViewPreferencesData = {
-  showAdvancedView: boolean
+  showAdvancedView?: boolean
+  statsBadgeVisible?: boolean


Why did you make these optional? As far as I can see, getViewPreferences always returns all properties, so callers won't have to do extra type checks.

cezaraugusto · 2019-08-26T18:10:51Z

@petemill updated, could you re-review?

petemill · 2019-08-26T18:27:04Z

components/brave_extension/extension/brave_extension/background/api/storageAPI.ts

@@ -7,6 +7,11 @@ import * as Shields from '../../types/state/shieldsPannelState'

 const keyName = 'shields-persistent-data'

+export const initialSettingsData = {


nit: needs type

petemill

Thanks for addressing feedback! Most of this review is nits which are optional for you. There is only 1 issue which I think absolutely needs change - and that is parsing the settings keys from the onPrefsChanged API.

petemill · 2019-08-26T18:28:09Z

components/brave_extension/extension/brave_extension/types/state/shieldsPannelState.ts

@@ -65,6 +67,11 @@ export interface UpdatePersistentData {
  (state: State, persistentData: Partial<PersistentData>): State
 }

+export interface MergeSettingsData {
+  (state: State, settingsData: SettingsData): SettingsData
+  // (state: State, settingsData: Partial<SettingsData>): State


commented out code

petemill · 2019-08-26T18:28:51Z

components/brave_extension/extension/brave_extension/types/other/settingsTypes.ts

+  [key: string]: OneOfSettings
+}
+
+export type OneOfSettings =


nit: might be clearer to be named SettingsKey or similar, since this isn't a setting but some kind of string key

in fact you could just do keyof BraveShieldsViewPreferences or keyof SettingsKeys from https://github.com/brave/brave-core/pull/3150/files/557e80636c52fb4e7f09ac578ad555a16a26caaf#diff-5875b9a5788aaf15e33a6ded24690654R171

petemill · 2019-08-26T18:32:47Z

components/brave_extension/extension/brave_extension/types/actions/settingsActions.ts

+
+interface SetStoreSettingsChangeReturn {
+  type: typeof types.SET_STORE_SETTINGS_CHANGE,
+  settingsData: SettingsData | Partial<SettingsData>


nit: isn't SettingsData | Partial<SettingsData> just the same result as Partial<SettingsData>?

petemill · 2019-08-26T18:34:10Z

components/brave_extension/extension/brave_extension/state/shieldsPanelState.ts

    // do not show any badge if there are no blocked items
-    setBadgeText(tabId, total > 99 ? '99+' : total > 0 ? total.toString() : '')
+    ? total > 99 ? '99+' : total > 0 ? total.toString() : ''


nit: indent? (up to you)

petemill · 2019-08-26T18:34:42Z

components/brave_extension/extension/brave_extension/state/shieldsPanelState.ts

@@ -29,6 +29,11 @@ export const updatePersistentData: shieldState.UpdatePersistentData = (state, pe
  return { ...state, persistentData: { ...state.persistentData, ...persistentData } }
 }

+export const mergeSettingsData: shieldState.MergeSettingsData = (state, settingsData) => {
+  // return { ...state, settingsData: { ...state.settingsData, ...settingsData } }


commented out code

petemill · 2019-08-26T18:39:19Z

components/brave_extension/extension/brave_extension/background/reducers/shieldsPanelReducer.ts

@@ -279,6 +282,11 @@ export default function shieldsPanelReducer (
        })
      break
    }
+    case settingsTypes.SET_STORE_SETTINGS_CHANGE: {
+      const settingsData: SettingsData | Partial<SettingsData> = action.settingsData


nit: possibly only need Partial<SettingsData> here

petemill · 2019-08-26T18:43:02Z

components/brave_extension/extension/brave_extension/background/api/shieldsAPI.ts

-  return {
-    showAdvancedView: showAdvancedViewPref.value
-  }
+export async function getViewPreferences (): Promise<GetViewPreferencesData | {}> {


How could this function return {} and not GetViewPreferencesData? From the code it seems like it should return every property or throw an error. We want the signature to be correct so that callers can trust the type and typescript won't error if we don't check it.

it doesn't but typescript will complain not having it since let newSettings = {} is empty

I think it's ok to do let newSettings = {} as GetViewPreferencesData and keep the function return signature correct. WDYT?

petemill · 2019-08-26T19:30:01Z

chromium_src/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

@@ -96,6 +96,10 @@ void BraveAddCommonStrings(content::WebUIDataSource* html_source,
      IDS_SETTINGS_BRAVE_SHIELDS_HTTPS_EVERYWHERE_CONTROL_LABEL},
    {"noScriptControlLabel",
      IDS_SETTINGS_BRAVE_SHIELDS_NO_SCRIPT_CONTROL_LABEL},
+    {"lookFeelTitle",


nit: prefix these strings with braveShields since these are mixed with chromium strings

petemill · 2019-08-26T19:39:09Z

components/brave_extension/extension/brave_extension/background/events/settingsEvents.ts

+import settingsActions from '../actions/settingsActions'
+
+chrome.settingsPrivate.onPrefsChanged.addListener(function (settings) {
+  settingsActions.settingsDidChange(settings[0])


This will fire for every settings change in chromium, not just the 2 we care about - many completely irrelevant things. And it can contain multiple settings, not just 1. I recommend that we:

Iterate through the array and check for keys we care about (in shieldsAPI.settingsKeys)

Send all the relevant keys and new values to settingsDidChange

petemill · 2019-08-26T19:41:20Z

components/brave_extension/extension/brave_extension/background/events/webNavigationEvents.ts

@@ -12,4 +14,7 @@ chrome.webNavigation.onBeforeNavigate.addListener(function ({ tabId, url, frameI
 chrome.webNavigation.onCommitted.addListener(function ({ tabId, url, frameId }: chrome.webNavigation.WebNavigationTransitionCallbackDetails) {
  const isMainFrame: boolean = frameId === 0
  actions.onCommitted(tabId, url, isMainFrame)
+  // check whether or not the settings store should update based on settings changes.
+  // this action is needed in the onCommitted phase for edge cases such as when after Brave is re-launched
+  settingsActions.settingsDataShouldUpdate()


Seems overkill to fetch this data from the settings API every time a page is navigated, especially if we're subscribing to changes. Perhaps just check once at background script init, just before we subscribe to changes by including the events module?

petemill · 2019-08-27T02:40:57Z

@cezaraugusto Is there a code loop going on here?

chrome.settingsPrivate.onPrefsChanged -> settingsActions.settingsDidChange -> shieldsAPI.setViewPreferences -> chrome.settingsPrivate.setPreference

Seems like onPrefsChanged should just update the state.

cezaraugusto · 2019-08-28T20:25:22Z

@petemill thanks again, all comments addressed

bsclifton · 2019-09-12T20:41:54Z

Removing milestone as this is not reviewed/approved/merged 👍

Change description of --skip_signing

bsclifton · 2019-12-02T06:25:00Z

@cezaraugusto can you give this a rebase? 😄

cezaraugusto · 2020-01-23T20:34:15Z

Did a new rebase "just in case" 👀

cezaraugusto · 2020-03-02T17:07:34Z

Hello team did a new rebase, can you take a new look?

bsclifton · 2020-03-02T21:13:46Z

New setting LGTM 👍 Confirmed test plan works great too- will look at the code now

bsclifton

Code changes look good! Tests look good too

bsclifton · 2020-03-02T22:00:34Z

@petemill can you please give this a re-review 😄

bsclifton · 2020-03-03T06:09:43Z

Note for @bridiver who was flagged for patch review:
Only patch related change made was adding extra text values to chromium_src/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

See https://github.com/brave/brave-core/pull/3150/files#diff-9f40da04b5dd117f1be1af269b742ee7

bridiver

chromium_src override looks fine

bsclifton · 2020-03-04T00:29:00Z

@petemill can you please re-review 😄

petemill

💯 left some nits

petemill · 2020-03-04T19:29:04Z

components/brave_extension/extension/brave_extension/actions/settingsActions.ts

+ * Get a list of settings values from brave://settings and update if comparison
+ * against settings values from store deosn't match.
+ */
+export const settingsDataShouldUpdate: actions.SettingsDataShouldUpdate = () => {


nit: this name is confusing. Perhaps fetchAndDispatchSettings or similar?

petemill · 2020-03-04T19:31:51Z

components/brave_extension/extension/brave_extension/types/actions/settingsActions.ts

+  (settings: Settings): SetStoreSettingsChangeReturn
+}
+
+export interface SettingsDataShouldUpdate {


nit: Not sure this belongs here since it's not actually an action, can just be local to its function?

petemill · 2020-03-04T19:35:20Z

components/brave_extension/extension/brave_extension/types/other/settingsTypes.ts

+export type Settings = {
+  key: string
+  type: any
+  value: boolean


nit: this won't always be a boolean but for now it's ok. Maybe leave a (TODO: can support multiple types, see PrefType in chromel.d.ts).

petemill · 2020-03-04T19:36:22Z

components/definitions/chromel.d.ts

@@ -1,6 +1,6 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this file,
- * You can obtain one at http://mozilla.org/MPL/2.0/. */
+* License, v. 2.0. If a copy of the MPL was not distributed with this file,


did you mean to change this?

fix brave/brave-browser#3121

cezaraugusto · 2020-03-05T03:36:48Z

Thanks all for the reviews. Considering all feedback is addressed, I'm merging this one.

cezaraugusto added the feature/shields label Aug 12, 2019

cezaraugusto requested review from petemill and yrliou August 12, 2019 09:50

cezaraugusto requested a review from bridiver as a code owner August 12, 2019 09:50

cezaraugusto self-assigned this Aug 12, 2019

petemill requested changes Aug 12, 2019

View reviewed changes

petemill reviewed Aug 12, 2019

View reviewed changes

petemill reviewed Aug 14, 2019

View reviewed changes

bsclifton added this to the 0.71.x - Nightly milestone Aug 20, 2019

innergy mentioned this pull request Aug 20, 2019

Option to hide Brave Shields activity count brave/brave-browser#3121

Closed

cezaraugusto force-pushed the ca-3121 branch 3 times, most recently from cbf3611 to 557e806 Compare August 26, 2019 17:31

petemill reviewed Aug 26, 2019

View reviewed changes

petemill requested changes Aug 26, 2019

View reviewed changes

cezaraugusto force-pushed the ca-3121 branch from 557e806 to 093a7e5 Compare August 28, 2019 20:24

cezaraugusto force-pushed the ca-3121 branch from 093a7e5 to 35ac4f2 Compare August 28, 2019 20:40

bsclifton removed this from the 0.71.x - Dev milestone Sep 12, 2019

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Merge pull request #3150 from brave/enable_skip_signing_on_windows

9ba0599

Change description of --skip_signing

bsclifton self-requested a review December 2, 2019 06:24

cezaraugusto force-pushed the ca-3121 branch 2 times, most recently from 30c8e8b to cc16fae Compare December 3, 2019 18:01

cezaraugusto force-pushed the ca-3121 branch from cc16fae to 409c59a Compare January 23, 2020 20:33

cezaraugusto requested a review from petemill January 23, 2020 20:33

cezaraugusto force-pushed the ca-3121 branch from 409c59a to 8667715 Compare March 2, 2020 17:02

bsclifton approved these changes Mar 2, 2020

View reviewed changes

bridiver approved these changes Mar 4, 2020

View reviewed changes

bsclifton added this to the 1.7.x - Nightly milestone Mar 4, 2020

petemill approved these changes Mar 4, 2020

View reviewed changes

Shields: Add setting to hide activity count

a692ec9

fix brave/brave-browser#3121

cezaraugusto force-pushed the ca-3121 branch from 8667715 to a692ec9 Compare March 4, 2020 23:13

cezaraugusto merged commit 0985439 into master Mar 5, 2020

cezaraugusto deleted the ca-3121 branch March 5, 2020 03:37

		@@ -7,6 +7,11 @@ import * as Shields from '../../types/state/shieldsPannelState'

		const keyName = 'shields-persistent-data'

		export const initialSettingsData = {

[brave-extension] add settings option to hide Shields activity count #3150

[brave-extension] add settings option to hide Shields activity count #3150

Conversation

cezaraugusto commented Aug 12, 2019 • edited by bsclifton Loading

Test Plan:

Reviewer Checklist:

After-merge Checklist:

petemill 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

petemill Aug 14, 2019 • 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

cezaraugusto commented Aug 26, 2019

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill Aug 27, 2019 • 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

petemill commented Aug 27, 2019

cezaraugusto commented Aug 28, 2019

bsclifton commented Sep 12, 2019

bsclifton commented Dec 2, 2019

cezaraugusto commented Jan 23, 2020

cezaraugusto commented Mar 2, 2020

bsclifton commented Mar 2, 2020

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Mar 2, 2020

bsclifton commented Mar 3, 2020

bridiver left a comment

Choose a reason for hiding this comment

bsclifton commented Mar 4, 2020

petemill 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

cezaraugusto commented Mar 5, 2020

cezaraugusto commented Aug 12, 2019 •

edited by bsclifton

Loading

petemill Aug 14, 2019 •

edited

Loading

petemill Aug 27, 2019 •

edited

Loading