Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

add a cookie manager #11107

Closed
wants to merge 2 commits into from
Closed

add a cookie manager #11107

wants to merge 2 commits into from

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Sep 23, 2017 •
edited
Loading

this allows users to view all the cookies, sort them, and delete individual ones or in bulk.

fix #1991

todo: add ability to edit cookies and add to global whitelist/blacklist

test plan:

  1. automated tests should pass
  2. browse some sites in various browsing sessions (session tab, regular tab, private tab, etc.)
  3. go to about:cookies
  4. you should see cookies for those sites
  5. click on a cookie and right-click to delete or copy. both should work.
  6. repeat step 5 but for multiple selected cookies at once
  7. select some cookies, hit delete key. they should be deleted.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@diracdeltas diracdeltas added this to the 0.19.x (Beta Channel) milestone Sep 23, 2017
@diracdeltas diracdeltas self-assigned this Sep 23, 2017
@diracdeltas diracdeltas modified the milestones: 0.19.x (Beta Channel), 0.21.x (Nightly Channel) Sep 23, 2017
diracdeltas
diracdeltas commented Sep 23, 2017
View reviewed changes
js/about/cookies.js
const SortableTable = require('../../app/renderer/components/common/sortableTable')
const aboutActions = require('./aboutActions')
const messages = require('../constants/messages')
const moment = require('moment')
Copy link
Member Author

Choose a reason for hiding this comment

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

if #11061 is merged before this, i'll replace moment with date-fns

@codecov-io
Copy link

codecov-io commented Sep 23, 2017 •
edited
Loading

Codecov Report

Merging #11107 into master will decrease coverage by 0.15%.
The diff coverage is 18.3%.

@@            Coverage Diff             @@
##           master   #11107      +/-   ##
==========================================
- Coverage    52.5%   52.34%   -0.16%     
==========================================
  Files         268      268              
  Lines       25241    25343     +102     
  Branches     4028     4042      +14     
==========================================
+ Hits        13252    13267      +15     
- Misses      11989    12076      +87
Flag Coverage Δ
#unittest 52.34% <18.3%> (-0.16%) ⬇️
Impacted Files Coverage Δ
js/lib/appUrlUtil.js 84.21% <ø> (ø) ⬆️
js/constants/messages.js 100% <ø> (ø) ⬆️
js/constants/appConstants.js 100% <ø> (ø) ⬆️
app/renderer/components/common/sectionTitle.js 82.35% <ø> (ø) ⬆️
js/about/preferences.js 45.81% <ø> (-0.05%) ⬇️
js/stores/appStore.js 15.28% <0%> (-0.52%) ⬇️
app/browser/menu.js 50.87% <0%> (-1.06%) ⬇️
js/about/aboutActions.js 9.09% <0%> (-0.24%) ⬇️
js/actions/appActions.js 16.41% <0%> (-0.1%) ⬇️
js/contextMenus.js 18.19% <10%> (-0.24%) ⬇️
... and 12 more

luixxiul pushed a commit that referenced this pull request Sep 23, 2017
Polish about:cookies design with sectionTitle and BEM naming
49b36b5 
Proposal for #11107

- Apply AboutPageSectionTitle to the header
- Restyled with BEM naming

Auditors:

Test Plan:
1. Open about:cookies
2. Make sure elements inside the header are aligned properly
3. Click 'Delete All Cookies'
4. Make sure 'No cookies saved' is displayed under the header
@luixxiul
Copy link
Contributor

luixxiul commented Sep 23, 2017 •
edited
Loading

I tried to polish the design of the manager with this commit : 49b36b5. It would be appreciated if you would refer or cherry-pick it. Thanks!

screenshot 2017-09-23 20 17 11

screenshot 2017-09-23 20 02 05

diracdeltas pushed a commit that referenced this pull request Sep 23, 2017
@diracdeltas
Polish about:cookies design with sectionTitle and BEM naming
90b198e 
Proposal for #11107

- Apply AboutPageSectionTitle to the header
- Restyled with BEM naming

Auditors:

Test Plan:
1. Open about:cookies
2. Make sure elements inside the header are aligned properly
3. Click 'Delete All Cookies'
4. Make sure 'No cookies saved' is displayed under the header
@diracdeltas
Copy link
Member Author

^ cherry-picked

