891640 - Background thumbnails should use a low network priority

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: Drop the network priority of the remote browser used for the capture

The background thumbnail code should use a low network priority in the aim of keeping user-initiated network requests as responsive as possible.

I was originally skeptical this patch works correctly, given both that the networking code lives in the parent and this lives in the child, and bug 666804.

So to verify this was working, I patched netwerk/protocol/http/nsHttpChannel.cpp, adding a line to nsHttpChannel::SetPriority(int32_t value):

+    printf("http set priority - now %d\n", newValue);

and also adjusted the PRIORITY_DELTA in this patch from 20 to 44 (just to make it an easily recognizable value :)

Then, upon running the background thumbnail tests, I see in the output various lines like:

0:02.82 http set priority - now 44

which strongly indicates to me that the code is doing the right thing.  Assuming that's true, it therefore means the assertion in bug 666804 comment 1 is now incorrect - that xul:browser's remote webNavigation now *does* seem to implement nsIDocumentLoader.

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 772971 [details] [diff] [review]
Drop the network priority of the remote browser used for the capture

I think it would be easier to understand what's going on if you set the priority property directly to nsISupportsPriority.PRIORITY_LOWEST.

(In reply to Mark Hammond (:markh) from comment #0)
> Assuming that's true, it therefore means the assertion in bug 666804 comment
> 1 is now incorrect - that xul:browser's remote webNavigation now *does* seem
> to implement nsIDocumentLoader.

That comment is referring to the webNavigation property of remote browsers: browser.webNavigation.QueryInterface(nsIDocumentLoader) won't work because for remote browsers, browser.webNavigation is a stub object that doesn't implement nsIDocumentLoader: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#318

But it sounds like your patch shows that nsIDocumentLoader is supported in the content process, great.

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: Updated based on feedback

Using Ci.nsISupportsPriority.PRIORITY_LOWEST as suggested.

Comment 3

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: qref!

*sob*

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 773787 [details] [diff] [review]
qref!

Review of attachment 773787 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +18,5 @@
>                  getInterface(Ci.nsIDOMWindowUtils);
>      dwu.preventFurtherDialogs();
>  
> +    // We want a low network priority for this service - lower than b/g tabs
> +    // etc - so setit to the lowest priority available.

setit -> set it

@@ +21,5 @@
> +    // We want a low network priority for this service - lower than b/g tabs
> +    // etc - so setit to the lowest priority available.
> +    let supportsPriority = this._webNav
> +                            .QueryInterface(Ci.nsIDocumentLoader)
> +                            .loadGroup.QueryInterface(Ci.nsISupportsPriority);

Please put dots at the end of lines to match the style here.

@@ +22,5 @@
> +    // etc - so setit to the lowest priority available.
> +    let supportsPriority = this._webNav
> +                            .QueryInterface(Ci.nsIDocumentLoader)
> +                            .loadGroup.QueryInterface(Ci.nsISupportsPriority);
> +    supportsPriority.adjustPriority(Ci.nsISupportsPriority.PRIORITY_LOWEST);

This probably works, but adjustPriority adds a delta to the current priority, so (a) it doesn't really make sense to pass an absolute priority value to it, and (b) it's not technically what we want.  We want to use an absolute low priority regardless of the current priority, so please just assign loadGroup.QueryInterface(Ci.nsISupportsPriority).priority = Ci.nsISupportsPriority.PRIORITY_LOWEST (or LOW).

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c958ad88e85d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c958ad88e85d