[webkit-reviews] review denied: [Bug 67931] [GTK] Add WebKitWebContext to GTK API : [Attachment 107042] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 67931: [GTK] Add WebKitWebContext to GTK API
https://bugs.webkit.org/show_bug.cgi?id=67931

Attachment 107042: Patch
https://bugs.webkit.org/attachment.cgi?id=107042&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107042&action=review


Looks good, but I have a few nits and a small request to fix some of the FIXMEs
now that we are touching this code. I feel that if we don't fix them now,
they'll never get fixed.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:96
> + * WEBKIT_CACHE_MODEL_WEB_BROWSER.

Default value -> The default value

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:134
> +WebKitCacheModel
> +webkit_web_context_get_cache_model(WebKitWebContext* context)

Extra newline here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:87
> +WK_EXPORT GType
> +webkit_web_context_get_type	      (void);
> +
> +WK_EXPORT WebKitWebContext *
> +webkit_web_context_get_default     (void);
> +
> +WK_EXPORT void
> +webkit_web_context_set_cache_model (WebKitWebContext *context,
> +				       WebKitCacheModel  cache_model);
> +WK_EXPORT WebKitCacheModel
> +webkit_web_context_get_cache_model (WebKitWebContext *context);
> +

Do you think it makes sense to stop lining the arguments up between functions?
It's really annoyng when you finally add a longer function and have to realign
the entire file.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:48
> +    // FIXME: Add disk cache handling when soup has the API
> +    unsigned long urlCacheMemoryCapacity = 0;
> +    unsigned long urlCacheDiskCapacity = 0;
> +    uint64_t diskFreeSize = 0;

Soup has the API now, but I'm not sure if it's public yet.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:52
> +    // FIXME: The Mac port calculates these values based on the amount of
physical memory that's
> +    // installed on the system. Currently these values match the Mac port
for users with more than
> +    // 512 MB and less than 1024 MB of physical memory.

Is it possible to do this now, at least for Linux?