[Webkit-unassigned] [Bug 24758] [Gtk] Implement a WebDataSource for the gtk port

[bugzilla-daemon at webkit.org]

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





------- Comment #2 from gns at gnome.org  2009-03-23 14:27 PDT -------
(From update of attachment 28853)
> +DocumentLoaderGtk::DocumentLoaderGtk(const ResourceRequest& request, const SubstituteData& substituteData)
> +    : DocumentLoader(request, substituteData)
> +    , m_isDataSourceReffed(false)
> +    , m_dataSource(0)

Although mac seems to do it, I think GTK+ classes usually do not append 'Gtk'
to their name. If there is no reason to do this rename, I think we should go
with DocumentLoader::DocumentLoader.

> +void DocumentLoaderGtk::attachToFrame()
> +{
> +    DocumentLoader::attachToFrame();
> +
> +    refDataSource();
> +}

Is this the cause for the rename?

> -void FrameLoaderClient::dispatchWillSendRequest(DocumentLoader*, unsigned long, ResourceRequest&, const ResourceResponse&)
> +void FrameLoaderClient::dispatchWillSendRequest(DocumentLoader* loader, unsigned long identifier, ResourceRequest& request, const ResourceResponse& redirectResponse)
>  {
> -    notImplemented();
> +    if (redirectResponse.isNull())
> +        static_cast<DocumentLoaderGtk*>(loader)->increaseLoadCount(identifier);
>  }

These casts are not something I see in our other webcore support classes. Are
they needed? If they are, perhaps it would be better to cook up a core()/kit()
conversion pair, such as the ones that exist for things such as WebView and
WebFrame.

>      WebKitWebNavigationReason kit(WebCore::NavigationType type);
>      WebCore::NavigationType core(WebKitWebNavigationReason reason);
> +
>  }

Here's the rogue blank line I mentioned.

> +    // WebKitWebDataSource private
> +    WEBKIT_API WebKitWebDataSource*
> +    webkit_web_data_source_init_with_loader(PassRefPtr<DocumentLoaderGtk>);
> +

I would make this webkit_web_data_source_new_with_core_loader, to match the
already-existing history item one. If I remember correctly, I am using
_with_core_request in my proposed NetworkRequest implementation, too.

> +G_CONST_RETURN gchar* webkit_web_data_source_get_unreachable_url(WebKitWebDataSource* webDataSource)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(webDataSource), NULL);
> +
> +    WebKitWebDataSourcePrivate* priv = webDataSource->priv;
> +    const KURL& unreachableURL = priv->loader->unreachableURL();
> +
> +    if (unreachableURL.isEmpty())
> +        return NULL;
> +    else
> +        return unreachableURL.string().utf8().data();
> +}

What is this used for?

> + * webkit_web_frame_get_provisional_data_source:
> + * @frame: a #WebKitWebFrame
> + *
> + * Returns the provisional data source. You use the
> + * webkit_web_frame_load_request method to initiate an asynchronous client
> + * request which will create a provisional data source. The provisional data
> + * source will transition to a committed data source once any data has been
> + * received. Therefore this method returns NULL if either a load request is
> + * not in progress, or a load request has completed.

I think this comment seems to imply that the user needs to take action to cause
loads to initiate. It would be better to rephrase this, so that it is easily
understandable that any loads initiated by WebKit or by the user explicitly
with load_request will cause a provisional data source to be created.

Looking good to me. I think this is a very important missing piece in our API.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.