-
Notifications
You must be signed in to change notification settings - Fork 871
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
Add support for first party storage removal on website close. #16470
Conversation
328f8db
to
f89d283
Compare
A Storybook has been deployed to preview UI for the latest push |
f89d283
to
05a1ff2
Compare
A Storybook has been deployed to preview UI for the latest push |
05a1ff2
to
d17fbed
Compare
A Storybook has been deployed to preview UI for the latest push |
This looks great @goodov, but i don't think it fully resolves brave/brave-browser#26465. We still need the second point in #26465 before we close #26465 out.
Never the less, this is great stuff! |
yes, definitely! I've removed "Resolves" keyword from the description. Can you take a look at the UI strings and maybe suggest some changes (if any)? |
@bradleyrichter wanted to ping you and your team on the wording here. We started discussing this a while back, but i dont think the design team landed on final text you all preferred. What do you all think is the ideal label here for what's called "Forget by default" in the video? |
d17fbed
to
79e7485
Compare
A Storybook has been deployed to preview UI for the latest push |
79e7485
to
a1db8d6
Compare
A Storybook has been deployed to preview UI for the latest push |
60d9782
to
da3e16f
Compare
A Storybook has been deployed to preview UI for the latest push |
da3e16f
to
ddc3fba
Compare
A Storybook has been deployed to preview UI for the latest push |
ddc3fba
to
4bd207b
Compare
112bbf1
to
0e3d310
Compare
if (new_domain == previous_domain) | ||
return; | ||
net::URLToEphemeralStorageDomain(last_committed_url); | ||
if (new_domain != previous_domain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of lines of comments wouldn't hurt
using FirstPartyStorageLifetimeKey = | ||
std::pair<content::BrowserContext*, url::Origin>; | ||
|
||
class FirstPartyStorageLifetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class level comment?
|
||
namespace ephemeral_storage { | ||
|
||
const char kFirstPartyStorageURLsToCleanup[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
private: | ||
void OnCanEnable1PESForUrl(const GURL& url, | ||
base::OnceCallback<void(bool)> on_ready, | ||
bool can_enable_1pes); | ||
bool IsDefaultCookieSetting(const GURL& url) const; | ||
|
||
void CleanupFirstPartyStorageAreasOnStartup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments, i.e. why / what for this is needed and used
0e3d310
to
04f2002
Compare
04f2002
to
48aadd6
Compare
@brave/android-tests-reviewers PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Forget by default option is implemented as "Block cross-site cookies" + logic to delete cookies on website close.
The availability of the new mode is controlled by
Enable First Party Storage Cleanup support
onbrave://flags
or--enable-feature=BraveForgetFirstPartyStorage
.This PR doesn't change the "default" state for the shields cookie policy if the feature is enabled (it's still "Block cross-site cookies").
Spec: https://docs.google.com/document/d/1TeDqwnKqOJqSv0b80liZjAFR1BUPc6urGkkHiHN526s/
4cpjfSluSp.mp4
Core part for brave/brave-browser#26465
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: