Closed Bug 1351281 Opened 3 years ago Closed 3 years ago

Stop adjusting network priorities in front-end code

Categories

(Firefox :: Tabbed Browser, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: mayhemer, Assigned: kershaw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

As we have landed bug 1312782, there is no longer a need for adjusting priorities here.  It's actually counter productive right now.

The goal of this bug is to remove increasePriority and decreasePriority methods usage on active tab change, see [1].  If unused altogether, remove those methods too.

[1] https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/browser/modules/NetworkPrioritizer.jsm#170
Priority: -- → P1
I've decided to just disable this functionality for now and leave the code in.  We may potentially find it useful later when finding priorities among the inactive tabs and when we want to prioritize among windows (not just tabs).
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Summary: Remove priority adjustments from NetworkPrioritizer.jsm when active tab is changing → Disable priority adjustments from NetworkPrioritizer.jsm when active tab is changing
Attached patch v1 (comment out the event hook) (obsolete) — Splinter Review
Just a pure one line disable.  Bug to remove this file eventually already filed: 1354179.
Attachment #8855354 - Flags: review?(mconley)
I'm confused. If, as you say in bug 1354179, the goal is to remove NetworkPrioritizer.jsm, why are we commenting this out, and not just removing these lines?:

http://searchfox.org/mozilla-central/rev/78cefe75fb43195e7f5aee1d8042b8d8fc79fc70/browser/base/content/browser.js#1452-1454
Flags: needinfo?(honzab.moz)
(In reply to Mike Conley (:mconley) from comment #3)
> I'm confused. If, as you say in bug 1354179, the goal is to remove
> NetworkPrioritizer.jsm, why are we commenting this out, and not just
> removing these lines?:
> 
> http://searchfox.org/mozilla-central/rev/
> 78cefe75fb43195e7f5aee1d8042b8d8fc79fc70/browser/base/content/browser.
> js#1452-1454

Because I was honestly lazy to look for exactly this spot!  Yes, let's just remove these lines.  Thanks.  Will update the patch.
Flags: needinfo?(honzab.moz)
Comment on attachment 8855354 [details] [diff] [review]
v1 (comment out the event hook)

Cool - clearing review request until the new patch is posted.
Attachment #8855354 - Flags: review?(mconley)
(In reply to Honza Bambas (:mayhemer) from comment #1)
> when we want to prioritize among windows (not just tabs).

NetworkPrioritizer.jsm is already doing this.
Blocks: 1354179
Assignee: honzab.moz → kechang
Attached patch Disable NetworkPrioritizer.jsm (obsolete) — Splinter Review
Summary:
 - As comment #3 said, just remove the code where NetworkPrioritizer.jsm is imported.

Thanks.
Attachment #8855354 - Attachment is obsolete: true
Attachment #8863645 - Flags: review?(mconley)
Comment on attachment 8863645 [details] [diff] [review]
Disable NetworkPrioritizer.jsm

We should remove the module and the test if we don't need them anymore. That's however not clear to me. Does Necko differentiate between foreground and background windows (as opposed to tabs)?
Attachment #8863645 - Flags: review?(mconley) → review-
(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 8863645 [details] [diff] [review]
> Disable NetworkPrioritizer.jsm
> 
> We should remove the module and the test if we don't need them anymore.

I'm getting confused about this bug and bug 1354179. I thought we want to disable NetworkPrioritizer.jsm first, and remove it entirely later.
Or we should just remove NetworkPrioritizer.jsm in this bug?

Honza, what do you think? Is there any reason we want to separate things into two bugs?


> That's however not clear to me. Does Necko differentiate between foreground
> and background windows (as opposed to tabs)?

Yes, we have current top level outer content window id stored in Necko [1].

[1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/netwerk/protocol/http/nsHttpConnectionMgr.h#619
Flags: needinfo?(honzab.moz)
I wanted to preserve the code in case we may want to use it for something else than prioritization.  But it will be in history and we can always revert or get inspired with it.  

So, remove it completely.  I really don't care personally, tho.  The goal is to stop the code from doing its work at the first place.
Flags: needinfo?(honzab.moz)
No longer blocks: 1354179
Duplicate of this bug: 1354179
I think this will also make the browser binding's setPriority and adjustPriority methods unused. We should probably remove these too if Necko doesn't want front-end code to mess with this stuff.
Summary:
 - Remove NetworkPrioritizer.jsm
 - Remove adjustPriority/setPriority method
Attachment #8863645 - Attachment is obsolete: true
Attachment #8863973 - Flags: review?(dao+bmo)
Comment on attachment 8863973 [details] [diff] [review]
Remove NetworkPrioritizer.jsm completely

Please remove adjustPriority here too:

http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/browser/base/content/tabbrowser.xml#2043
Attachment #8863973 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #14)
> Comment on attachment 8863973 [details] [diff] [review]
> Remove NetworkPrioritizer.jsm completely
> 
> Please remove adjustPriority here too:
> 
> http://searchfox.org/mozilla-central/rev/
> ae8c2e2354db652950fe0ec16983360c21857f2a/browser/base/content/tabbrowser.
> xml#2043

Thanks for catching this!
Summary: Disable priority adjustments from NetworkPrioritizer.jsm when active tab is changing → Stop adjusting network priorities in front-end code
Carry reviewer's name.
Attachment #8863973 - Attachment is obsolete: true
Attachment #8864059 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7a07a06250be
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 618193
Blocks: 1320261
Blocks: 590016
Blocks: 1162693
Blocks: 660321
See Also: → 1366822
You need to log in before you can comment on or make changes to this bug.