[webkit-reviews] review denied: [Bug 24758] [Gtk] Implement a WebDataSource for the gtk port : [Attachment 29263] web data source and documentloader impl for gtk port

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Maciej Stachowiak
<mjs at apple.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 29263: web data source and documentloader impl for gtk port
https://bugs.webkit.org/attachment.cgi?id=29263&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +void DocumentLoader::setDataSource(WebKitWebDataSource* dataSource,
WebKitWebView* webView)
> +{
> +    ASSERT(!m_dataSource);
> +
> +    m_dataSource = dataSource;
> +    refDataSource();
> +}

What is the webView used for?

> +#include <wtf/HashSet.h>
> +
> +#include "DocumentLoader.h"
> +
> +#include "webkitdefines.h"

I think you should order these, and make remove the empty lines.

> +namespace WebCore {
> +
> +    class ResourceRequest;
> +    class SubstituteData;
> +}

Needless empty line, or add an additional one at the end.

> -FrameLoaderClient::shouldUseCredentialStorage(DocumentLoader*, unsigned long
 identifier)
> +FrameLoaderClient::shouldUseCredentialStorage(WebCore::DocumentLoader*,
unsigned long  identifier)

Why these changes? There's a using statement already. Please remove these, they
just add noise to the patch, IMO.

> +#include "config.h"
> +
> +#include "webkitwebdatasource.h"

Join those.

> +#include "webkitwebresource.h"
> +#include "webkitprivate.h"
> +
> +#include <glib.h>
> +#include <runtime/InitializeThreading.h>
> +#include <wtf/Assertions.h>
> +
> +#include "ArchiveResource.h"
> +#include "DocumentLoaderGtk.h"
> +#include "FrameLoaderClientGtk.h"
> +#include "FrameLoader.h"
> +#include "KURL.h"
> +#include "PlatformString.h"
> +#include "ResourceRequest.h"
> +#include "SharedBuffer.h"
> +#include "SubstituteData.h"

Join and sort the rest.

> +extern "C" {

Unnecessary, right?

> + * Creates a new #WebKitWebDataSource from a #WebKitNetworkRequest.
Normally,
> + * #WebKitWebFrame objects create their data sources, so don't invoke this
> + * method directly.

I think s/don't invoke/you will almost never want to invoke/, to match similar
documentation on GTK+.

> + * webkit_web_data_source_get_text_encoding_name:

Renaming these is still pending.

> +G_CONST_RETURN gchar* webkit_web_data_source_get_data(WebKitWebDataSource*
webDataSource)

My point about these still stands. We are not sure there are no \0's in the
data, so we better return the length, too. So I suggest:

G_CONST_RETURN gchar* webkit_web_data_source_get_data(WebKitWebDataSource*
webDataSource, gsize* length)

> +    if (!priv->mainResource)
> +	   priv->mainResource =
webkit_web_resource_init_with_core_resource(coreResource.release());

new_with_core, like history (still stands)

> + * Return value: a #GArray of #WebKitWebResource. You need to free the array

> + * and its subresources once you are done with it.

Uh? Why would the user need to free the GArray?

Also, Mac seems to use the identifier of the resource for various APIs. Should
we return a GHashTable that maps identifiers to the actual sub resources, and
allow for things such as requesting for a specific resource using the
identifier?

> +    // WebKitWebDataSource private
> +    WebKitWebDataSource*
> +   
webkit_web_data_source_new_with_loader(PassRefPtr<WebKit::DocumentLoader>);
> +

new_with_core_resource should be here, too?

>      Frame* coreFrame = core(frame);
> -    DocumentLoader* docLoader = coreFrame->loader()->documentLoader();
> +    WebCore::DocumentLoader* docLoader =
coreFrame->loader()->documentLoader();
>      String mimeType = docLoader->responseMIMEType();
>      return g_strdup(mimeType.utf8().data());

Not necessary?

r- for now