[webkit-reviews] review denied: [Bug 24758] [Gtk] Implement a WebDataSource for the gtk port : [Attachment 34920] updated patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Jan Alonzo <jmalonzo at gmail.com>'s
request for review:
Bug 24758: [Gtk] Implement a WebDataSource for the gtk port
https://bugs.webkit.org/show_bug.cgi?id=24758

Attachment 34920: updated patch
https://bugs.webkit.org/attachment.cgi?id=34920&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> +#include <glib.h>
> +
> +/**
> + * SECTION:webkitwebdatasource
> + * @short_description: Encapsulates the content to be displayed in a
#WebKitWebFrame.
> + * @see_also: #WebKitWebFrame
> + *
> + * A data source encapsulates the content of a #WebKitWebFrame. A
> + * #WebKitWebFrame has a main resource and subresources and the data source
> + * provides methods to access these resources. #WebKitWebDataSource objects
> + * have an associated initial request. Make sure to check if the data source

> + * is in the process of being loaded before accessing its data.
> + */

It's not totally clear to me what's the difference between the initial request
and the plain request that you can also get from the datasource. It would be
good to expand on this either here or in the docs for each function IMHO.

> +#define WEBKIT_WEB_DATA_SOURCE_GET_PRIVATE(obj)	 
(G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_WEB_DATA_SOURCE,
WebKitWebDataSourcePrivate))
> +
> +G_DEFINE_TYPE(WebKitWebDataSource, webkit_web_data_source, G_TYPE_OBJECT);
> +
> +static void webkit_web_data_source_dispose(GObject* object)
> +{
> +    WebKitWebDataSource* webDataSource = WEBKIT_WEB_DATA_SOURCE(object);
> +    WebKitWebDataSourcePrivate* priv = webDataSource->priv;
> +
> +    ASSERT(priv->loader);
> +    ASSERT(!priv->loader->isLoading());

Just wondering, why can't a datasource be destroyed while the loader is
loading?

> +
> +    while (priv->subresources) {
> +	   WebKitWebResource* res =
WEBKIT_WEB_RESOURCE(priv->subresources->data);
> +	   ASSERT(res);
> +	   g_object_unref(res);
> +	   priv->subresources = priv->subresources->next;
> +    }
> +
> +    if (priv->subresources) {
> +	   g_list_free(priv->subresources);
> +	   priv->subresources = NULL;
> +    }

It's impossible that this will be executed given how the previous while is
constructed. Just iterate through the list normally and do a g_list_free at the
end.

> +
> +/**
> + * webkit_web_data_source_new_with_request:
> + * @request: the #WebKitNetworkRequest to use to create this data source
> + *
> + * Creates a new #WebKitWebDataSource from a #WebKitNetworkRequest.
Normally,
> + * #WebKitWebFrame objects create their data sources so you will almost
never
> + * want to invoke this method directly.
> + *
> + * Returns: a new #WebKitWebDataSource
> + *
> + * Since: 1.1.13
> + */
> +WebKitWebDataSource*
webkit_web_data_source_new_with_request(WebKitNetworkRequest* request)
> +{
> +    ASSERT(request);

I'd just use g_return_val_if_fail even if this method is pseudo-private.

> +
> +    const gchar* uri = webkit_network_request_get_uri(request);
> +
> +    WebKitWebDataSource* datasource;
> +    datasource = webkit_web_data_source_new_with_loader(
> +	   WebKit::DocumentLoader::create(ResourceRequest(KURL(KURL(),
String::fromUTF8(uri))),
> +					  SubstituteData()));
> +
> +    WebKitWebDataSourcePrivate* priv = datasource->priv;
> +    priv->initialRequest = request;

Mention in the doc that the function steals the ref to the request?

> +}
> +
> +/**
> + * webkit_web_data_source_get_network_request:

The new name is get_request, not get_network_request

> + * @data_source: a #WebKitWebDataSource
> + *
> + * Returns a #WebKitNetworkRequest that was used to create this
#WebKitWebDataSource
> + *
> + * Return value: the #WebKitNetworkRequest that created the @data_source
> + *
> + * Since 1.1.12
> + */
> +WebKitNetworkRequest*
webkit_web_data_source_get_request(WebKitWebDataSource* webDataSource)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(webDataSource), NULL);
> +
> +    WebKitWebDataSourcePrivate* priv = webDataSource->priv;
> +    FrameLoader* frameLoader = priv->loader->frameLoader();
> +    if (!frameLoader || !frameLoader->frameHasLoaded())
> +	   return NULL;

We don't return the request if the frame is not loaded because it won't be
there or because we shouldn't? I guess this links with my doubts about what's
the difference between initial request and this.

> +
> +    ResourceRequest request = priv->loader->request();
> +
> +	if (!priv->networkRequest)
> +	    priv->networkRequest =
webkit_network_request_new_with_core_request(request);

Indentation seems to be a bit off here.