@diracdeltas diracdeltas force-pushed the feature/cookie-manager branch from 90b198e to 5eb8540 Compare October 12, 2017 22:26
diracdeltas pushed a commit that referenced this pull request Oct 12, 2017
@diracdeltas
Polish about:cookies design with sectionTitle and BEM naming
5eb8540 
Proposal for #11107

- Apply AboutPageSectionTitle to the header
- Restyled with BEM naming

Auditors:

Test Plan:
1. Open about:cookies
2. Make sure elements inside the header are aligned properly
3. Click 'Delete All Cookies'
4. Make sure 'No cookies saved' is displayed under the header
@diracdeltas diracdeltas requested review from bsclifton and bbondy and removed request for bbondy and bsclifton October 12, 2017 22:27
diracdeltas and others added 2 commits October 12, 2017 22:29
@diracdeltas
add a cookie manager
727ed4b 
this allows users to view all the cookies, sort them, and delete individual ones or in bulk.

fix #1991

todo: add ability to edit cookies and add to global whitelist/blacklist
@diracdeltas
Polish about:cookies design with sectionTitle and BEM naming
deb1343 
Proposal for #11107

- Apply AboutPageSectionTitle to the header
- Restyled with BEM naming

Auditors:

Test Plan:
1. Open about:cookies
2. Make sure elements inside the header are aligned properly
3. Click 'Delete All Cookies'
4. Make sure 'No cookies saved' is displayed under the header
@diracdeltas diracdeltas force-pushed the feature/cookie-manager branch from 5eb8540 to deb1343 Compare October 12, 2017 22:29
@diracdeltas
Copy link
Member Author

rebased

@cezaraugusto cezaraugusto mentioned this pull request Oct 16, 2017
Closed
@cezaraugusto
Copy link
Contributor

looks great but after playing around for a while the browser has frozen for several times due to many cookies I received. Site visited was wired.com, quora.com, and google.com.

This is not really related to this PR but more like with the way we handle large amounts of tabular data. I created #11551 to improve this and set to the same milestone.

This is a blocker for me but a quick solution would be to limit the max number of cookies, similar to how we do for history. Not sure about the implications on privacy.

If limiting is good enough lmk I'll make another review or otherwise, I'm working on a solution.

@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
@luixxiul
Copy link
Contributor

cc @cezaraugusto on triage (do you think this PR should be postponed?)

@luixxiul luixxiul added needs-info Another team member needs information from the PR/issue opener. PR/needs-rebase ۞ labels Oct 25, 2017
@bbondy bbondy modified the milestones: 0.20.x (Beta Channel), Backlog Oct 27, 2017
@diracdeltas
Copy link
Member Author

marking as blocked on #11551 for now

@luixxiul luixxiul mentioned this pull request Nov 12, 2017
Closed
@uriesk uriesk mentioned this pull request Nov 12, 2017
Closed
@bsclifton bsclifton modified the milestones: Triage Backlog, Completed work Feb 28, 2018
@alexwykoff
Copy link
Contributor

this needs a design review because cookie lists can get hella long

@diracdeltas
Copy link
Member Author

we're probably just going to close this in favor of waiting til chromium fork

@diracdeltas diracdeltas added the post-v1 We don't expect to be able to resolve this before releasing v1.0 with Brave Core (instead of Muon). label Apr 10, 2018
@bsclifton bsclifton removed this from the Completed work milestone Jul 13, 2018
@bsclifton
Copy link
Member

Closing as this is marked with post-v1 and the accompanying issue (#1991) has been closed

@bsclifton bsclifton closed this Jul 13, 2018
@bsclifton bsclifton deleted the feature/cookie-manager branch August 13, 2018 07:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bsclifton bsclifton Awaiting requested review from bsclifton

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@diracdeltas diracdeltas

@bradleyrichter bradleyrichter

Labels
feature/about-pages needs-info Another team member needs information from the PR/issue opener. post-v1 We don't expect to be able to resolve this before releasing v1.0 with Brave Core (instead of Muon). PR/blocked PR/needs-rebase ۞ QA/test-plan-specified wontfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Individual Cookie management feature request
8 participants
@diracdeltas @codecov-io @luixxiul @cezaraugusto @alexwykoff @bsclifton @bbondy @bradleyrichter