Skip to content
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

Applied Shields v2 UX to settings and shields panel and use FP V2 by default #5684

Merged
merged 4 commits into from
Jun 4, 2020

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented May 28, 2020

Resolves brave/brave-browser#9194
Resolves brave/brave-browser#9975


Screen Shot 2020-05-29 at 12 05 59 PM


Screen Shot 2020-05-29 at 1 15 26 PM


Screen Shot 2020-05-29 at 1 15 34 PM


Screen Shot 2020-05-29 at 1 15 43 PM


Screen Shot 2020-05-28 at 8 01 15 PM

Submitter Checklist:

Test Plan:

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.

@simonhong
Copy link
Member Author

@pes10k @rebron I think this PR is ready to review :)

@rebron
Copy link
Collaborator

rebron commented May 29, 2020

Labels and renaming looks great. Will leave the rest to @pes10k.

Copy link
Contributor

@pes10k pes10k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!

@pes10k
Copy link
Contributor

pes10k commented May 30, 2020

This all looks great to me, terrific @simonhong !

@simonhong
Copy link
Member Author

@bridiver ping to owners review.

case ControlType::BLOCK:
return "block";
case ControlType::BLOCK_THIRD_PARTY:
FALLTHROUGH;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this also need a v2 check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the UI, same string default is only used for BLOCK_THIRD_PARTY and DEFAULT.

@simonhong
Copy link
Member Author

@rebron I'll revisit and update this PR again after fingerprint v2 is enabled by default.
@bridiver told me that these new option names are valid only for v2. cc: @pes10k

@pes10k
Copy link
Contributor

pes10k commented Jun 2, 2020

Yep, these are for v2 fingerprinting, but we're aiming to ship v2 in 1.11, so should all line up :)

Right now the v2 fingerprinting protects haven't been fully implemented, but have eclipsed what was available in v1, so switching over to v2, even as is, is strictly superior

@simonhong simonhong changed the title Applied Shields v2 UX to settings and shields panel Applied Shields v2 UX to settings and shields panel and use FP V2 by default Jun 3, 2020
map->SetContentSettingCustomScope(
ContentSettingsPattern::Wildcard(), ContentSettingsPattern::Wildcard(),
ContentSettingsType::PLUGINS, brave_shields::kFingerprinting,
ContentSettingsType::PLUGINS, brave_shields::kFingerprintingV2,
CONTENT_SETTING_BLOCK);
map->SetContentSettingCustomScope(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not relevant anymore, there is no longer a first/third party scope. Can you please cleanup the tests in a followup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonhong simonhong merged commit bc143f2 into master Jun 4, 2020
@simonhong simonhong deleted the use_v2_shields_ux branch June 4, 2020 01:25
petemill pushed a commit that referenced this pull request Jul 27, 2020
Delete brave_infura_project_id arg on mobile
petemill pushed a commit that referenced this pull request Jul 28, 2020
Delete brave_infura_project_id arg on mobile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants