Bug 189505 – [WPE][GTK] API documentation is unclear about how to modify requests before sending them

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Adrian Perez]

There are two places where a WebKitURIRequest can be inspected
before it is sent over the network, and it is not clear in the
documentation in which case the modifications apply to it will
be reflected in the actual data sent over the network:

 - Signal handlers for WebKitWebView::decide-policy
   Modifications *ARE NOT* sent over the network.

 - Signal handlers for WebKitPage::send-request (in a Web extension)
   Modifications *ARE* sent over the network.

It would be useful if the API documentation was explicit about
this.

[Adrian Perez]

Created attachment 349403 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

Comment on attachment 349403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349403&action=review

I've been confused by this before too. Thanks.

> Source/WebKit/UIProcess/API/glib/WebKitNavigationAction.cpp:128
> + * Modifications to the returned object are <emphasis>not</emphasis> taken

Hm, gtk-doc removed support for XML a long time ago. Are you sure this is actually going to work? I suspect the output will literally say <emphasis>not</emphasis>. Do you have some recent HTML example of this working?

I think it's supposed to be markdown instead?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 349403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349403&action=review
> 
> I've been confused by this before too. Thanks.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitNavigationAction.cpp:128
> > + * Modifications to the returned object are <emphasis>not</emphasis> taken
> 
> Hm, gtk-doc removed support for XML a long time ago. Are you sure this is
> actually going to work? I suspect the output will literally say
> <emphasis>not</emphasis>. Do you have some recent HTML example of this
> working?

Carlos García figured out that <emphasis> works in his patch for bug #188742

> I think it's supposed to be markdown instead?

Apparently DocBook is still being used, but in versions ≥1.20 it is
also possible to use Markdown intermixed with DocBook. Or at least
that's what I understand from reading here:

  https://developer.gnome.org/gtk-doc-manual/stable/documenting_syntax.html.en

  “While markdown is now preferred one can mix both. One limitation here
   is that one can use docbook xml within markdown, but markdown within
   docbook xml is not supported.”

¯\_(ツ)_/¯

[WebKit Commit Bot]

Comment on attachment 349403 [details]
Patch

Clearing flags on attachment: 349403

Committed r235909: <https://trac.webkit.org/changeset/235909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44352135>

[Carlos Garcia Campos]

Comment on attachment 349403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349403&action=review

> Source/WebKit/UIProcess/API/glib/WebKitNavigationAction.cpp:130
> + * into account when the request is sent over the network, and is intended
> + * only to aid in evaluating whether a navigation action should be taken or

This is not accurate either. NavigationAction is used in policy decisions, but also in create signal. It's not intended to decide whether the action should be taken or not, it's just information, the intentation of that information depends on the caller rather tahn the action object itself. I would add this comment to decide-policy signal, to clarify that you can ignore/download/use, but request can't be modified at that point.

> Source/WebKit/UIProcess/API/glib/WebKitResponsePolicyDecision.cpp:121
> + * Return the #WebKitURIRequest associated with the response decision.
> + * Modifications to the returned object are <emphasis>not</emphasis> taken
> + * into account when the request is sent over the network, and is intended
> + * only to aid in evaluating whether a response decision should be taken or
> + * not. To modify requests before they are sent over the network the

It makes more sense here in this context, but maybe we can add a single comment in the signal description, you are suggesting to use a different signal in the end.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #8)
> Comment on attachment 349403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349403&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitNavigationAction.cpp:130
> > + * into account when the request is sent over the network, and is intended
> > + * only to aid in evaluating whether a navigation action should be taken or
> 
> This is not accurate either. NavigationAction is used in policy decisions,
> but also in create signal. It's not intended to decide whether the action
> should be taken or not, it's just information, the intentation of that
> information depends on the caller rather tahn the action object itself. I
> would add this comment to decide-policy signal, to clarify that you can
> ignore/download/use, but request can't be modified at that point.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitResponsePolicyDecision.cpp:121
> > + * Return the #WebKitURIRequest associated with the response decision.
> > + * Modifications to the returned object are <emphasis>not</emphasis> taken
> > + * into account when the request is sent over the network, and is intended
> > + * only to aid in evaluating whether a response decision should be taken or
> > + * not. To modify requests before they are sent over the network the
> 
> It makes more sense here in this context, but maybe we can add a single
> comment in the signal description, you are suggesting to use a different
> signal in the end.

Thanks for the feedback, I'll make a follow-up patch soon to further
improve the documentation taking it into account :-)