Bug 76789 - [GTK] [WK2] Add WebKitResponsePolicyDecision
: [GTK] [WK2] Add WebKitResponsePolicyDecision
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 76343
:
  Show dependency treegraph
 
Reported: 2012-01-21 13:11 PST by
Modified: 2012-02-08 07:45 PST (History)


Attachments
Patch (21.84 KB, patch)
2012-01-21 13:37 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.87 KB, patch)
2012-01-24 16:08 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (66.28 KB, patch)
2012-01-25 11:53 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.93 KB, patch)
2012-01-25 12:07 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-21 13:11:24 PST
We need a class to handle response policy decisions.
------- Comment #1 From 2012-01-21 13:37:44 PST -------
Created an attachment (id=123456) [details]
Patch
------- Comment #2 From 2012-01-21 13:41:20 PST -------
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
------- Comment #3 From 2012-01-23 01:25:43 PST -------
(From update of attachment 123456 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=123456&action=review

Looks good, I would just move the request to the parent class.

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:51
> +    GRefPtr<WebKitURIRequest> request;

Both Response and Navigation have a Request, so I guess it should be inherited

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:126
> +/*

Double * is needed here.

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:132
> + * Returns: The URI request that is associated with this policy decision.

Mark this as transfer none

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:140
> +/*

Double * here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:146
> + * Returns: The URI response that is associated with this policy decision.

transfer none

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:158
> +    decision->priv->request = adoptGRef(webkitURIRequestCreateForResourceRequest(toImpl(request)->resourceRequest()));
> +    decision->priv->response = adoptGRef(webkitURIResponseCreateForResourceResponse(toImpl(response)->resourceResponse()));

I think these should be construct only properties
------- Comment #4 From 2012-01-24 16:08:38 PST -------
Created an attachment (id=123834) [details]
Patch
------- Comment #5 From 2012-01-24 16:12:07 PST -------
(In reply to comment #3)
> (From update of attachment 123456 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123456&action=review
> 
> Looks good, I would just move the request to the parent class.

It's probably not a good idea in this case because there may be other policy decision types that do not have requests associated with them (such as geolocation).

> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:126
> > +/*
> 
> Double * is needed here.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:132
> > + * Returns: The URI request that is associated with this policy decision.
> 
> Mark this as transfer none

Fixed.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:140
> > +/*
> 
> Double * here too.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:146
> > + * Returns: The URI response that is associated with this policy decision.
> transfer none

Fixed. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:158
> > +    decision->priv->request = adoptGRef(webkitURIRequestCreateForResourceRequest(toImpl(request)->resourceRequest()));
> > +    decision->priv->response = adoptGRef(webkitURIResponseCreateForResourceResponse(toImpl(response)->resourceResponse()));
> 
> I think these should be construct only properties

I skipped this suggestion as well for the reason I outlined on the original policy client bug.
------- Comment #6 From 2012-01-25 09:05:24 PST -------
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 123456 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=123456&action=review
> > 
> > Looks good, I would just move the request to the parent class.
> 
> It's probably not a good idea in this case because there may be other policy decision types that do not have requests associated with them (such as geolocation).

Ah, good point, I didn't know there were more policy decision types.
------- Comment #7 From 2012-01-25 09:08:44 PST -------
(From update of attachment 123834 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=123834&action=review

Looks good to me

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:29
> +#include <WebKit2/WKAPICast.h>

Already included by WebKitPrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecisionPrivate.h:24
> +#include <WebKitPrivate.h>

Use #include "WebKitPrivate.h"
------- Comment #8 From 2012-01-25 11:53:56 PST -------
Created an attachment (id=123983) [details]
Patch
------- Comment #9 From 2012-01-25 12:06:35 PST -------
(In reply to comment #7)

> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:29
> > +#include <WebKit2/WKAPICast.h>
> 
> Already included by WebKitPrivate.h

Fixed.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecisionPrivate.h:24
> > +#include <WebKitPrivate.h>
> 
> Use #include "WebKitPrivate.h"

Fixed.
------- Comment #10 From 2012-01-25 12:07:13 PST -------
Created an attachment (id=123986) [details]
Patch
------- Comment #11 From 2012-02-07 00:38:07 PST -------
(From update of attachment 123986 [details])
Looks good to me
------- Comment #12 From 2012-02-07 06:44:01 PST -------
(From update of attachment 123986 [details])
Rejecting attachment 123986 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
yDecisionPrivate.h
patching file Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-docs.sgml
patching file Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt
patching file Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp
Hunk #2 succeeded at 145 (offset 8 lines).
Hunk #3 succeeded at 225 (offset 8 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Philippe N..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11443281
------- Comment #13 From 2012-02-07 23:21:49 PST -------
Committed r107043: <http://trac.webkit.org/changeset/107043>