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

Brave ephemeral storage flag #7473

Closed
wants to merge 4 commits into from
Closed

Conversation

cathiechen
Copy link

Resolves

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

mrobinson and others added 3 commits December 17, 2020 17:29
Instead of using ephemeral storage for all unblocked third-party frame
storage, only use ephemeral storage when third-party storage is blocked.
This means that turning on the ephemeral storage flag will always
replaced blocked storage in third-party frames with an ephemeral
version, regardless of other settings.
@cathiechen cathiechen requested a review from a team as a code owner December 17, 2020 17:16
Copy link
Contributor

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Nice work. I think it makes sense to rebase and to merge all of these commits into one before sending on to be reviewed by core Brave reviewers, with you as the commit author.

Copy link
Author

@cathiechen cathiechen left a comment

Choose a reason for hiding this comment

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

I see. It's checked inside RestrictedCookieManager::CookieListToGetAllForUrlCallback.

if (url::Origin::Create(url) == top_frame_origin)
return false;
return true;
return !cookie_settings->IsCookieAccessAllowed(
Copy link
Author

Choose a reason for hiding this comment

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

@mrobinson Do you think we should check IsCookieAccessOrEphemeralCookiesAccessAllowed first in this function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean to add a DCHECK here for that? If so, I think that'd be just fine.

Copy link
Author

Choose a reason for hiding this comment

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

I meant

if (!IsCookieAccessOrEphemeralCookiesAccessAllowed())
  return false;

RestrictedCookieManager::CookieListToGetAllForUrlCallback will check it. So no need to add it here.

@cathiechen
Copy link
Author

Nice work. I think it makes sense to rebase and to merge all of these commits into one before sending on to be reviewed by core Brave reviewers, with you as the commit author.

Make sense. Sorry for the confusion. Let's move to #7207 I'll close this one:)

@cathiechen cathiechen closed this Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants