1348044 - Mark top level loading channels as urgent-start

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P1)

Description

[Honza Bambas (:mayhemer)]

This means channel created by nsDocShell::DoURILoad.

Comment 1

[Honza Bambas (:mayhemer)]

Interested in hearing about conflicts with the P field...

Comment 2

[Kershaw Chang [:kershaw]]

I'd like to work on this one.

Comment 3

[Kershaw Chang [:kershaw]]

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

Comment 4

[Honza Bambas (:mayhemer)]

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

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Mark the channel as UrgentStart for top level document loading

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

Thanks.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Honza Bambas (:mayhemer)]

(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

Comment 8

[Honza Bambas (:mayhemer)]

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

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: 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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[Kershaw Chang [:kershaw]]

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

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: 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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9323cfe9cd8e
Mark the channel as UrgentStart for top level document loading. r=smaug

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/9323cfe9cd8e