Created attachment 239460 [details] [review]
Patch

Didn't add the "Since:" to the docs because I don't know if we want to add this for 2.42

(In reply to comment #1)
> Didn't add the "Since:" to the docs because I don't know if we want to add this
> for 2.42

hard code freeze was Monday, so it will be 2.44

Comment on attachment 239460 [details] [review]
Patch

(Too late for 2.42, but since I'm already reading this bug...)

Basically looks good. For some reason, I was thinking it was going to have to be more complicated than this.

>+	/**
>+	 * SOUP_MESSAGE_PRIORITY:
>+	 *
>+	 * Alias for the #SoupMessage:priority property (the
>+	 * message's priority).
>+	 **/

You should override the default g_param_spec-generated doc with a SoupMessage:priority gtk-doc, and put "see soup_message_set_priority() for more details" there. (Or else include the relevant bits from those docs in the property docs as well.)

>+ * Sets the priority of a message. Note that this won't have any
>+ * effect unless used before the message is added to the message
>+ * processing queue

If this restriction turns out to be a problem, the queue could connect to notify::priority and re-sort itself when a message changes priority. But we don't need to implement that unless we need it later.

>    either by calling soup_session_queue_message(),
>+ * soup_session_send_message(), soup_session_send() or
>+ * soup_session_send_async().

As currently written, the code will not work with SoupSessionSync (or with synchronous messages on a plain SoupSession), because in the synchronous/blocking case, priority ends up being determined semi-randomly by thread scheduling. I want to fix that some day, but for now I'd say just document that it doesn't work in that case. Oh, and you forgot soup_request_send*(). Actually, you probably don't need to list the specific methods, you can just say "added to the session's message processing queue" or something like that.

oh, needs a test :)

(In reply to comment #3)
> (From update of attachment 239460 [details] [review])

> >    either by calling soup_session_queue_message(),
> >+ * soup_session_send_message(), soup_session_send() or
> >+ * soup_session_send_async().
> 
> As currently written, the code will not work with SoupSessionSync (or with
> synchronous messages on a plain SoupSession), because in the
> synchronous/blocking case, priority ends up being determined semi-randomly by
> thread scheduling. I want to fix that some day, but for now I'd say just
> document that it doesn't work in that case. Oh, and you forgot
> soup_request_send*(). Actually, you probably don't need to list the specific
> methods, you can just say "added to the session's message processing queue" or
> something like that.

True, I always forget about the synchronous case.

Created attachment 239483 [details] [review]
Patch

Comment on attachment 239483 [details] [review]
Patch

>+SOUP_VERSION_2_44

please add the version 2.44 stuff as a separate commit

>+	expected_priorities[0] = SOUP_MESSAGE_PRIORITY_HIGH;
>+	expected_priorities[1] = SOUP_MESSAGE_PRIORITY_NORMAL;
>+	expected_priorities[2] = SOUP_MESSAGE_PRIORITY_LOW;

Swap that around to HIGH, LOW, NORMAL. As it is now, the test will pass even if priorities are completely broken, because the default would be for the first-queued message to run first, then the second, then the third.


Other than that, this looks good. I haven't branched yet, so this has to wait until after then (next Monday).

(In reply to comment #7)
> (From update of attachment 239483 [details] [review])
> >+SOUP_VERSION_2_44
> 
> please add the version 2.44 stuff as a separate commit

OK, I'll do

> >+	expected_priorities[0] = SOUP_MESSAGE_PRIORITY_HIGH;
> >+	expected_priorities[1] = SOUP_MESSAGE_PRIORITY_NORMAL;
> >+	expected_priorities[2] = SOUP_MESSAGE_PRIORITY_LOW;
> 
> Swap that around to HIGH, LOW, NORMAL. As it is now, the test will pass even if
> priorities are completely broken, because the default would be for the
> first-queued message to run first, then the second, then the third.

Not really because the priorities are set using the priorities[] array not that one, and thus are set to LOW, HIGH, NORMAL, which will be the expected result without priorities.

ah, right. ok, so it's good to commit post-freeze, with the 2.44 defines split out separately first

Comment on attachment 239483 [details] [review]
Patch

libsoup is branched now

Fixed in master c146806d