Closed Bug 891640 Opened 7 years ago Closed 7 years ago
Background thumbnails should use a low network priority
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 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+
Using Ci.nsISupportsPriority.PRIORITY_LOWEST as suggested.
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+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.