Mark top level loading channels as urgent-start

RESOLVED FIXED in Firefox 55

Status

()

Core
Document Navigation
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mayhemer, Assigned: kershaw)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
This means channel created by nsDocShell::DoURILoad.
(Reporter)

Updated

a year ago
Blocks: 1312741
Priority: -- → P2
(Reporter)

Comment 1

a year ago
Interested in hearing about conflicts with the P field...
Priority: P2 → P1
(Assignee)

Comment 2

a year ago
I'd like to work on this one.
Assignee: nobody → kechang
(Assignee)

Comment 3

a year ago
(In reply to Honza Bambas (:mayhemer) from comment #0)
> This means channel created by nsDocShell::DoURILoad.

The channels created by nsDocShell::DoURILoad also include iframe documents.
I think we only need to mark urgent-start for the top level document?
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 4

a year ago
(In reply to Kershaw Chang [:kershaw] from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> > This means channel created by nsDocShell::DoURILoad.
> 
> The channels created by nsDocShell::DoURILoad also include iframe documents.
> I think we only need to mark urgent-start for the top level document?

Yes!  There should be examples in nsDocShell how to recognize it.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 5

a year ago
Created attachment 8857295 [details] [diff] [review]
Mark the channel as UrgentStart for top level document loading

Summary:
 - In |nsDocShell::DoURILoad|, mark the channel as UrgentStart if |isTopLevelDoc| is true.

Thanks.
Attachment #8857295 - Flags: review?(bugs)
Comment on attachment 8857295 [details] [diff] [review]
Mark the channel as UrgentStart for top level document loading

Honza, do we want this behavior for all the top level pages, or only top level foreground pages?

It is unclear to me why this should happen with background tabs.
Flags: needinfo?(honzab.moz)
Attachment #8857295 - Flags: review?(bugs) → feedback?(honzab.moz)
(Reporter)

Comment 7

a year ago
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8857295 [details] [diff] [review]
> Mark the channel as UrgentStart for top level document loading
> 
> Honza, do we want this behavior for all the top level pages, or only top
> level foreground pages?
> 
> It is unclear to me why this should happen with background tabs.

Hmm.. I think we may allow this for background tabs as well to some extent (for tabs user opens by middle-clicking a link from the foreground page).  We should not if that is a reload by a timeout script or a <meta "refresh"> - i.e. not a user interaction.

There will never be a big number of urgent-start marked requests.  And since those are going out ASAP, they will probably also be done and gone ASAP.

OTOH, if you do a reload of all tabs, then definitely urgent-start has to be set *only* for the foreground tab.  There are limits in the necko code, but I think we should let them free for what is probably going to happen on the foreground tab soon.

To sum:

when                        do urgent start

left click                  yes
middle click                yes
reload active tab           yes
reload all                  yes, but only for the active tab
any background tab reload   no


To write it in a pseudo code:

if (!reload || isForeground)
   set urgent-start
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 8

a year ago
Comment on attachment 8857295 [details] [diff] [review]
Mark the channel as UrgentStart for top level document loading

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

This is a very good start.  Let's add some more conditions as I outlined in comment 7.

Whether a docshell is active is easy to find out with the mIsActive flag.  It changes when you switch tabs.

The 'reload' trigger can be found from mLoadType.  All types are listed at [1] and I think those having the LOAD_CMD_REALOAD type [2] are enough to indicate it's a reload.

Hence the condition may look like if (mIsActive || !(mLoadType & LOAD_CMD_REALOAD)) -> set urgent-start if top-level.


[1] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/docshell/base/nsDocShellLoadTypes.h#40
[2] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/docshell/base/nsDocShellLoadTypes.h#50-54,57
Attachment #8857295 - Flags: feedback?(honzab.moz) → feedback+
(Assignee)

Comment 9

a year ago
Created attachment 8861777 [details] [diff] [review]
Mark the channel as UrgentStart for top level document loading

Summary:
 - Add conditions as Honza said.
 - Besides LOAD_CMD_RELOAD, I think we don't want to mark for LOAD_CMD_HISTORY and LOAD_CMD_PUSHSTATE, so I only check if |mLoadType| has LOAD_CMD_NORMAL in this patch.

Thanks.
Attachment #8857295 - Attachment is obsolete: true
Attachment #8861777 - Flags: review?(bugs)
Comment on attachment 8861777 [details] [diff] [review]
Mark the channel as UrgentStart for top level document loading

>+  // Mark the http channel as UrgentStart for top level document loading
>+  // in active tab.
>+  if (mIsActive || (mLoadType & LOAD_CMD_NORMAL)) {
>+    if (httpChannel && isTopLevelDoc) {
>+      nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
>+      if (cos) {
>+        cos->AddClassFlags(nsIClassOfService::UrgentStart);
>+      }
>+    }
>+  }
I think we do want history loads to be urgent too. Even more urgent than anything else. It is after all back-forward.
So perhaps check also mLoadType & LOAD_CMD_HISTORY
Attachment #8861777 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

a year ago
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8861777 [details] [diff] [review]
> Mark the channel as UrgentStart for top level document loading
> 
> >+  // Mark the http channel as UrgentStart for top level document loading
> >+  // in active tab.
> >+  if (mIsActive || (mLoadType & LOAD_CMD_NORMAL)) {
> >+    if (httpChannel && isTopLevelDoc) {
> >+      nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
> >+      if (cos) {
> >+        cos->AddClassFlags(nsIClassOfService::UrgentStart);
> >+      }
> >+    }
> >+  }
> I think we do want history loads to be urgent too. Even more urgent than
> anything else. It is after all back-forward.
> So perhaps check also mLoadType & LOAD_CMD_HISTORY

Thanks for the review. I'll change the condition.
(Assignee)

Comment 12

a year ago
Created attachment 8861877 [details] [diff] [review]
Mark the channel as UrgentStart for top level document loading, r=smaug

Summary:
 - Also check mLoadType & LOAD_CMD_HISTORY
 - Carry reviewer's name

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2423aa9a19a7cd39ee012ded409e5ccd0265e5e
Attachment #8861777 - Attachment is obsolete: true
Attachment #8861877 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 13

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9323cfe9cd8e
Mark the channel as UrgentStart for top level document loading. r=smaug
Keywords: checkin-needed

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9323cfe9cd8e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.