[webkit-reviews] review denied: [Bug 29134] [GTK] Add API to access sub resources : [Attachment 39481] comments addressed, tests added

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 29134: [GTK] Add API to access sub resources
https://bugs.webkit.org/show_bug.cgi?id=29134

Attachment 39481: comments addressed, tests added
https://bugs.webkit.org/attachment.cgi?id=39481&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> +
> +	https://bugs.webkit.org/show_bug.cgi?id=29134
> +	[GTK] Add API to access sub resources
> +
> +	   Implement getting subresources, and improve testing of
> +	main, and sub resources loading.
> +
> +	   * WebCoreSupport/FrameLoaderClientGtk.cpp:
> +	   (WebKit::identifierToString):
> +	   (WebKit::FrameLoaderClient::dispatchWillSendRequest):
> +	   (WebKit::FrameLoaderClient::assignIdentifierToInitialRequest):
> +	   (WebKit::FrameLoaderClient::provisionalLoadStarted):
> +	   (WebKit::FrameLoaderClient::dispatchDidFinishLoading):
> +	   * tests/testwebresource.c:
> +	   (server_callback):
> +	   (resource_request_starting_cb):
> +	   (load_finished_cb):
> +	   (test_web_resource_loading):
> +	   (resource_request_starting_sub_cb):
> +	   (load_finished_sub_cb):
> +	   (idle_quit_loop_cb):
> +	   (test_web_resource_sub_resource_loading):
> +	   (main):
> +	   * webkit/webkitprivate.h:
> +	   * webkit/webkitwebdatasource.cpp:
> +	   (webkit_web_data_source_get_main_resource):
> +	   (webkit_web_data_source_get_sub_resources):
> +	   * webkit/webkitwebdatasource.h:
> +	   * webkit/webkitwebresource.cpp:
> +	   (webkit_web_resource_init_with_core_resource):
> +	   (webkit_web_resource_get_data):
> +	   * webkit/webkitwebview.cpp:
> +	   (webkit_web_view_dispose):
> +	   (webkit_web_view_finalize):
> +	   (webkit_web_view_init):
> +	   (webkit_web_view_add_resource):
> +	   (webkit_web_view_get_resource):
> +	   (webkit_web_view_get_main_resource):
> +	   (webkit_web_view_clear_resources):
> +	   (webkit_web_view_get_sub_resources):

No need to check the method names unless they have specific comments.

> +static char* identifierToString(unsigned long identifier)

Maybe name this toString instead.

> +    if (!redirectResponse.isNull()) {
> +	   // This is a redirect, so we need to update the WebResource's
knowledge
> +	   // of the URI.
> +	   g_free(webResource->priv->uri);
> +	   webResource->priv->uri =
g_strdup(request.url().string().utf8().data());

I would've preferred to set the property instead of accessing private members
like this. 

> -void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long,
WebCore::DocumentLoader*, const ResourceRequest&)
> +void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long
identifier, WebCore::DocumentLoader*, const ResourceRequest& request)
>  {
> -    notImplemented();
> +    webkit_web_view_add_resource(getViewFromFrame(m_frame),
identifierToString(identifier),
> +				   
WEBKIT_WEB_RESOURCE(g_object_new(WEBKIT_TYPE_WEB_RESOURCE, "uri",
request.url().string().utf8().data(), 0)));
>  }
>  
>  void FrameLoaderClient::postProgressStartedNotification()
> @@ -799,7 +815,10 @@ void
FrameLoaderClient::finishedLoading(WebCore::DocumentLoader* documentLoader)
>  
>  void FrameLoaderClient::provisionalLoadStarted()
>  {
> -    notImplemented();
> +    WebKitWebView* webView = getViewFromFrame(m_frame);
> +
> +    if (m_frame == webkit_web_view_get_main_frame(webView))
> +	   webkit_web_view_clear_resources(webView);
>  }
>  
>  void FrameLoaderClient::didFinishLoad() {
> @@ -824,6 +843,23 @@ void
FrameLoaderClient::dispatchDidFinishLoading(WebCore::DocumentLoader* loader
>  {
>     
static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
>  
> +    WebKitWebView* webView = getViewFromFrame(m_frame);
> +    GOwnPtr<gchar> identifierString(identifierToString(identifier));
> +    WebKitWebResource* webResource = webkit_web_view_get_resource(webView,
identifierString.get());
> +
> +    const char* uri = webkit_web_resource_get_uri(webResource);
> +    RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(),
uri)));
> +
> +    // If coreResource is NULL here, the resource failed to load,
> +    // unless it's the main resource.
> +    if (!coreResource && webResource !=
webkit_web_view_get_main_resource(webView))

You could just get the main resource from the data source here.

> +    if (!coreResource)
> +	   coreResource = loader->mainResource().releaseRef();
> +
> +    webkit_web_resource_init_with_core_resource(webResource,
coreResource.get());
> +

What's this for?

> diff --git a/WebKit/gtk/webkit/webkitprivate.h
b/WebKit/gtk/webkit/webkitprivate.h
> index c471714..38c6869 100644
> --- a/WebKit/gtk/webkit/webkitprivate.h
> +++ b/WebKit/gtk/webkit/webkitprivate.h
> @@ -140,6 +140,12 @@ extern "C" {
>  
>	   gboolean disposing;
>	   gboolean usePrimaryForPaste;
> +
> +	   // These are hosted here because the DataSource object is
> +	   // created too late in the frame loading process.

Can you please expound more about when we need the resource and why it's too
late in the frame loading process? Can we have a resource without a data
source/document loader?
Are we doing something different from Mac here?

> +	   WebKitWebResource* mainResource;
> +	   char* mainResourceIdentifier;
> +	   GHashTable* subResources;

Same. Can we have a main resource without a data source/document loader?

>     
webkit_web_resource_new_with_core_resource(PassRefPtr<WebCore::ArchiveResource>
);
>  
> +    void
> +    webkit_web_resource_init_with_core_resource(WebKitWebResource*,
PassRefPtr<WebCore::ArchiveResource>);
> +

You could just add a WebResource param to _new_with_core_resource and check for
NULL in the impl.
  
> +    void
> +    webkit_web_view_add_resource(WebKitWebView*, char*, WebKitWebResource*);

> +
> +    WebKitWebResource*
> +    webkit_web_view_get_resource(WebKitWebView*, char*);
> +
> +    WebKitWebResource*
> +    webkit_web_view_get_main_resource(WebKitWebView*);
> +
> +    void
> +    webkit_web_view_clear_resources(WebKitWebView*);
> +
> +    GList*
> +    webkit_web_view_get_sub_resources(WebKitWebView*);

> -    RefPtr<ArchiveResource> coreResource = priv->loader->mainResource();
> +    if (priv->mainresource)
> +	   return priv->mainresource;
>  
> -    ASSERT(coreResource);
> +    WebKitWebFrame* webFrame =
webkit_web_data_source_get_web_frame(webDataSource);
> +    WebKitWebView* webView = getViewFromFrame(webFrame);
>  
> -    if (priv->mainresource)
> -	   g_object_unref(priv->mainresource);
> +    priv->mainresource =
WEBKIT_WEB_RESOURCE(g_object_ref(webkit_web_view_get_main_resource(webView)));
>  
> -    priv->mainresource =
webkit_web_resource_new_with_core_resource(coreResource.release());

How is the view mainresource different from the DS main resource?

r-. We need to clarify why we can't use DS for these and expound more on
difference between WebView resources to DS resources.