1351281 - Stop adjusting network priorities in front-end code

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Honza Bambas (:mayhemer)]

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

Comment 1

[Honza Bambas (:mayhemer)]

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).

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1 (comment out the event hook)

Just a pure one line disable.  Bug to remove this file eventually already filed: 1354179.

Comment 3

[Mike Conley (: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

Comment 4

[Honza Bambas (:mayhemer)]

(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.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8855354 [details] [diff] [review]
v1 (comment out the event hook)

Cool - clearing review request until the new patch is posted.

Comment 6

[Dão Gottwald [:dao]]

(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.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Disable NetworkPrioritizer.jsm

Summary:
 - As comment #3 said, just remove the code where NetworkPrioritizer.jsm is imported.

Thanks.

Comment 8

[Dão Gottwald [:dao]]

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)?

Comment 9

[Kershaw Chang [:kershaw]]

(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

Comment 10

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Dão Gottwald [:dao]]

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.

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Remove NetworkPrioritizer.jsm completely

Summary:
 - Remove NetworkPrioritizer.jsm
 - Remove adjustPriority/setPriority method

Comment 14

[Dão Gottwald [:dao]]

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

Comment 15

[Kershaw Chang [:kershaw]]

(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!

Comment 16

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aefd85cd6996693c3465b229fc3e78a253f1481

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Remove NetworkPrioritizer.jsm completely, r=dao

Carry reviewer's name.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a07a06250be
Remove NetworkPrioritizer.jsm, r=dao

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a07a06250be