Browser-laptop: "Clear browsing data" user settings

[Kanichiro]

Did you search for similar issues before submitting this one?
Yes
Describe the issue you encountered:

Expected behavior:
Settings to stay On

• Platform (Win7, 8, 10? macOS? Linux distro?):
  Linux Mint 18 - Cinnamon
• Brave Version:
  See screenshot
• Steps to reproduce:
  
  
  
  1. From the History menu, click "Clear all Cookies and Site Data."
  
  
  2. Move the selector bars for Browser History & Download History from off to on
  
  
  3. Click Clear
  
  
• Screenshot if needed:
  see screen shots
• Any related issues:
  Everytime I select "Clear browsing data," I am forced to move the selector bars for Browser History & Download History from off to on. I tend to delete these items often, even hourly at times as I can visit a lot of news sites and I prefer to delete what these sites store on my computer.

Please allow your Users to make these settings default or not. I should be able to just select ON as my prefer setting and have them stay ON!

[maazadeeb]

@bsclifton I would like to take this up. 😄 I did some initial investigation. Seems like the state of the setting is not being persisted. Could you point me to a example where the state is being persisted and also retrieved? Will persisting the settings at https://github.com/brave/browser-laptop/blob/master/js/components/clearBrowsingDataPanel.js#L32 be sufficient?

[bsclifton]

Hi @maaz93! Let's do this :smile:

So our persistent session contains Window settings and Application settings. This requested feature is definitely an Application setting, since it would be global. Here's our documented storage format (you're interested in the AppStore):
https://github.com/brave/browser-laptop/blob/master/docs/state.md