> +
> +    return priv->networkRequest;
> +}
> +
> +/**
> + * webkit_web_data_source_get_encoding:
> + * @data_source: a #WebKitWebDataSource
> + *
> + * Returns the text encoding name as set in the #WebKitWebView, or if not,
the
> + * text encoding of the response.
> + *
> + * Return value: the encoding name of the #WebKitWebView or of the response
> + *
> + * Since: 1.1.13
> + */
> +G_CONST_RETURN gchar*
webkit_web_data_source_get_encoding(WebKitWebDataSource* webDataSource)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(webDataSource), NULL);
> +
> +    WebKitWebDataSourcePrivate* priv = webDataSource->priv;
> +    String textEncodingName = priv->loader->overrideEncoding();
> +
> +    if (!textEncodingName)
> +	       textEncodingName = priv->loader->response().textEncodingName();

indentation is wrong.

> +
> +    CString encoding = textEncodingName.utf8();
> +    if (!priv->textEncoding || priv->textEncoding != encoding.data())
> +	   priv->textEncoding = g_strdup(encoding.data());
> +
> +    return priv->textEncoding;
> +}
> +

> +/**
> + * webkit_web_data_source_get_data:
> + * @data_source: a #WebKitWebDataSource
> + *
> + * Returns the raw data that represents the the frame's content.The data
will
> + * be incomplete until the data has finished loading. Returns %NULL if the
web
> + * frame hasn't loaded any data. Use webkit_web_data_source_is_loading to
test
> + * if data source is in the process of loading.
> + *
> + * Return value: a #GString which contains the raw data that represents the
@data_source or %NULL if the
> + * @data_source hasn't loaded any data.

Should mention the data is owned by the data source, because returning a const
GString in a C API does not make much sense.

> + *
> + * Since: 1.1.13
> + */
> +G_CONST_RETURN GString* webkit_web_data_source_get_data(WebKitWebDataSource*
webDataSource)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(webDataSource), NULL);
> +
> +    WebKitWebDataSourcePrivate* priv = webDataSource->priv;
> +
> +    RefPtr<SharedBuffer> mainResourceData =
priv->loader->mainResourceData();
> +
> +    if (!mainResourceData)
> +	   return NULL;
> +
> +    if (!priv->data)
> +	   priv->data = g_string_new_len(mainResourceData->data(),
mainResourceData->size());
> +
> +    return priv->data;
> +}
> +

> +}
> +
> +/**
> + * webkit_web_data_source_get_subresources:
> + * @data_source: a #WebKitWebDataSource
> + *
> + * Retrieves the subresources of this @datasource.
> + *
> + * Return value: a #GList of #WebKitWebResource. The #GList if owned by
WebKit
> + * and should not be freed or destroyed.
> + *
> + * Since: 1.1.13
> + */
> +GList* webkit_web_data_source_get_subresources(WebKitWebDataSource*
webDataSource)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(webDataSource), NULL);
> +
> +    WebKitWebDataSourcePrivate* priv = webDataSource->priv;
> +
> +    Vector<PassRefPtr<ArchiveResource> > coreSubresources;

Just wondering: is that space needed by C++? :)

> +    priv->loader->getSubresources(coreSubresources);
> +
> +    // A resource may have added so we need to check if the sizes are not
equal.
> +    if (!priv->subresources || coreSubresources.size() !=
g_list_length(priv->subresources)) {

Mmm, couldn't the size be the same but the contents different? (dynamic
modification of the page?)

> +	   if (priv->subresources) {
> +	       // If sizes are not equal, refresh our internal copy but free
the resources first.
> +	       for ( ; priv->subresources; priv->subresources = 
priv->subresources->next) {
> +		   g_object_unref(priv->subresources->data);
> +	       }
> +	       g_list_free(priv->subresources);
> +	       priv->subresources = NULL;
> +	   }
> +
> +	   for (unsigned i = 0; i < coreSubresources.size(); i++) {
> +	       WebKitWebResource* resource =
webkit_web_resource_new_with_core_resource(coreSubresources.at(i));
> +	       ASSERT(resource);
> +	       priv->subresources = g_list_prepend(priv->subresources,
resource);
> +	   }
> +    }
> +
> +    return priv->subresources;
> +}
> +

> +/**
> + * webkit_web_data_source_get_resource_for_url:

called get_subresource now. Also as mentioned on IRC I think we call these URI.


> + * @data_source: a #WebKitWebDataSource
> + * @url: the url of a resource
> + *
> + * Returns the #WebKitWebResource for the given URL
> + *
> + * Return value: the #WebKitWebResource for @url or %NULL if there is no
> + * #WebKitWebResource associated with the @url.
> + *
> + * Since: 1.1.13
> + */
> +WebKitWebResource*
webkit_web_data_source_get_subresource_for_url(WebKitWebDataSource*
webDataSource, const gchar* url)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(webDataSource), NULL);

Also check the URL?

> +
> +    // Make sure we have the subresources internally.
> +    GList* subresources =
webkit_web_data_source_get_subresources(webDataSource);
> +
> +    GList* element = g_list_find_custom(subresources, url,
> +					  
(GCompareFunc)webkit_web_data_source_find_resource_func);
> +    return WEBKIT_WEB_RESOURCE(element->data);
> +}
> +

> +
> +/**
> + * webkit_web_data_source_get_unreachable_url:

Same thing about URI/URL.

Marking as r- for now, but I think we are getting there :)



>