[webkit-reviews] review denied: [Bug 24758] [Gtk] Implement a WebDataSource for the gtk port : [Attachment 33015] updated datasource 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 33015: updated datasource patch
https://bugs.webkit.org/attachment.cgi?id=33015&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> +void DocumentLoader::detachDataSource()
> +{
> +    ASSERT(!m_isDataSourceReffed);
> +    unrefDataSource();
> +}

Mmm, I don't think this ASSERT can be right, since unrefDataSource won't do
anything if !m_isDataSourceReffed.

> +
> +void DocumentLoader::detachFromFrame()
> +{
> +    WebCore::DocumentLoader::detachFromFrame();
> +
> +    if (m_loadingResources.isEmpty())
> +	   unrefDataSource();
> +
> +}

So this will silently not do stuff if there are resources still being loaded. I
suppose that's OK?

> +
> +void DocumentLoader::decreaseLoadCount(unsigned long identifier)
> +{
> +    HashSet<unsigned long>::iterator it =
m_loadingResources.find(identifier);
> +
> +    // It is valid for a load to be cancelled before it's started.
> +    if (it == m_loadingResources.end())
> +	   return;
> +
> +    m_loadingResources.remove(it);
> +
> +    if (m_loadingResources.isEmpty() && !frame())
> +	   unrefDataSource();
> +
> +}

Mmm, this seems to not do one unref per call, as opposed to the
increaseLoadCount function which does that (for different resources). Is this
done elsewhere?


>  WTF::PassRefPtr<WebCore::DocumentLoader>
FrameLoaderClient::createDocumentLoader(const WebCore::ResourceRequest&
request, const SubstituteData& substituteData)
>  {
> -    return DocumentLoader::create(request, substituteData);
> +    RefPtr<WebKit::DocumentLoader> loader =
WebKit::DocumentLoader::create(request, substituteData);
> +
> +    WebKitWebDataSource* webDataSource =
webkit_web_data_source_new_with_loader(loader.get());
> +    loader->setDataSource(webDataSource);
> +    g_object_unref(webDataSource);

Mmm, this seems a bit pointless:

- Create a loader.
- Create a datasource from the loader (and nothing else).
- Set the datasource in the loader.

Couldn't this happen automatically with a 'create' of our own or something like
that?

> +#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());
> +    priv->loader->detachDataSource();
> +    priv->loader->deref();
> +
> +    if (priv->initialRequest)
> +	   g_object_unref(priv->initialRequest);
> +
> +    if (priv->networkRequest)
> +	   g_object_unref(priv->networkRequest);
> +
> +    if (priv->mainResource)
> +	   g_object_unref(priv->mainResource);
> +
> +    if (priv->data)
> +	   g_string_free(priv->data, FALSE);

You need to set all those to NULL, dispose can be called multiple times.

> +
> +    if (priv->subresources) {
> +	   for (unsigned i=0; i < priv->subresources->len; i++) {
> +	       WebKitWebResource* res = &g_array_index(priv->subresources,
WebKitWebResource, i);
> +	       if (res)
> +		   g_object_unref(res);
> +	   }
> +
> +	   g_array_free(priv->subresources, TRUE);
> +    }

What's the point of using an array here? A list seems much more natural (seems
Gustavo suggests a hash to be able to get easily a resource by its identifier,
that might make sense too).

> +
> +    G_OBJECT_CLASS(webkit_web_data_source_parent_class)->dispose(object);
> +}
> +
> +static void webkit_web_data_source_finalize(GObject* object)
> +{
> +    WebKitWebDataSource* dataSource = WEBKIT_WEB_DATA_SOURCE(object);
> +    WebKitWebDataSourcePrivate* priv = dataSource->priv;
> +
> +    priv->unreachableURL = CString();

Same comment about this than in previous patch.

> +/**
> + * webkit_web_data_source_new:
> + * @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.12
> + */
> +WebKitWebDataSource* webkit_web_data_source_new(WebKitNetworkRequest*
request)
> +{
> +    ASSERT(request);
> +
> +    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;
> +
> +    return datasource;
> +}

_new functions should, when possible, only be a call to g_object_new (otherwise
bindings have to override them). This could probably be fixed by making the
request/uri a property of the object and doing the needed setup there?

> +
> +/**
> + * webkit_web_data_source_get_text_encoding_name:

Same than in the other patch, we might want to just call thes
"get_text_encoding" to be consistent with existing APIs.

> +
> +/**
> + * 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.
> + *
> + * Since: 1.1.12
> + */
> +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(mainResourceData->data());

Can the data have '0'? If yes, then you need the length and do
g_string_new_with_len.

> +
> +/**
> + * webkit_web_data_source_get_subresources:
> + * @data_source: a #WebKitWebDataSource
> + *
> + * Returns the @data_source subresources that have finished downloading
> + *
> + * Return value: a #GArray of #WebKitWebResource. The array if owned by
WebKit
> + * and should not be freed or destroyed.
> + *
> + * Since: 1.1.12
> + */
> +GArray* webkit_web_data_source_get_subresources(WebKitWebDataSource*
webDataSource)

See previous comment about using a list or a hash table here.

> +
> +/**
> + * webkit_web_data_source_get_resource_for_url:
> + * @data_source: a #WebKitWebDataSource
> + * @url: the url of a resource
> + *
> + * Returns the #WebKitWebResource for the given URL
> + *
> + * Return value: the #WebKitWebResource for @url
> + *
> + * Since: 1.1.12
> + */
> +WebKitWebResource*
webkit_web_data_source_get_resource_for_url(WebKitWebDataSource* webDataSource,
const gchar* url)

Mmm, shouldn't this return one of the elements in the array we have?

Marking r- while we address some of the issues.