Background thumbnails should use a low network priority

RESOLVED FIXED in Firefox 25

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
Firefox 25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 772971 [details] [diff] [review]
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.
Attachment #772971 - Flags: feedback?(gavin.sharp)
Attachment #772971 - Flags: feedback?(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.
Attachment #772971 - Flags: feedback?(adw) → feedback+
(Assignee)

Comment 2

6 years ago
Created attachment 773786 [details] [diff] [review]
Updated based on feedback

Using Ci.nsISupportsPriority.PRIORITY_LOWEST as suggested.
Attachment #772971 - Attachment is obsolete: true
Attachment #772971 - Flags: feedback?(gavin.sharp)
Attachment #773786 - Flags: review?(adw)
(Assignee)

Comment 3

6 years ago
Created attachment 773787 [details] [diff] [review]
qref!

*sob*
Attachment #773786 - Attachment is obsolete: true
Attachment #773786 - Flags: review?(adw)
Attachment #773787 - Flags: review?(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).
Attachment #773787 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/c958ad88e85d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.