Bug 217034 – [GTK] Stop using the default website data store for the inspector

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

GTK port should never use the default data store.

[Carlos Garcia Campos]

Created attachment 409829 [details]
Patch

[Michael Catanzaro]

Comment on attachment 409829 [details]
Patch

Questions:

 *Each inspector is 1:1 associated with a web view. Why doesn't the inspector use that view's data store?
 * Why is it better to create a custom data store using default paths than to just use the default data store?
 * Why does GTK need this but not WPE?
 * Don't applications use the default data store *by default*? What's wrong with this?
 * If GTK port should never use the default data store, shouldn't we have an ASSERT to ensure it doesn't get created?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 409829 [details]
> Patch
> 
> Questions:
> 
>  *Each inspector is 1:1 associated with a web view. Why doesn't the
> inspector use that view's data store?

The inspector doesn't use the same process pool as the web view, so it uses its own network process. If the web view is ephemeral we still want to save inspector settings. We have always saved the inspector settings globally, and not per application.

>  * Why is it better to create a custom data store using default paths than
> to just use the default data store?

Using the default data store is problematic for GLib API, we end up writing stuff globally, instead of per application, which is not desired in most of the cases. We can't probably change the default locations of website data store, for backwards compatibility reasons, note that these paths are versioned too, not the same as the default ones. After this patch users will lose the inspector settings.

>  * Why does GTK need this but not WPE?

WPE doesn't have WebInspectorProxy, because only the remote inspector is supported in WPE.

>  * Don't applications use the default data store *by default*? What's wrong
> with this?

No, applications use a WebKitWebsiteDataManager that creates a website data store with the default session ID, which is different. The GLib API doesn't use the default data store, but there are places like the inspector where it's created internally.

>  * If GTK port should never use the default data store, shouldn't we have an
> ASSERT to ensure it doesn't get created?

That's the plan, but I need to fix the cases where it's used first. I caught this thanks to the assert I added in patch atached to bug #216041

[Michael Catanzaro]

OK, makes sense, especially for saving ephemeral settings. Except:

(In reply to Carlos Garcia Campos from comment #3)
> We have always saved the inspector settings globally, and not per application.

Problem is this makes it hard to audit that Epiphany/WebKit isn't leaking data into the default data dirs. Currently, as long as you don't open yelp, ~/.var/app/org.gnome.Epiphany/cache/webkitgtk should never exist. But with this change, you have to never open yelp *or* the web inspector, which is harder. ;) What do you think about using a different global location, like "webkitgtk-inspector" instead of the default location?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> OK, makes sense, especially for saving ephemeral settings. Except:
> 
> (In reply to Carlos Garcia Campos from comment #3)
> > We have always saved the inspector settings globally, and not per application.
> 
> Problem is this makes it hard to audit that Epiphany/WebKit isn't leaking
> data into the default data dirs. Currently, as long as you don't open yelp,
> ~/.var/app/org.gnome.Epiphany/cache/webkitgtk should never exist. But with
> this change, you have to never open yelp *or* the web inspector, which is
> harder. ;) What do you think about using a different global location, like
> "webkitgtk-inspector" instead of the default location?

It's not the default one, this one is versioned, so the directory is not webkitgtk, but webkitgtk-4.0.

[Michael Catanzaro]

Versioning is good. I approve of versioning. But having webkitgtk-4.0 create both a versioned and an unversioned directory seems pretty confusing, right? One would be led to believe that the webkitgtk directory is used by older versions of WebKit prior to the 4.0 API.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> Versioning is good. I approve of versioning. But having webkitgtk-4.0 create
> both a versioned and an unversioned directory seems pretty confusing, right?
> One would be led to believe that the webkitgtk directory is used by older
> versions of WebKit prior to the 4.0 API.

The unversioned directory shouldn't be used anymore, it was a mistake. For next API bump I plan to use default base cache and data dirs to ensure there isn't global directory by default.

[EWS]

Committed r267729: <https://trac.webkit.org/changeset/267729>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409829 [details].

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> The unversioned directory shouldn't be used anymore, it was a mistake. For
> next API bump I plan to use default base cache and data dirs to ensure there
> isn't global directory by default.

Good idea. Even better if default dirs are based on prgname. It would be a good idea to change this now under #if USE(GTK4) conditions.