set shields interface views as a global default in settings #2973

cezaraugusto · 2019-07-22T16:48:20Z

close brave/brave-browser#4784

Submitter Checklist:

Test Plan:

Settings --> Shields

Go to brave://settings/shields
Select Simple View
Go to a website
Open Shields
--> Expected: Chceck shields view is Simple
Go to brave://settings/shields again
Select Advanced View
Go to a website
Open shields
--> Expected: Chceck shields view is Advanced (larger)

Shields --> Settings

Clean profile
Go to a website
Open shields
--> Expected: Shields opens in Simple view (clean profile defaults to "simple")
Toggle option to "Advanced view" inside shields popup
Visit brave://settings/shields in another window or tab
--> Expected: "Default view" has 'Advanced view' selected
Visit website again
Open Shields
Click 'Simple View' inside shields popup
Switch back to Settings tab / window
--> Expected: "Default view" has 'Simple view' selected

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.

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html

simonhong · 2019-07-29T23:34:16Z

IMO, using webui message seems more proper than extension api for this. WDYT? cc: @petemill

petemill · 2019-07-30T04:14:49Z

@simonhong This is using the existing chrome.braveShields extensions API as this is to enable the set / get in the extension browserAction popup. The settings page itself uses the regular settingsPrivate API (via the prefs attribute). Unless I'm mistaken, we don't or can't use WebUIMessageHandler in an extension.

petemill

👍 🤜 🤛 💯 Pushed a successful rebase on master. Ran through some manual tests and I updated the PR description with more detailed test case of what I ran through (observing both settings page and shields popup reflects the setting value). Settings page addition looks great 🎨.

Let's see what CI has to say...

simonhong · 2019-07-30T05:24:02Z

@petemill ~~Oops, I thought above capture image is part of settings page :)~~
I see. we want to set it from shields panel and settings page. If so, shields extension api is right choice :)

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html

common/extensions/api/brave_shields.json

browser/extensions/api/brave_shields_api.cc

common/pref_names.cc

petemill · 2019-07-31T20:13:35Z

I pushed a change to use the chrome.settingsPrivate API and to make the Preference name more descriptive and shields-specific. @cezaraugusto @bridiver please re-review with these changes: 62420d4...f97caaf

components/brave_extension/extension/brave_extension/manifest.json

petemill · 2019-07-31T22:36:28Z

patches/chrome-common-extensions-api-_permission_features.json.patch

+     "whitelist": [
+       "63ED55E43214C211F82122ED56407FF1A807F2A3",  // Media Router Dev
+       "226CF815E39A363090A1E547D53063472B8279FA"   // Media Router Release
+      , "A321D47A2B4CA86898167A55CA8B2E02385EA7CD" // Brave Shields


@bridiver there's a few ways to approach this:

Keep this .patch of _permission_features.json

Overwrite the settingsPrivate entry in _api_features.json using the same technique we're doing for other APIs. Disadvantage: requires duplicating security-focused chromium code and is a risk IMO.

Overwrite _permission_features.json in the same way we overwrite _api_features.json. Comes with same risk as 2)?.

Invent a new way to inject additions to the JSON of _api_features.json or _permission_features.json without overwriting the chromium source.

