[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


christian at twotoasts.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |christian at twotoasts.de




------- Comment #3 from christian at twotoasts.de  2009-03-23 14:36 PDT -------
This is already looking interesting. I have a few comments for you :)

There are three files which don't attribute copyright to you and it looks as
though an Apple employée wrote them. You might want to add your name there,
even
if they are based on existing code.

+static void webkit_web_data_source_init(WebKitWebDataSource* webDataSource)
+{
+    JSC::initializeThreading();
+    webDataSource->priv = WEBKIT_WEB_DATA_SOURCE_GET_PRIVATE(webDataSource);
+}

The initialisation call here looks out of place. Would it make sense to put
that inside webkit_init?

+WebKitWebDataSource* webkit_web_data_source_new(WebKitNetworkRequest* request)
+{
+    const gchar* uri = webkit_network_request_get_uri(request);
+    return
webkit_web_data_source_init_with_loader(DocumentLoaderGtk::create(ResourceRequest(KURL(KURL(),
String::fromUTF8(uri))), SubstituteData()));
+}

This line is just too long, please add a line break here ;)

+WebKitWebFrame* webkit_web_data_source_get_web_frame(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)
+        return NULL;

Do we usually expect a NULL value? In that case, we should document it,
otherwise it should be an ASSERT.

+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;
+
+    ResourceRequest request = priv->loader->request();
+    WebKitNetworkRequest* webRequest =
webkit_network_request_new(request.url().string().utf8().data());
+
+    return webRequest;
+}

I'm guessing this depends on the improvements of WebKitNetworkRequest so we can
use the actual request rather than creating a new one, so I suggest a FIXME
here. And we want to store the request, otherwise subsequent calls will lead
to unexpected results, ie. memory leaks and failing pointer comparisons.

+ * Returns %TRUE if @data_source is in the process of loading its content,
+ * otherwise it returns %FALSE
+ *
+ * Return value: %TRUE if the @data_source is still loading, %FALSE if it's
+ * already loaded
+ *
+ * Since: 1.1.4

You want to avoid the similar wording here. The main description could be
something like "Determines whether the @data_source is currently in the process
of loading its content" and the return value perhaps "%TRUE if the @data_source
is currently loading, %FALSE otherwise".

+WEBKIT_API WebKitNetworkRequest *
+webkit_web_data_source_get_request            (WebKitWebDataSource 
*data_source);

This should likely be _get_network_request.

+WEBKIT_API G_CONST_RETURN gchar *
+webkit_web_data_source_get_text_encoding_name (WebKitWebDataSource 
*data_source);

We have functions _get_encoding and _set_encoding, I think we should use
_get_encoding here accordingly.

+WebKitWebDataSource*
webkit_web_frame_get_provisional_data_source(WebKitWebFrame* frame)
+{
+    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);
+
+    Frame* coreFrame = core(frame);
+    WebKitWebDataSource* dataSource =
webkit_web_frame_get_data_source_from_core_loader(coreFrame->loader()->provisionalDocumentLoader());
+
+    g_return_val_if_fail(WEBKIT_IS_WEB_DATA_SOURCE(dataSource), NULL);
+
+    return dataSource;
+}

The return_if_fail in there is wrong. The comment says NULL is returned if
there
is no provisional data source. And if you want to make sure WebCore doesn't
return anything else that must be an ASSERT.
The same for the non-provisional data source.


-- 
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.