[Webkit-unassigned] [Bug 146589] [GTK] Add API to WebKitWebsiteDataManager to handle cached data

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=146589

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #257641|review?                     |review+
              Flags|                            |

--- Comment #13 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 257641
  --> https://bugs.webkit.org/attachment.cgi?id=257641
Updated patch

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

You'll need to update all the Since tags, obviously.

And it'd be nice to see implementation in Ephy before landing, since it's not great to add API that no apps are using, but I guess the MB implementation is enough to show the API is sufficient for the task.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:46
> +struct _WebKitSecurityOrigin {
> +    _WebKitSecurityOrigin(WebCore::SecurityOrigin* coreSecurityOrigin)
> +        : securityOrigin(coreSecurityOrigin)
> +    {
> +    }

Why is it useful to create WebKitSecurityOrigin objects with a null WebCore::SecurityOrigin? I would have expected the constructor to receive a reference, and to use a Ref rather than RefPtr member variable.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:79
> +WebCore::SecurityOrigin& webkitSecurityOriginGetSecurityOrigin(WebKitSecurityOrigin* origin)
> +{
> +    ASSERT(origin);
> +    return *origin->securityOrigin;
> +}

This ASSERT indicates to me that allowing null WebCore::SecurityOrigins is probably not appropriate.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:652
> + * Note that this is not supported for #WebKitWebContext<!-- -->s using
> + * %WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS process model; an empty list will
> + * always be returned in such case.

Were we still using the soup cache in shared secondary process mode when you wrote this patch? Hopefully this comment is obsolete and should be removed?

> Tools/MiniBrowser/gtk/main.c:396
> +    WebKitWebsiteDataManager* manager = webkit_web_context_get_website_data_manager(webkit_web_context_get_default());

WebKitWebsiteDataManager *manager

> Tools/MiniBrowser/gtk/main.c:424
> +    AboutDataRequest *dataRequest = aboutDataRequestNew(request);
> +    WebKitWebsiteDataManager* manager = webkit_web_context_get_website_data_manager(webkit_web_context_get_default());

WebKitWebsiteDataManager *manager

> Tools/MiniBrowser/gtk/main.c:493
> +    WebKitUserContentManager *userContentManager = webkit_user_content_manager_new();

Looks like it's leaked?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:736
> +    // Disk cache delays the storing of initial resources for 1 second to avoid
> +    // affecting early page load. So, wait 1 second here to make sure resources
> +    // have already been stored.
> +    test->wait(1);

This is unfortunate.

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:42
> +WebViewTest::WebViewTest(WebKitProcessModel processModel)
> +    : Test(processModel)
> +    , m_webView(WEBKIT_WEB_VIEW(g_object_ref_sink(webkit_web_view_new_with_context(m_webContext.get()))))
> +    , m_mainLoop(g_main_loop_new(nullptr, TRUE))
>  {
>      assertObjectIsDeletedWhenTestFinishes(G_OBJECT(m_webView));
>      g_signal_connect(m_webView, "web-process-crashed", G_CALLBACK(WebViewTest::webProcessCrashed), this);

This is not good. Use delegating constructors to avoid the need to reimplement the constructor body. Make sure there is a constructor that can take both a WebKitUserContentManager and a WebKitProcessModel.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160917/829a2cc7/attachment.html>