Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix#2907: Clearing Browsing History should clear Tab History as well #3089

Merged
merged 7 commits into from
Jan 7, 2021

Conversation

soner-yuksel
Copy link
Contributor

@soner-yuksel soner-yuksel commented Nov 26, 2020

Summary of Changes

This pull request fixes #2907

  • This PR makes changes to Tab History when Browsing History is cleared
  • After clearing the browser history from settings, the tab history for every tab is also cleared.
  • In addition the history data of the saved tabs is also cleared in order to achieve when the browser is reloaded it will not bring back the cleared history.

Security Review

https://github.com/brave/security/issues/268

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

  • Visit websites in different tabs or same tab
  • Open settings - Navigate to Privacy - Clear Data for Browsing History
  • Check the Tab History is also cleared

Screenshots:

Issue:2907

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).

@soner-yuksel soner-yuksel changed the title Fix#2904: Clearing Browsing History should clear Tab History as well Fix#2907: Clearing Browsing History should clear Tab History as well Nov 26, 2020
Client/Frontend/Browser/Tab.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/Tab.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/Tab.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

left feedback in DMs

@soner-yuksel soner-yuksel added this to the 1.23 milestone Dec 2, 2020
@soner-yuksel
Copy link
Contributor Author

left feedback in DMs

Not Registering first entered URL problem is fixed and method is changed with "clear"

Creating the security ticket for review

@soner-yuksel soner-yuksel requested a review from iccub December 3, 2020 18:33
@soner-yuksel soner-yuksel self-assigned this Dec 4, 2020
*/
let argument: [Any] = ["_c", "lea", "r"]

let method = argument.compactMap { $0 as? String }.joined()
Copy link
Collaborator

Choose a reason for hiding this comment

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

PR Looks good. This MIGHT not work if the optimizer is smart. If it is, this will get optimized back into a single string _clear and then get flagged by the automatic review for private methods :D
Typically I base-64 encode the string and then decode it at runtime. But for now, leave it and let's see. Just adding a comment in case it does get rekt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I added extra obfuscation which includes base-64 encode and unicode encoding but we decided it is unnecessary.
But like you said let's see, we can put it back.

@soner-yuksel soner-yuksel requested a review from jumde December 23, 2020 14:41
Copy link
Contributor

@jumde jumde left a comment

Choose a reason for hiding this comment

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

lgtm

@soner-yuksel soner-yuksel merged commit aef4874 into development Jan 7, 2021
@soner-yuksel soner-yuksel deleted the tab-history-clear branch January 7, 2021 22:42
@kylehickinson kylehickinson removed this from the 1.23 milestone Feb 1, 2021
@kjozwiak kjozwiak added this to the 1.23 milestone Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clearing Browsing history should clear tab history as well
6 participants