We need a class to handle response policy decisions.
Created attachment 123456 [details] Patch
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 on attachment 123456 [details] Patch 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
Created attachment 123834 [details] Patch
(In reply to comment #3) > (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. 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.
(In reply to comment #5) > (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). Ah, good point, I didn't know there were more policy decision types.
Comment on attachment 123834 [details] Patch 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"
Created attachment 123983 [details] Patch
(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.
Created attachment 123986 [details] Patch
Comment on attachment 123986 [details] Patch Looks good to me
Comment on attachment 123986 [details] Patch 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
Committed r107043: <http://trac.webkit.org/changeset/107043>