GNOME Bugzilla – Bug 741447
Clear personal data should allow clearing all stored data
Last modified: 2017-02-01 06:25:23 UTC
HTML5 local storage is left on disk after clearing temporary files. A couple thoughts: * It's possible that we might want to have separate UI for local storage, on the grounds that web sites may not consider local storage to be temporary. (The disadvantage would be possibly unnecessary complexity.) * We might want this functionality WebKitWebContext instead.
Created attachment 292616 [details] [review] Clear temporary files should clear HTML5 local storage Clear temporary files currently clears the cache and favicon database, but leaves all local storage. We should probably delete that too.
So this depends on bug #737808, that has a patch not ready to land. We should try to fix these bugs before the freeze.
We have more problems in this area than just localstorage. In particular, I'm trying to figure out why I have ~/.local/share/webkit/databases (instead of ~/.local/share/webkitgtk/databases) and ~/.local/share/webkit/icondatabase (in addition to ~/.cache/epiphany/icondatabase). I guess the values we specify in WebProcessPoolGtk.cpp are not all honored. We might need to reconsider the API added in https://bugs.webkit.org/show_bug.cgi?id=138828. I was thinking that a website-data-directory property, specifying a directory under which directories for localstorage and databases (and eventually mediakeys) would be created, would work better.
(In reply to comment #3) > We have more problems in this area than just localstorage. In particular, I'm > trying to figure out why I have ~/.local/share/webkit/databases (instead of > ~/.local/share/webkitgtk/databases) and ~/.local/share/webkit/icondatabase (in > addition to ~/.cache/epiphany/icondatabase). I guess the values we specify in > WebProcessPoolGtk.cpp are not all honored. Because that have changed several times and very inconsistenly. > We might need to reconsider the API added in > https://bugs.webkit.org/show_bug.cgi?id=138828. I was thinking that a > website-data-directory property, specifying a directory under which directories > for localstorage and databases (and eventually mediakeys) would be created, > would work better. I think it's important to have a separate directory for every feature, you can set all to the same dir if you want. In the case of favicon database, if you don't set a directory, the feature is not enabled. In other cases like IndexedDB, subdirectories are created with different schemas, so I would rather keep every kind of database in its own directory.
Created attachment 344058 [details] [review] Rework personal data dialog using new WebKit API This uses the new WebKit API, we need a new release to depend on. I'm sure the UI needs some tweaks, but the functionality if there.
Review of attachment 344058 [details] [review]: Really nice, it looks great! I prefer you wait until the next WebKit release to commit it. One bad thing I notice is the type to search doesn't work, the input gets captured by the tree view instead of by your search box. So please fix that; anywhere the user can see a search button, typing a normal character should activate the button and begin a search. Another bad thing is that I still have a bunch of application cache files left over (including a 10 MB one) after deleting all application cache from the beginning of time. But I guess that's what you fixed in https://trac.webkit.org/changeset/211035, right? The biggest problem is that you can only clear data for particular sites if selecting the beginning of time option. I don't care about the restriction, but I do about how it's presented to users: there's no way to know that you need to select "from the beginning of time" in order to clear data for particular sites. So we need something better here. One easy solution would be to change the default state of the combo box to "from the beginning of time". I'd make one incidental change to the prefs dialog itself: change the "Clear personal data..." button to "Manager personal data..." so that users don't fear that pressing the button is destructive. If you want bonus points, store in GSettings which of the top-level checkboxes are checked: bug #770755. ::: src/clear-data-dialog.c @@ +147,3 @@ + + treestore = GTK_TREE_STORE (dialog->treestore); + for (guint i = 0; i < G_N_ELEMENTS(data_entries); i++) { G_N_ELEMENTS (data_entries) @@ +178,3 @@ + } + + g_list_free_full (data_list, (GDestroyNotify)webkit_website_data_unref); Is the cast really needed? @@ -127,2 +387,9 @@ - gtk_widget_set_sensitive (dialog->clear_button, - dialog->num_checked != 0); + return visible; +} + ... 6 more ... You have to check if it's NULL first or it will crash if there's a reference cycle. (Which means there's not much value in using g_clear_object here either.) @@ +407,3 @@ + object_class->dispose = clear_data_dialog_dispose; + + g_type_ensure (WEBKIT_TYPE_WEBSITE_DATA); Why is this required? I see you don't need it in class_init itself, not when setting properties. You're getting a hang without it? ::: src/resources/gtk/clear-data-dialog.ui @@ +130,3 @@ + <item translatable="yes">the past day</item> + <item translatable="yes">the past week</item> + <item translatable="yes">the past 4 weeks</item> "4" -> "four"
(In reply to Michael Catanzaro from comment #6) > You have to check if it's NULL first or it will crash if there's a reference > cycle. > > (Which means there's not much value in using g_clear_object here either.) Er, something went wrong in splinter. The problem is you call g_cancellable_cancel() in dispose without null-checking the cancellable as required.
(In reply to Michael Catanzaro from comment #6) > Review of attachment 344058 [details] [review] [review]: > > Really nice, it looks great! I prefer you wait until the next WebKit release > to commit it. Sure, I'll wait until we have a new version to depend on. > One bad thing I notice is the type to search doesn't work, the input gets > captured by the tree view instead of by your search box. So please fix that; > anywhere the user can see a search button, typing a normal character should > activate the button and begin a search. Ah, I didn't know that, I'll check how it's done in other dialogs. > Another bad thing is that I still have a bunch of application cache files > left over (including a 10 MB one) after deleting all application cache from > the beginning of time. But I guess that's what you fixed in > https://trac.webkit.org/changeset/211035, right? No, that fix is only for WebSQL databases, what do you mean exactly by application cache files? Offline web application cache? HTTP disk cache? We have unit tests in WebKit to check that data is properly removed. > The biggest problem is that you can only clear data for particular sites if > selecting the beginning of time option. I don't care about the restriction, > but I do about how it's presented to users: there's no way to know that you > need to select "from the beginning of time" in order to clear data for > particular sites. So we need something better here. One easy solution would > be to change the default state of the combo box to "from the beginning of > time". Yes, that's how the WebKit2 API works, you can remove data from sites, or clear data for a period of time, but not both. Making the rows insensitive is not clear enough. I thought about making from the beginning of time the default, but then you would be confused when switching to another option anyway, and using last hour is consistent with other browsers. I think we should just explain it in the dialog. > I'd make one incidental change to the prefs dialog itself: change the "Clear > personal data..." button to "Manager personal data..." so that users don't > fear that pressing the button is destructive. Good idea, it's also consistent with cookies and passwords too. > If you want bonus points, store in GSettings which of the top-level > checkboxes are checked: bug #770755. I'm not sure I agree with that bug, though. > ::: src/clear-data-dialog.c > @@ +147,3 @@ > + > + treestore = GTK_TREE_STORE (dialog->treestore); > + for (guint i = 0; i < G_N_ELEMENTS(data_entries); i++) { > > G_N_ELEMENTS (data_entries) > > @@ +178,3 @@ > + } > + > + g_list_free_full (data_list, (GDestroyNotify)webkit_website_data_unref); > > Is the cast really needed? I think so, but I'll check. > @@ -127,2 +387,9 @@ > - gtk_widget_set_sensitive (dialog->clear_button, > - dialog->num_checked != 0); > + return visible; > +} > + > ... 6 more ... > > You have to check if it's NULL first or it will crash if there's a reference > cycle. > > (Which means there's not much value in using g_clear_object here either.) Nope, g_cancellable_cancel is null safe. > @@ +407,3 @@ > + object_class->dispose = clear_data_dialog_dispose; > + > + g_type_ensure (WEBKIT_TYPE_WEBSITE_DATA); > > Why is this required? I see you don't need it in class_init itself, not when > setting properties. You're getting a hang without it? I'm using WebKitWebsiteData in the ui file, we just need to ensure the type is already registered when gtkbuilder builds the GtkListStore. Static glib types are never unregistered, so we only need to ensure it once, and class init is a good place for that. > ::: src/resources/gtk/clear-data-dialog.ui > @@ +130,3 @@ > + <item translatable="yes">the past day</item> > + <item translatable="yes">the past week</item> > + <item translatable="yes">the past 4 weeks</item> > > "4" -> "four" Copied it from chromium :-) But I agree four looks better.
One more thing: can you add some indication that the operation either succeeded or failed? Could be as simple as a GtkMessageDialog with message "Success!" and a button that says "Yay!" in the case of success. Your API has a GError parameter so it should be possible. *The clear data dialog should be non-interactive during this time.*
(In reply to Carlos Garcia Campos from comment #8) > > Another bad thing is that I still have a bunch of application cache files > > left over (including a 10 MB one) after deleting all application cache from > > the beginning of time. But I guess that's what you fixed in > > https://trac.webkit.org/changeset/211035, right? > > No, that fix is only for WebSQL databases, what do you mean exactly by > application cache files? Offline web application cache? HTTP disk cache? We > have unit tests in WebKit to check that data is properly removed. Offline web application cache. I have in ~/.cache/epiphany/applications a 57.3 kB ApplicationCache.db, a 32.8 ApplicationCache.db-shm, and an 10.4 MB ApplicationCache.db-wal, despite having just deleted all of my offline web application cache since the beginning of time using your dialog. > Yes, that's how the WebKit2 API works, you can remove data from sites, or > clear data for a period of time, but not both. Making the rows insensitive > is not clear enough. I thought about making from the beginning of time the > default, but then you would be confused when switching to another option > anyway, and using last hour is consistent with other browsers. I think we > should just explain it in the dialog. OK, that works too. > > If you want bonus points, store in GSettings which of the top-level > > checkboxes are checked: bug #770755. > > I'm not sure I agree with that bug, though. OK, then please close it. ;)
(In reply to Michael Catanzaro from comment #9) > One more thing: can you add some indication that the operation either > succeeded or failed? Could be as simple as a GtkMessageDialog with message > "Success!" and a button that says "Yay!" in the case of success. Your API > has a GError parameter so it should be possible. *The clear data dialog > should be non-interactive during this time.* I don't understand, there's a spinner while getting the data, then the treeview is shown. In case of no data, a message saying there's no data is shown. The API has a GError and returns gboolean but it never fails. I used that approach for consistency with the glib async pattern or in case it fails in the future. But at the moment it can only fail if the cancellable is cancelled, which only happens on dispose.
(In reply to Michael Catanzaro from comment #10) > (In reply to Carlos Garcia Campos from comment #8) > > > Another bad thing is that I still have a bunch of application cache files > > > left over (including a 10 MB one) after deleting all application cache from > > > the beginning of time. But I guess that's what you fixed in > > > https://trac.webkit.org/changeset/211035, right? > > > > No, that fix is only for WebSQL databases, what do you mean exactly by > > application cache files? Offline web application cache? HTTP disk cache? We > > have unit tests in WebKit to check that data is properly removed. > > Offline web application cache. I have in ~/.cache/epiphany/applications a > 57.3 kB ApplicationCache.db, a 32.8 ApplicationCache.db-shm, and an 10.4 MB > ApplicationCache.db-wal, despite having just deleted all of my offline web > application cache since the beginning of time using your dialog. And is that shown in the list of data fetched? If we fetch that and fail to clear, then file a bug report to WebKit, otherwise it's probably leaked. > > Yes, that's how the WebKit2 API works, you can remove data from sites, or > > clear data for a period of time, but not both. Making the rows insensitive > > is not clear enough. I thought about making from the beginning of time the > > default, but then you would be confused when switching to another option > > anyway, and using last hour is consistent with other browsers. I think we > > should just explain it in the dialog. > > OK, that works too. > > > > If you want bonus points, store in GSettings which of the top-level > > > checkboxes are checked: bug #770755. > > > > I'm not sure I agree with that bug, though. > > OK, then please close it. ;) Well, I said I'm not sure.
(In reply to Carlos Garcia Campos from comment #11) > I don't understand, there's a spinner while getting the data, then the > treeview is shown. In case of no data, a message saying there's no data is > shown. The API has a GError and returns gboolean but it never fails. I used > that approach for consistency with the glib async pattern or in case it > fails in the future. But at the moment it can only fail if the cancellable > is cancelled, which only happens on dispose. I mean the clear operation. Currently when you press clear the dialog disappears immediately, and the user doesn't know if it worked or not. (In reply to Carlos Garcia Campos from comment #12) > And is that shown in the list of data fetched? If we fetch that and fail to > clear, then file a bug report to WebKit, otherwise it's probably leaked. It was shown before I clicked Clear. Once I clicked Clear, it's no longer shown. So yes, it's been leaked, but it was not leaked until I performed the Clear operation.
(In reply to Michael Catanzaro from comment #13) > It was shown before I clicked Clear. Once I clicked Clear, it's no longer > shown. So yes, it's been leaked, but it was not leaked until I performed the > Clear operation. To be clear: it disappeared immediately, and did not reappear after restarting Epiphany, so WebKit really cannot see it anymore, even though it's still there on disk.
(In reply to Michael Catanzaro from comment #13) > (In reply to Carlos Garcia Campos from comment #11) > > I don't understand, there's a spinner while getting the data, then the > > treeview is shown. In case of no data, a message saying there's no data is > > shown. The API has a GError and returns gboolean but it never fails. I used > > that approach for consistency with the glib async pattern or in case it > > fails in the future. But at the moment it can only fail if the cancellable > > is cancelled, which only happens on dispose. > > I mean the clear operation. Currently when you press clear the dialog > disappears immediately, and the user doesn't know if it worked or not. Ah, it never fails, so I don't want to delay the dialog destruction for nothing. > (In reply to Carlos Garcia Campos from comment #12) > > And is that shown in the list of data fetched? If we fetch that and fail to > > clear, then file a bug report to WebKit, otherwise it's probably leaked. > > It was shown before I clicked Clear. Once I clicked Clear, it's no longer > shown. So yes, it's been leaked, but it was not leaked until I performed the > Clear operation. That's a WebKit bug then. We have a unit test to check exactly that, though.
(In reply to Carlos Garcia Campos from comment #15) > Ah, it never fails, so I don't want to delay the dialog destruction for > nothing. Then the GError parameters are just ignored? > > (In reply to Carlos Garcia Campos from comment #12) > > > And is that shown in the list of data fetched? If we fetch that and fail to > > > clear, then file a bug report to WebKit, otherwise it's probably leaked. > > > > It was shown before I clicked Clear. Once I clicked Clear, it's no longer > > shown. So yes, it's been leaked, but it was not leaked until I performed the > > Clear operation. > > That's a WebKit bug then. We have a unit test to check exactly that, though.
(In reply to Carlos Garcia Campos from comment #15) > Ah, it never fails, so I don't want to delay the dialog destruction for > nothing. I think it should be delayed until the function returns anyway. What you can do then is display a GtkMessageDialog that says the operation succeeded, but still add code to Epiphany to display an error dialog as well in case WebKit ever decides to set the error parameter in the future. We have to do something to indicate the operation succeeded; if the dialog just disappears when you click clear, users get suspicious.
(In reply to Michael Catanzaro from comment #17) > (In reply to Carlos Garcia Campos from comment #15) > > Ah, it never fails, so I don't want to delay the dialog destruction for > > nothing. > > I think it should be delayed until the function returns anyway. What you can > do then is display a GtkMessageDialog that says the operation succeeded, but > still add code to Epiphany to display an error dialog as well in case WebKit > ever decides to set the error parameter in the future. We have to do > something to indicate the operation succeeded; if the dialog just disappears > when you click clear, users get suspicious. That's what both chromium and firefox do, and I'm sure nobody suspects anything, you just assumes it worked. I bet it's also what our current code does. Not closing the dialog might look like it hung, or showing a spinner just to close the dialog is not ideal either. Showing a stupid popup just to say that data was deleted is annoying, you are supposed to delete the data. So, no, I'm not going to do that.
(In reply to Michael Catanzaro from comment #16) > (In reply to Carlos Garcia Campos from comment #15) > > Ah, it never fails, so I don't want to delay the dialog destruction for > > nothing. > > Then the GError parameters are just ignored? > Yes.