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

Add settings section for clearing browser history #1738

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JayKid
Copy link

@JayKid JayKid commented Sep 15, 2021

Description

Minimal version of the settings pane to clear browsing history.

Sadly, the functionality is not there:

  1. First attempt was to use places.deleteAllHistory() but it's not available in this scope and I can't use require/import to fetch it

  2. Tried using directly postMessage as places internally does but that postMessage within places is directed to the particular worker so this didn't work either

Also, I used inline styles which should be migrated to some stylesheet, but just wanted to put a quick PoC together to ask for pointers when it comes to actually clearing the DB of its stored URLs.

New settings section

Screenshot 2021-09-15 at 13 18 04

Screenshot 2021-09-15 at 13 21 35

Screenshot 2021-09-15 at 13 21 41

Minimal version of the settings pane to clear browsing history.

Sadly, the functionality is not there:

1. First attempt was to use `places.deleteAllHistory()` but it's not available in this scope and I can't use require/import to fetch it

2. Tried using directly postMessage as places internally does but that postMessage within places is directed to the particular worker so this didn't work either

Also used inline styles which should be migrated to some stylesheet, but just wanted to put a quick PoC together to ask for pointers when it comes to actually clearing the DB of its stored URLs.
@PalmerAL
Copy link
Collaborator

The right way to do this would be postMessage to the preload script -> send an IPC from there (like this) -> have an IPC listener in the UI process that calls places.deleteHistory(). But I'm not sure that's really the right solution anyway - there's a "clear all history" button in the history view already. So I think what we want is just the "clear history on closing the browser" part.

For that, you'd need two things:

  1. History gets written to disk here: https://github.com/minbrowser/min/blob/master/js/places/placesWorker.js#L165. If the setting is enabled you'd want to skip that and just keep it in-memory.
  2. When the app is quit, you'd need to clear cookies and site data. The Electron API for this is asynchronous, so you'd need to restructure the quitting process a bit to let that finish before quitting.

But thinking about this more (and I'm thinking and writing this at the same time), I wonder if "clear history on quit" is really the right behavior:

  • Some people leave their browser open for weeks and never quit, and so this isn't useful to them.
  • If you accidentally quit in the middle of a session and re-open, all your history is gone.
  • Some people might want to keep history for a shorter time (like 1 week), which this doesn't do anything for.

So another, possibly better, way we could do this would be:

  • Have some different options for how long to keep history, from 1 hour up to the current time of 6 weeks.
  • Whenever we delete an item from history, check whether there are other items from that domain still in history. If there are none left, call clearStorageData for that domain.
    • I think this would also provide a privacy improvement for users who keep the default setting of 6 weeks, since cookies can sometimes last longer than that.

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.

2 participants