Bug 147288 – [GTK] Enable persistent cookie storage in MiniBrowser.

• Status: NEW
• Component: WebKitGTK

[Hyungwook Lee]

I think it will be good if we support persistent cookie storage in MiniBrowser.

[Hyungwook Lee]

Created attachment 257507 [details]
Patch

[Michael Catanzaro]

Comment on attachment 257507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257507&action=review

Hi Hyungwook, why do you want this? It seems useful to me that when MiniBrowser is restarted, there are no cookies that could be affecting the results of whatever it is I'm testing. Maybe it's not likely to often cause problems in practice, though, so I'm not opposed to this change.

> Tools/MiniBrowser/gtk/main.c:298
> +    // Enable persistent cookie storage.

Please remove this comment, it's a "what" comment and doesn't add any value. We prefer comments that explain "why."

[Hyungwook Lee]

I think persistent cookie function is default function of most browser support it.

To prevent confusion of using cookie, we'd better to meet another browser's behavior even if it is mini-browser.

[Hyungwook Lee]

Created attachment 268747 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 268747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268747&action=review

MiniBrowser is not a general purpose browser, it's for testing, so I'm not sure we want to save cookies. I agree it could be useful for testing cookies, though. So, maybe we could add a command line option to pass the cookies storage path, or the other way around, always save cookies and add a command line option to not use persistent cookies.

> Tools/MiniBrowser/gtk/main.c:299
> +    webkit_cookie_manager_set_persistent_storage(cookieManager, g_build_filename(g_get_user_data_dir(), "webkitgtk", "cookies.db", NULL), WEBKIT_COOKIE_PERSISTENT_STORAGE_SQLITE);

This is leaking the value returned by g_build_filename(), it returns a new allocated string that should be freed. Also, I don't think we should use a generic webkitgtk dir, but someting more specific like MiniBrowser.