So what we could do would be to add a new item there called clearBrowsingDataDefaults:

    ...
  },
  about: {
    newtab: {
      gridLayout: string // 'small', 'medium', 'large'
    }
  },
  menu: {
    template: object // used on Windows and by our tests: template object with Menubar control
  },
  defaultBrowserCheckComplete: boolean // true to indicate default browser check is complete
  clearBrowsingDataDefaults: {
    clearBrowserHistory: boolean,
    clearDownloadHistory: boolean,
    ...

[bsclifton]

Of course, docs/state.md is only for documentation, so we'd need to implement code that reads from / writes to this new proposed value. The js/components/clearBrowsingDataPanel.js control that you linked to in your post is included from js/components/main.js:
https://github.com/brave/browser-laptop/blob/master/js/components/main.js#L982

Here, you have an opportunity (since js/components/main.js has access to the application state access here) to read from the AppState and bind the value we get back as a property for the React control. For example:

<ClearBrowsingDataPanel
  clearBrowsingDataDetail={this.props.windowState.get('clearBrowsingDataDetail')}
  onHide={this.onHideClearBrowsingDataPanel}
  defaultValues={this.props.appState.get('clearBrowsingDataDefaults')} />

Notice that we try to retrieve the values from the AppState (the data persisted by the AppStore). The first time you run this, it'll be undefined since the data doesn't exist. If you wanted to provide default values, you absolutely could inside of the clearBrowsingDataPanel control itself by adding a handler for React's componentDidMount and checking the bound value. Something along the lines of:

componentDidMount () {
  isClearBrowserHistoryChecked = this.props.defaultValues ? this.props.defaultValues.get('clearBrowserHistory') : true
}

And then (fun part)- if the value changes at all (you can check in the method you originally linked to, you'd want to persist this new value by creating a new action (/js/actions/appAction.js) which will take the settings and update the AppStore (js/stores/appStore.js). This logic would do a set on the appState, something like:

const defaultSettings = {
  clearBrowserHistory: action.clearBrowserHistory,
  ...
}
appState = appState.set('clearBrowsingDataDefaults')

[bsclifton]

@maaz93 I know I captured a lot of information (and you may already know large chunks of this), but I hope these posts are helpful. Please ask me any questions- I'd love to help more :smile:

(Permission-wise, I can't assign you to the issue, so I assigned the issue to myself and I'll work with you)

[maazadeeb]

@bsclifton That's so clear! 😄 I think this is sufficient to start off. I had one question though. There are 3 options in the History tab, that open up the same dialog with different options selected by default. If I were to implement this default state persistence, would I have to maintain a state for each of these options or I would do something on the lines of a logical OR between the defaultPersistedState and the expected dialogState?

For eg. If I choose Autofill data after clicking Clear History and then click Clear. When I choose the Clear Cache option next, do I need to have Autofill data and Cached images and files selected by default or just Cached images and files ?

[bsclifton]

@maaz93 good find! This does introduce an edge case

If the user picks "Clear Cache" from the menu, it disables everything except clear cache. We must leave it like this or else folks may accidentally erase other settings. In these cases (where a modal is initialized with everything disabled except for X), the state of each setting should NOT persist itself (when the modal is closed, etc)

This would be a great one to write a test case for :smile: I can help with that too, when you get further along (let me know!)

[maazadeeb]

@bsclifton I'm a little confused with this statement

In these cases (where a modal is initialized with everything disabled except for X), the state of each setting should NOT persist itself (when the modal is closed, etc)

There are 3 options in the browser History menu, Clear History, Clear Cache and Clear All Cookies and Site Data. And clicking on these results in the following pre-populated dialogs, respectively

Clear History

Clear Cache

Clear All Cookies and Site Data

In all these scenarios, some options are checked by default. In what scenario would I persist the options then?

[bsclifton]

@maaz93 interesting... so we'll have to think this through

Besides picking from the menu, you can also load this screen through preferences:

From here, it would be great to persist the settings. The menus though are almost describing what they should do. "Clear Cache" should never default to also having "Saved site settings + permissions" checked for example. Perhaps the items from the menu would be better served by having a yes/no confirm box, rather than exposing the sliders?

The version from the preferences I would feel comfortable recommending the persisting of the data. Before we do anything, I'm going to have to defer to @bradleyrichter. Brad, what do you think about this issue?

[bradleyrichter]

Sorry there is some confusion here.

First, we need to reset the menu options as described here: #3093

Then, with that single menu - I think it should be reset after each use because we have the automatic clearing options in the prefs security panel.

Does this make sense?

[bsclifton]

@maaz93 - you can start with #3093 and then we can figure out how to handle this one

@bradleyrichter with regards to this ticket, the feature request was to persist the settings that were chosen. You're proposing that the items which are enabled/disabled in that modal get reset with each use? If so, we can close this ticket as a won't fix

I can see value in saving the last selected value, in case the person is clearing something often

[maazadeeb]

@bsclifton Cool. I'll start off with #3093

Also, just my suggestion on this thread, I'm with @bsclifton and @Kanichiro on the fact that it seems like a useful feature to save the last selected values for the user, as I feel this menu is more accessible than the security panel. 😄

[bradleyrichter]

Ok. We can save the last settings since it is non-destructive until hitting the go button.

On Oct 27, 2016, at 12:26 PM, Maaz Syed Adeeb [email protected] wrote:

@bsclifton Cool. I'll start off with #3093

Also, just my suggestion on this thread, I'm with @bsclifton and @Kanichiro on the fact that it seems like a useful feature to save the last selected values for the user, as I feel this menu is more accessible than the security panel. 😄

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[maazadeeb]

@bsclifton I'll be working on this now as #3093 is closed. Will need help writing tests, once I'm done! 😃

[bsclifton]

@maaz93 how's it going? Let me know if you need help :smile:

[maazadeeb]

@bsclifton I have a working piece of code right now. But, I feel it can be simplified before submitting a PR. I feel I could benefit from another pair of eyes. Could you take a look at it if I pushed a commit to my forked repo? 😄

[bsclifton]

@maaz93 sure thing :smile: let me know your branch name and I'll check it out

[maazadeeb]

@bsclifton Here it is. The major issue that I'm facing is having two props, namely clearBrowsingDataDetail and defaultValues, that need to be selectively bound to the SwitchControl's checkedOn prop. Let me know your thoughts! 😄

[bsclifton]

@maaz93 I left you some comments on your commit https://github.com/maaz93/browser-laptop/commit/97537e3e16ee03f667326362bbec53e785145148

Please read through them and let me know if it makes sense. Basically, we should remove the existing code which is saving what was chosen to the windowState (we don't want this, since your changes persist the data to the APP state, which is shared between windows). That will resolve the problem you've found :smile:

[maazadeeb]

@bsclifton I went through the comments. I didn't think that I could delete the existing code in favor of my change. That makes so much more sense! We delete the code to store in the windowStore and persist the settings on clicking Clear directly to appStore. Thanks!

[maazadeeb]

@bsclifton I've been sitting on this bug a long time now. 🕐 When I started making some changes, I realised that I didn't get the flow of code. I spent some time reading/experimenting with React. I also understood the Flux architecture that you guys are also following. (Component -> Action -> Dispatcher -> Store -> Component).

With this understanding, I have an important question to ask. Is it absolutely necessary for every component in Brave to extend ImmutableComponent? I believe this is the reason why there is so much code to update the state of the toggle buttons in clearBrowsingDataPanel on every click, as we cannot use this.state, we resort to using this.props everywhere. I see there are a few components which don't extend ImmutableComponent.

If no, then I think I can cook up a pretty simple solution by not extending it and using this.state, else I'll have to just build on top the existing code and not replace it 😄

[bsclifton]

@maaz93 ImmutableComponent is just an optimization we introduced for components which don't need to track state. If you need to make use of state, please just inherit from React.Conponent 😄

[maazadeeb]

@bsclifton I'm finally done with my change! It's at this branch in my fork. Please have a look if you can 😄 . I need some help writing tests now, as mentioned before. 😄 I would like to know what sort of tests I've to write and also how to fix broken tests.

[bsclifton]

@maaz93 nice, great job! :smile: To get started with tests, I'd recommend getting familiar with running our webdriver tests. These are different than the unit tests because they are automating Brave running (in a specific way).

getting started

Just like when you run Brave regularly, you'll want two terminals open. One you can run npm run watch-all. In the other, you can run npm run uitest -- --grep="Clear Browsing Panel"

This should kick off the automated tests which are in place for the screen you edited (and likely you will get errors). Notice it will open a window and actually use the UI. This is a great way to get started.

Since functionality changed, some of the tests may be broken. I'd start off by trying to fix those first

writing the new test

This part is not going to be easy; but I'd recommend reading our testing docs (which have links to webdriver documentation and more). It's also helpful to look at other existing tests.

(as you read below, you may search in clearBrowsingDataPanelTest.js for the test clears site settings and permissions, which is very similar)

From a high level perspective, your plan for writing the test would be like this: make each test as if you were using the UI yourself

• test setup; add data as needed (history entries, autofill data, etc).
• write the test so that it will:
  
  
  
  • go to about:preferences#security
  
  
  • click the "Clear browser data now" button (using the CSS selector for click()), modal comes up
  
  
  • click some of the toggle switches to change the setting
  
  
  • hit clear
  
  
  • waitUntil getAppState() shows the expected data is cleared
  
  

[maazadeeb]

@bsclifton Thanks for the info. As suggested, I'll try and fix broken tests first. Then write the new ones!

[bsclifton]

@maaz93 how's it going? Any help needed? Let me know :smile:

[maazadeeb]

@bsclifton I'm almost there! I believe I've fixed the tests that I broke. I'm working on writing the new tests. I'll be adding the tests to clearBrowsingDataPanelTest.js itself. 😄

[maazadeeb]

@bsclifton Is there a way I can delete the appState from somewhere on the file system? I want to test the scenario where the user opens the clear browsing data panel for the first time and the default values are undefined.

[bsclifton]

@maaz93 sorry it's taken me so long to respond; the easiest way I can recommend would be to find your session folder (or the session file) and temporarily move it out of the way. You're on Fedora, right? I believe the session is stored here: ~/.config/brave-development/session-store-1.

Make sure all instances of Brave are closed and then try moving that file (rename as session-store-2, for example). Then you can launch Brave and it'll be like your appState is fresh :smile:

[maazadeeb]

@bsclifton No problem. I'm on a Mac. Though the folder structure is not the same, the file name session-store-1 is the same. So I was able to find it. Thanks! 😄

[Kanichiro]

I was running when I made my original post (see screenshot).

As for the Brave browser, I installed the latest version available for Linux (0.12.15). This version has an updated History, Browsing file delete dialog box.

Unfortunately, The new version takes a minor inconvenience from the version (0.12.0) I was using and turned it into a giant mess!

Originally all I had to do is turn on a few switches, click clear and I'd be done. Now I need to turn on more switches and RESTART the bloody browser in order to clear my browsing history, cookies etc. Upon restart, the switches are back in the OFF position.

I tend to clear my browsing history a lot, often times more than once per hour, and with this NEW and IMPROVED version I have to restart the browser every time I do this as well as turn on the switches.

Is there a way for me to get a copy of _Brave 0.12.0 for Linux_ to install? If not, I am done with Brave as a browser. I find this new version unacceptable. I am amazed it passed though QC.

Please get someone who knows what they are doing to fix this mess!

And again, is there a way for me to get a copy of the older version of _Brave (0.12.0) for Linux_? I liked that version a lot, and recommended it to people on the Linux Mint forums.

Linux Mint version 18 64-bit Cinnamon desktop

Dialog box as opened

Changes to the switches I made before clearing data

Do you want to Restart Now?

Diaglog box After Restart

[bsclifton]

@Kanichiro this new version hasn't been merged to master yet and definitely hasn't been released yet... are you running from source using the branch we have, 0.13.1-branch? If not, then you are not using the fix (in fact, I only accepted the change last nite). We have released updates (as you saw, 0.12.15), but the fix has not been released.

You can always download any version you'd like from the releases page:
https://github.com/brave/browser-laptop/releases

Apologies that you've been having a bad time; I promise that there is a fix coming

[bsclifton]

@Kanichiro regarding the restart, that was introduced by @diracdeltas a while back (September 2016) with https://github.com/brave/browser-laptop/commit/9f7073c21162fe6ff1ffd1d0964bb8f613806638

It's done as a work-around to clear WebRTC device IDs. I'll create an issue sharing that you feel it's inappropriate to prompt for restart. In the meantime, just choose NO and you'll get the same behavior as before

_edit:_
New issue can be found here: https://github.com/brave/browser-laptop/issues/6611

[Kanichiro]

@bsclifton Thanks, I've downloaded and installed my former version of Brave (0.12.0 for Linux).

Much, much appreciated for this link to get this older version! I think I'll keep stick with this version until you give the OK.

ps: I kept the backup copy just in case!

[bsclifton]

Fixed with https://github.com/brave/browser-laptop/pull/6026 which was merged into branch 0.13.1-branch.

Test plan

1. Launch Brave visit preferences
2. On the left, choose Security tab
3. Click Clear Browsing Data
4. Choose very specific options; like download history and all site cookies and click Clear
5. After window closes, click Clear Browsing Data again
6. The same options should be selected now. Close the window with Cancel.
7. Pick Clear Browsing Data from the History menu
8. Confirm the same options from step 4 are shown. Hit cancel to close
9. Load about:history and click Clear Browsing Data
10. Confirm the same options from step 4 are shown. Hit cancel to close