(Preferrably as a follow-up to unblock @cezaraugusto 's feature in this PR).

see brave/common/extensions/api/_api_features.json

oh, this is permissions, but easy to copy it

this is pretty trivial change so I think we should fix before merging

@bridiver added _api_permissions.json, please review

components/brave_extension/extension/brave_extension/braveShieldsPanel.tsx

close brave/brave-browser#4784

petemill · 2019-08-01T04:59:17Z

Fixed the unit tests by mocking chrome.settingsPrivate here: https://github.com/brave/brave-core/pull/2973/files#diff-af22ed52c8ebf2799010b2d6e01b7107R113

cezaraugusto · 2019-08-01T14:46:33Z

@bridiver @petemill seems all feedback was addressed. do we need anything else to finish up this PR?

common/pref_names.cc

petemill

LGTM - CI looks like it had passed apart from a lint warning, which I pushed a fix for

common/extensions/api/_permission_features.json

…eShields.{get,set}ViewPreferences API

…enabled

kjozwiak · 2019-08-02T15:47:28Z

Please make sure that the PR's and associated issues are moved into milestones once completed 👍

bsclifton · 2019-08-15T05:08:46Z

@cezaraugusto does this need to go to 0.69.x also? I tried to cherry-pick, but ran into some merge conflicts in components/definitions/chromel.d.ts. If merge is safe there, I can create an uplift for you. Let me know! 😄

set shields interface views as a global default in settings

bsclifton · 2019-08-15T07:26:58Z

@cezaraugusto created #3187 - can you help me make sure this looks good? 😄

set shields interface views as a global default in settings

cezaraugusto self-assigned this Jul 24, 2019

cezaraugusto force-pushed the ca-4784 branch 2 times, most recently from d8a4161 to 6c471e3 Compare July 24, 2019 17:33

petemill mentioned this pull request Jul 24, 2019

Shields gets getViewPreferences and setViewPreferences API #3001

Merged

petemill reviewed Jul 24, 2019

View reviewed changes

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html Outdated Show resolved Hide resolved

cezaraugusto force-pushed the ca-4784 branch 3 times, most recently from 53ce545 to 3b52ba4 Compare July 29, 2019 21:15

cezaraugusto requested review from petemill, simonhong and bbondy July 29, 2019 21:18

cezaraugusto force-pushed the ca-4784 branch from 3b52ba4 to c1e6cd5 Compare July 29, 2019 21:22

cezaraugusto marked this pull request as ready for review July 29, 2019 21:22

cezaraugusto requested a review from bridiver as a code owner July 29, 2019 21:22

petemill force-pushed the ca-4784 branch from c1e6cd5 to ed0b332 Compare July 30, 2019 04:37

petemill previously approved these changes Jul 30, 2019

View reviewed changes

Shields gets getViewPreferences and setViewPreferences API

cde4417

petemill dismissed their stale review via 62420d4 July 30, 2019 05:05

petemill force-pushed the ca-4784 branch from ed0b332 to 62420d4 Compare July 30, 2019 05:05

petemill previously approved these changes Jul 30, 2019

View reviewed changes

petemill reviewed Jul 30, 2019

View reviewed changes

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html Outdated Show resolved Hide resolved

petemill reviewed Jul 30, 2019

View reviewed changes

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html Outdated Show resolved Hide resolved

bridiver reviewed Jul 30, 2019

View reviewed changes

common/extensions/api/brave_shields.json Outdated Show resolved Hide resolved

bridiver reviewed Jul 30, 2019

View reviewed changes

browser/extensions/api/brave_shields_api.cc Outdated Show resolved Hide resolved

petemill reviewed Jul 31, 2019

View reviewed changes

common/pref_names.cc Outdated Show resolved Hide resolved

petemill dismissed their stale review via 29104e2 July 31, 2019 20:10

diracdeltas reviewed Jul 31, 2019

View reviewed changes

components/brave_extension/extension/brave_extension/manifest.json Show resolved Hide resolved

petemill force-pushed the ca-4784 branch 2 times, most recently from 2025e6c to bded845 Compare July 31, 2019 22:31

petemill reviewed Jul 31, 2019

View reviewed changes

bridiver reviewed Jul 31, 2019

View reviewed changes

components/brave_extension/extension/brave_extension/braveShieldsPanel.tsx Show resolved Hide resolved

set shields interface views as a global default in settings

7e860d5

close brave/brave-browser#4784

petemill force-pushed the ca-4784 branch from bded845 to 6783675 Compare August 1, 2019 04:44

petemill reviewed Aug 1, 2019

View reviewed changes

common/pref_names.cc Outdated Show resolved Hide resolved

petemill force-pushed the ca-4784 branch from 6783675 to a1d11a5 Compare August 1, 2019 17:26

petemill previously approved these changes Aug 1, 2019

View reviewed changes

bridiver reviewed Aug 1, 2019

View reviewed changes

common/extensions/api/_permission_features.json Outdated Show resolved Hide resolved

petemill added 2 commits August 1, 2019 14:51

Shields uses chrome.settingsPrivate API instead of custom chrome.brav…

f5395db

…eShields.{get,set}ViewPreferences API

Shields: more specific Preferences name for whether advanced view is …

4838004

…enabled

petemill dismissed their stale review via 4838004 August 1, 2019 21:55

petemill force-pushed the ca-4784 branch from a1d11a5 to 4838004 Compare August 1, 2019 21:55

bridiver approved these changes Aug 1, 2019

View reviewed changes

cezaraugusto merged commit 2b9340b into master Aug 2, 2019

cezaraugusto deleted the ca-4784 branch August 2, 2019 14:12

kjozwiak added this to the 0.70.x - Nightly milestone Aug 2, 2019

cezaraugusto assigned petemill Aug 2, 2019

bsclifton pushed a commit that referenced this pull request Aug 15, 2019

Merge pull request #2973 from brave/ca-4784

4a54120

set shields interface views as a global default in settings

bsclifton mentioned this pull request Aug 15, 2019

set shields interface views as a global default in settings (uplift to 0.69.x) #3187

Merged

4 tasks

bsclifton pushed a commit that referenced this pull request Aug 27, 2019

Merge pull request #2973 from brave/ca-4784

a15b76e

set shields interface views as a global default in settings

bsclifton mentioned this pull request Sep 3, 2019

Shield View is NOT same in Normal and TOR tab for the same site brave/brave-browser#5872

Closed

btlechowski mentioned this pull request Sep 3, 2019

Shields: Add Default view option in settings for simple and advanced views brave/brave-browser#4784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set shields interface views as a global default in settings #2973

set shields interface views as a global default in settings #2973

cezaraugusto commented Jul 22, 2019 •

edited by petemill

Loading

simonhong commented Jul 29, 2019

petemill commented Jul 30, 2019

petemill left a comment

simonhong commented Jul 30, 2019 •

edited

Loading

petemill commented Jul 31, 2019 •

edited

Loading

petemill Jul 31, 2019 •

edited

Loading

bridiver Jul 31, 2019

bridiver Jul 31, 2019

bridiver Jul 31, 2019

petemill Aug 1, 2019

petemill commented Aug 1, 2019

cezaraugusto commented Aug 1, 2019

petemill left a comment •

edited

Loading

kjozwiak commented Aug 2, 2019

bsclifton commented Aug 15, 2019

bsclifton commented Aug 15, 2019

set shields interface views as a global default in settings #2973

set shields interface views as a global default in settings #2973

Conversation

cezaraugusto commented Jul 22, 2019 • edited by petemill Loading

Submitter Checklist:

Test Plan:

Settings --> Shields

Shields --> Settings

Reviewer Checklist:

After-merge Checklist:

simonhong commented Jul 29, 2019

petemill commented Jul 30, 2019

petemill left a comment

Choose a reason for hiding this comment

simonhong commented Jul 30, 2019 • edited Loading

petemill commented Jul 31, 2019 • edited Loading

petemill Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

bridiver Jul 31, 2019

Choose a reason for hiding this comment

bridiver Jul 31, 2019

Choose a reason for hiding this comment

bridiver Jul 31, 2019

Choose a reason for hiding this comment

petemill Aug 1, 2019

Choose a reason for hiding this comment

petemill commented Aug 1, 2019

cezaraugusto commented Aug 1, 2019

petemill left a comment • edited Loading

Choose a reason for hiding this comment

kjozwiak commented Aug 2, 2019

bsclifton commented Aug 15, 2019

bsclifton commented Aug 15, 2019

cezaraugusto commented Jul 22, 2019 •

edited by petemill

Loading

simonhong commented Jul 30, 2019 •

edited

Loading

petemill commented Jul 31, 2019 •

edited

Loading

petemill Jul 31, 2019 •

edited

Loading

petemill left a comment •

edited

Loading