Closed
Bug 1337672
Opened 8 years ago
Closed 8 years ago
Reduce the priority of network requests for <link rel="prefetch">
Categories
(Core :: Networking: HTTP, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
(Whiteboard: [necko-active])
Attachments
(2 files)
These patches will reduce the priority of network requests for <link rel="prefetch"> and Flash plugin content. Honza, if you think this is a good idea, should I use the nsISupportsPriority interface (as used for Tracking Protection requests in bug 1141814) and/or nsIClassOfService's Throttleable flag (added in bug 1312745)? Would nsIClassOfService's Background or Speculative flags also be appropriate?
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
@Chris: you want Jan Bambas [:mayhemer] to do the review I guess. Honza
Flags: needinfo?(odvarko)
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8834776 [details] Bug 1337672 - Part 1: Reduce the priority of prefetch network requests. https://reviewboard.mozilla.org/r/110618/#review111898
Attachment #8834776 -
Flags: review?(odvarko)
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8834777 [details] Bug 1337672 - Part 2: Reduce the priority of Flash network requests. https://reviewboard.mozilla.org/r/110620/#review111900
Attachment #8834777 -
Flags: review?(odvarko)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #3) > @Chris: you want Jan Bambas [:mayhemer] to do the review I guess. oops! Yes, thank you.
Assignee | ||
Comment 7•8 years ago
|
||
These patches will reduce the priority of network requests for <link rel="prefetch"> and Flash plugin content. Honza, if you think this is a good idea, should I use the nsISupportsPriority interface (as used for Tracking Protection requests in bug 1141814) and/or nsIClassOfService's Throttleable flag (added in bug 1312745)? Would nsIClassOfService's Background or Speculative flags also be appropriate? In my testing of my news sites linked from news.google.com, I only saw the following resources loaded using <link rel="prefetch">. None of them seem urgent. http://darkroom.baltimoresun.com/2017/01/preparing-for-the-trump-inauguration/ http://tpc.googlesyndication.com/safeframe/1-0-5/html/container.html https://a.disquscdn.com/next/embed/lounge.bundle.bc7675b3e7e1b4df8aad51f94abd7265.js https://assets.guim.co.uk/javascripts/e7ed4d0fceb8658f3ba7c1eb518ed2e1/app.js https://disqus.com/next/config.js https://ims-na1.adobelogin.com/favicon.ico?cache_bust=ddca24049ad2b8 https://securepubads.g.doubleclick.net/static/glade.js In contrast, I see Flash ad loaders on these news sites issuing nonstop requests to load new Flash ad images and XML.
Flags: needinfo?(honzab.moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
I don't know enough about the effects of network prioritization to have much opinion about this. For example, are requests for the current tab always prioritized higher than background tabs? I wouldn't want requests from the current page's Flash (perhaps including video) to be prioritized lower than any load from other tabs. From an engineering/code-readability perspective, the values `nsISupportsPriority::PRIORITY_LOW + 3` are terrible and should really be new values on nsISupportsPriority, no? But overall I'm going to defer review to mayhemer, since I lack enough context.
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8834777 [details] Bug 1337672 - Part 2: Reduce the priority of Flash network requests. https://reviewboard.mozilla.org/r/110620/#review112052
Attachment #8834777 -
Flags: review?(benjamin)
![]() |
||
Comment 12•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #0) > These patches will reduce the priority of network requests for <link > rel="prefetch"> and Flash plugin content. > > Honza, if you think this is a good idea, should I use the > nsISupportsPriority interface (as used for Tracking Protection requests in > bug 1141814) Yes > and/or nsIClassOfService's Throttleable flag (added in bug > 1312745)? I more tend to say no, but this has no affect on prioritization. It only says to simply throttle download speed (by suspending the channel periodically) for those requests. At the moment it's not clear how well it's going to work, if at all, so I don't suggest using it. > Would nsIClassOfService's Background or Speculative flags also be > appropriate? You have to ask Patrick about these two.
Flags: needinfo?(honzab.moz)
![]() |
||
Comment 13•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > I don't know enough about the effects of network prioritization to have much > opinion about this. For example, are requests for the current tab always > prioritized higher than background tabs? No, but we are working on it! > I wouldn't want requests from the > current page's Flash (perhaps including video) to be prioritized lower than > any load from other tabs. Good argument. > > From an engineering/code-readability perspective, the values > `nsISupportsPriority::PRIORITY_LOW + 3` are terrible and should really be > new values on nsISupportsPriority, no? We have LOWEST too, I think? > > But overall I'm going to defer review to mayhemer, since I lack enough > context.
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #13) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > > I don't know enough about the effects of network prioritization to have much > > opinion about this. For example, are requests for the current tab always > > prioritized higher than background tabs? > > No, but we are working on it! The code that will favor the foreground tab's requests might need a special case to identify which Flash plugin requests come from Flash content on the foreground tab. Something to keep in mind. > > From an engineering/code-readability perspective, the values > > `nsISupportsPriority::PRIORITY_LOW + 3` are terrible and should really be > > new values on nsISupportsPriority, no? > > We have LOWEST too, I think? Yes, though there are nine priority levels available between PRIORITY_LOW (10) and PRIORITY_LOWEST (20).
Updated•8 years ago
|
Whiteboard: [necko-active]
![]() |
||
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8834776 [details] Bug 1337672 - Part 1: Reduce the priority of prefetch network requests. https://reviewboard.mozilla.org/r/110618/#review112494 ::: uriloader/prefetch/nsPrefetchService.cpp:164 (Diff revision 2) > > + // Reduce the priority of prefetch network requests. > + nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(mChannel); > + if (priorityChannel) { > + priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOWEST); > + } so this class is actually used... :)
Attachment #8834776 -
Flags: review?(honzab.moz) → review+
![]() |
||
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8834777 [details] Bug 1337672 - Part 2: Reduce the priority of Flash network requests. https://reviewboard.mozilla.org/r/110620/#review112496 So, with this patch... when I go to a porn site and want to see the video I found, you will deliberately delay it for me among other advertising and what ever stuff I'm definitely not interested in? The video (on any video providing site) with autoplay is the main content of interest. This patch IMO goes exactly against delivering that with added priority. I will r- this for now. I have to think first about how to tail this in the overall CDP picture. I think for some content we want to lower the priority (ads etc) but when the content is the one of most interest, we should add priority. Question is how to recognize this soon enough. Maybe the position and size of the player element given to some simple heuristic is enough? Or... is it worth to add a priority to Flash et al in times of HTLM5 players? ::: dom/base/nsObjectLoadingContent.cpp:2624 (Diff revision 2) > } > > + // Reduce the priority of network requests to load Flash <object> elements. > + nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(chan); > + if (priorityChannel) { > + priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOW + 2); the LOW + 2 needs some comment to rationalize it. ::: dom/plugins/base/nsPluginHost.cpp:3272 (Diff revision 2) > } > + > + // Reduce the priority of network requests issued by Flash content. > + nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(channel); > + if (priorityChannel) { > + priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOW + 3); as well here (probably in relation to the above)
Attachment #8834777 -
Flags: review?(honzab.moz) → review-
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #16) > I will r- this for now. I have to think first about how to tail this in the > overall CDP picture. I think for some content we want to lower the priority > (ads etc) but when the content is the one of most interest, we should add > priority. > > Question is how to recognize this soon enough. Maybe the position and size > of the player element given to some simple heuristic is enough? I see what you mean. I will land the prefetch patch you r+'d and then close this bug for now. We can make a pretty good guess whether a Flash element is a video based on its size and aspect ratio. Chrome and Edge use similar heuristics to distinguish important from "non-essential" Flash (which they then make click-to-play). Some details about their heuristics: https://docs.google.com/document/d/1r4xFSsR4gtjBf1gOP4zHGWIFBV7WWZMgCiAHeepoHVw/ That said, we (and Chrome and Edge) plan to make most Flash content click-to-play over the next 6-12 months, so any Flash content that the user *does* load is likely to be something they want to see. So we might not need to bother adding any special logic to distinguish important from non-essential Flash. Flash video is still preferred over HTML5 video by some sites that use Adobe's DRM or analytics, such as HBO GO, Hulu, and CNN.
Comment 18•8 years ago
|
||
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8da87f54011 Part 1: Reduce the priority of prefetch network requests. r=mayhemer
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8da87f54011
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•8 years ago
|
Summary: Reduce the priority of network requests for <link rel="prefetch"> and Flash plugin content → Reduce the priority of network requests for <link rel="prefetch">
You need to log in
before you can comment on or make changes to this bug.
Description
•