Make it possible to check if a Window belongs to a pinned tab

RESOLVED WONTFIX

Status

()

Core
DOM
P1
normal
RESOLVED WONTFIX
5 months ago
4 months ago

People

(Reporter: farre, Assigned: Away for a while)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

5 months ago
Blocks: 1377766
(Assignee)

Comment 1

5 months ago
Turns out docshell already has a boolean flag indicating whether it is inside a pinned tab: isAppTab: https://searchfox.org/mozilla-central/rev/238406d4c1b3f147522ce0a45a4c6f84a8115781/docshell/base/nsIDocShell.idl#682
(Assignee)

Comment 2

5 months ago
Created attachment 8884476 [details] [diff] [review]
Exempt pinned tabs from budget based background timeout throttling

The feature currently doesn't have any tests so I only tested this manually.  I decided to query the attribute manually because the queries should be all relatively cheap and infrequent, and that way we can handle tabs going in and out of pinned mode for free.
Attachment #8884476 - Flags: review?(bkelly)
(Assignee)

Comment 3

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae90a8a36e872e4e2c19d38db87d57fc65c5c6b9

Comment 4

5 months ago
Why do we want to do this? Do we expect pinned tabs to use significant amount processing time?
I'm worried that we end up throttling less than what we do currently.
(Reporter)

Comment 5

5 months ago
With this patch we'd only skip budget throttling, not all throttling.

Comment 6

5 months ago
Comment on attachment 8884476 [details] [diff] [review]
Exempt pinned tabs from budget based background timeout throttling

Review of attachment 8884476 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable assuming docshell "AppTab" is indeed pinned tab.  I did not double-check that.

To answer Olli's question perhaps we should make this a pref we can then run an experiment on.

We also really need tests for this and the other budget throttling work.

::: dom/base/TimeoutManager.cpp
@@ +1235,5 @@
>  
> +bool
> +TimeoutManager::BudgetThrottlingEnabled() const
> +{
> +  if (mBudgetThrottleTimeouts) {

Can you avoid the extra nesting by using short-circuit logic like:

  if (!mBudgetThrottleTimeouts) {
    return false;
  }

  nsIDocShell* docShell = mWindow.GetDocShell();
  if (!docShell) {
    return true;
  }

  // etc
Attachment #8884476 - Flags: review?(bkelly) → review+
(Reporter)

Comment 7

5 months ago
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #6)
> Comment on attachment 8884476 [details] [diff] [review]
> Exempt pinned tabs from budget based background timeout throttling
> 
> Review of attachment 8884476 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> To answer Olli's question perhaps we should make this a pref we can then run
> an experiment on.

We could include this in the manual testing as well. 

> 
> We also really need tests for this and the other budget throttling work.

Yep, we have Bug 1378402 to track adding tests.
Assignee: nobody → ehsan
Priority: -- → P1
(Assignee)

Comment 8

4 months ago
So, I tested Chrome 59's treatment of background pinned tabs and they do not exempt those tabs from budget based throttling.  I was under the impression that this bug is filed for compatibility with what Chrome does, but having realized that fixing this will actually make us incompatible with them, I no longer think this is a good idea.  Sorry for not testing before.

I'm gonna WONTFIX because of that reason.  If someone wants to reopen they should probably start to first convince the Chrome team that they should do the same.  I certainly don't think we should be implementing different heuristics here, especially if what they have has turned out Web compatible for them.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.