Closed Bug 1319359 Opened 4 years ago Closed 4 years ago

Not to block the throbber if the active requests in load group are all tracking requests

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox53 --- affected

People

(Reporter: kershaw, Assigned: kershaw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(3 files)

See bug 1141814 comment #21.

This bug is aimed to modify a channel's load flags if the channel is loading a tracker.
Assignee: nobody → kechang
Blocks: CDP
Whiteboard: [necko-active]
According to bug 1124971 comment#11, it seems that we don't want to set LOAD_BACKGROUND in the parent after AsyncOpen has been called.
However, this bug may really need to do this in the parent. Or, should I not touch the mLoadFlags in the parent and only change HttpChannelChild's mLoadFlags?

Since the information in bug 1124971 is quite long time ago, I am not sure it's still valid now.
Jason, could you comment on this?
Flags: needinfo?(jduell.mcbugs)
Marking the tracking channel as LOAD_BACKGROUND not only affect the throbber but also move the timing for window load event earlier, since it changes |mForegroundCount| in the document's load group.
If we mark those tracking channels as LOAD_BACKGROUND, it is possible that those resources are still loading after the load event is fired. This could break some web sites since the load event means that all resources in the document are finished loading.

smaug, could you provide your comment from DOM point of view? Is it safe to do this? Thanks.
Flags: needinfo?(bugs)
Ehsan has been looking into this kind of stuff recently, well, de-prioritizing tracker JS.

But anyhow, making resources which are expected to be loaded before load event load later would pretty much guarantee to break pages. I don't think we can do changes there.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #3)
> Ehsan has been looking into this kind of stuff recently, well,
> de-prioritizing tracker JS.
> 
> But anyhow, making resources which are expected to be loaded before load
> event load later would pretty much guarantee to break pages. I don't think
> we can do changes there.

Thanks for the information.

Honza, based on smaug's comment, I think we should not set LOAD_BACKGROUND on those tracking channels.
What do you think?
Flags: needinfo?(honzab.moz)
Yep, let's WONTFIX this.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(honzab.moz)
Resolution: --- → WONTFIX
Clear the needinfo flag since this bug is resolved.
Flags: needinfo?(jduell.mcbugs)
We could split up LOAD_BACKGROUND into two different flags if we want tracking URIs to not block the throbber (but still block the load event).

That's assuming we want that behavior... (and we're willing to waste a precious flag bit on it).
FWIW, I like that idea. We could also use that flag when loading some iframes (ads) etc
we need to talk about this more.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
is the point of this bug

a] lowering the relative load priority of trackers?

or

b] just not linking tp to the throbber (and other things)?

We definitely should still be looking at a - i can't lay my hands on the bug for that. Is it this one or am I just missing it in the parent bug's deps?

If its b then we should just morph this bug into that goal rather than going for a new bug.
(In reply to Patrick McManus [:mcmanus] from comment #10)
> is the point of this bug
> 
> a] lowering the relative load priority of trackers?
> 

This is already done in bug 1141814.

> or
> 
> b] just not linking tp to the throbber (and other things)?
> 
> We definitely should still be looking at a - i can't lay my hands on the bug
> for that. Is it this one or am I just missing it in the parent bug's deps?
> 
> If its b then we should just morph this bug into that goal rather than going
> for a new bug.

Sure. I'll work on this bug based on comment #7.
Another thing which has been suggested somewhere is to not activate throbber when after top level page's load some iframes are being loaded, whether or not those iframes are from tp.
Summary: Mark HTTP channel as LOAD_BACK_GROUND if the resource load by the channel is a tracker → Not to block the throbber if the active requests in load group are all tracking requests
Hi Timothy,

This patch is about getting the image request proxy from the http channel.
Could you please take a look at this patch?

Thanks.
Attachment #8819844 - Flags: review?(tnikkel)
Summary:
- Add new load flag: LOAD_TRACKING_CONTENT
- Keep the count of tracking channels and document onload blocker in load group
- Mark the load flags with LOAD_TRACKING_CONTENT
Attachment #8819848 - Flags: review?(honzab.moz)
Summary:
- Add nsIOnloadBlocker for tracking the count in load group
- Add STATE_DELAY_LOAD_TRACKING_CONTENT in nsIWebProgressListener
- Check if all remaining requests in the load group are all tracking requests
Attachment #8819850 - Flags: review?(bugs)
Could you describe what behavior the patches give? Do we end up stopping throbber before load event has fired? Or what?
Comment on attachment 8819850 [details] [diff] [review]
Part3: Add STATE_DELAY_LOAD_TRACKING_CONTENT

I don't understand the setup here.
What is nsIOnloadBlocker for?
nsOnloadBlocker has been sort of just an optimization to put some dummy request to loadgroup to prevent load event to fire when document has still mOnloadBlockCount > 0.

I don't understand what nsILoadGroup.loadBlockerCount from part 2 means.
Before these patches any non-background nsIRequest has been a "load blocker".
What is the use case for nsILoadGroup.loadBlockerCount?
Attachment #8819850 - Flags: review?(bugs) → review-
Comment on attachment 8819844 [details] [diff] [review]
Part1: Find a way to get image request proxy

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

::: image/imgRequest.cpp
@@ +1169,5 @@
>  imgRequest::GetInterface(const nsIID & aIID, void** aResult)
>  {
> +  if (mFirstProxy && aIID.Equals(NS_GET_IID(imgIRequest))) {
> +    *aResult = mFirstProxy;
> +    NS_ADDREF((nsISupports*)*aResult);

this all sounds really wrong to me :)
Comment on attachment 8819848 [details] [diff] [review]
Part2: Calculate the tracking request count in load group

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

This whole machinery sounds wrong to me or is badly communicated.  I have to look at this deeper, probably after the new year.  

In the mean time, I'd like to have a clear definition of what we are trying to do here, why, and what's the expected benefit.  

There is also mentioning of Ehsan's work on deprio of tracking js.  What is the bug #(s) and how this but relates to it?

::: netwerk/base/nsLoadGroup.cpp
@@ +636,5 @@
>      nsLoadFlags flags;
>      rv = request->GetLoadFlags(&flags);
>      if (NS_FAILED(rv)) return rv;
>  
> +    if (flags & nsIRequest::LOAD_TRACKING_CONTENT) {

hmm.. and if this flag is removed during the channel's lifetime then what?

or, what if the flag is set after the channel has already been added to this loadgroup?  I don't think we can let this code in, it's super-fragile.
(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8819850 [details] [diff] [review]
> Part3: Add STATE_DELAY_LOAD_TRACKING_CONTENT
> 
> I don't understand the setup here.

What I am trying to do here is stopping the throbber before firing the load event. The idea is quite straightforward: If all foreground requests in the load group are tracking requests, notify the tab to stop the throbber. 

My original idea is to check whether the foreground request count is equal to tracking request count. However, as you said, nsOnloadBlocker is to prevent the load event to be fired. So, nsOnloadBlocker is always in the load group until the tracking requests are finished. In order to stop the throbber before load event, I have to not count nsOnloadBlocker in foreground request count.

> What is nsIOnloadBlocker for?

It's just a dummy interface to indicate the request is a nsOnloadBlocker.

> nsOnloadBlocker has been sort of just an optimization to put some dummy
> request to loadgroup to prevent load event to fire when document has still
> mOnloadBlockCount > 0.
> 
> I don't understand what nsILoadGroup.loadBlockerCount from part 2 means.
> Before these patches any non-background nsIRequest has been a "load blocker".
> What is the use case for nsILoadGroup.loadBlockerCount?
(In reply to Honza Bambas (:mayhemer) from comment #18)
> Comment on attachment 8819844 [details] [diff] [review]
> Part1: Find a way to get image request proxy
> 
> Review of attachment 8819844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgRequest.cpp
> @@ +1169,5 @@
> >  imgRequest::GetInterface(const nsIID & aIID, void** aResult)
> >  {
> > +  if (mFirstProxy && aIID.Equals(NS_GET_IID(imgIRequest))) {
> > +    *aResult = mFirstProxy;
> > +    NS_ADDREF((nsISupports*)*aResult);
> 
> this all sounds really wrong to me :)

Due to the special behavior of the image request's load group [1][2], I have to find a way to get the image request proxy and its load group.
I know it's a weird approach. I'll figure out a better approach.

[1] http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/image/imgLoader.cpp#833-843
[2] http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/image/imgLoader.cpp#2269
Attachment #8819844 - Flags: review?(tnikkel)
(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8819848 [details] [diff] [review]
> Part2: Calculate the tracking request count in load group
> 
> Review of attachment 8819848 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This whole machinery sounds wrong to me or is badly communicated.  I have to
> look at this deeper, probably after the new year.  
> 
> In the mean time, I'd like to have a clear definition of what we are trying
> to do here, why, and what's the expected benefit.  
> 

My idea is to count how many requests in a load group are tracking requests. If there are only tracking requests in the load group, then stop the throbber.
Not sure if this idea is matched to your expectation. Or do I misunderstand something?

> There is also mentioning of Ehsan's work on deprio of tracking js.  What is
> the bug #(s) and how this but relates to it?
> 

I'll figure out.

> ::: netwerk/base/nsLoadGroup.cpp
> @@ +636,5 @@
> >      nsLoadFlags flags;
> >      rv = request->GetLoadFlags(&flags);
> >      if (NS_FAILED(rv)) return rv;
> >  
> > +    if (flags & nsIRequest::LOAD_TRACKING_CONTENT) {
> 
> hmm.. and if this flag is removed during the channel's lifetime then what?
> 
> or, what if the flag is set after the channel has already been added to this
> loadgroup?  I don't think we can let this code in, it's super-fragile.

I don't understand your concern here. The same concern could also apply to LOAD_BACKGROUND. If LOAD_BACKGROUND is set after the channel has already been added in load group, |mForegroundCount| could be messed.
So, the only correct way to set LOAD_TRACKING_CONTENT is like LOAD_BACKGROUND: always remove the request from the load group, modify the load flags, and then add it back.
Will answer tomorrow
Flags: needinfo?(honzab.moz)
(In reply to Kershaw Chang [:kershaw] from comment #21)
> > this all sounds really wrong to me :)
> 
> Due to the special behavior of the image request's load group [1][2], I have
> to find a way to get the image request proxy and its load group.
> I know it's a weird approach. I'll figure out a better approach.

I know.  The code for it is bad.  Doing such a raw cast on a void ptr is.. at least very bad.  If you fix this, then it may work.

Still, I'd rather expose it somehow differently, e.g. have a new interface for it that imgRequest would impl.  Or keep the proxy not as a weak void* but as a proper refptr or something to be able to query it (as you do) but properly.  But that's a big change.

Simply said - something smells bad on how you wrote it.  But I understand the purpose, tho, would like to find some different way other than be so specific to imgrequests (but yeah, imglib sucks at this for years...)
(In reply to Kershaw Chang [:kershaw] from comment #20)
> (In reply to Olli Pettay [:smaug] from comment #17)
> > I don't understand what nsILoadGroup.loadBlockerCount from part 2 means.
> > Before these patches any non-background nsIRequest has been a "load blocker".
> > What is the use case for nsILoadGroup.loadBlockerCount?


So what is the use case for nsILoadGroup.loadBlockerCount? Load blocker isn't really any different to other nsIRequests.

In general it might be bad to stop throbber too early. Often it is load event listener which initializes the page to be ready to accept user input etc. But this is something to try out, I guess.
(In reply to Kershaw Chang [:kershaw] from comment #22)
> My idea is to count how many requests in a load group are tracking requests.
> If there are only tracking requests in the load group, then stop the
> throbber.
> Not sure if this idea is matched to your expectation. Or do I misunderstand
> something?

So, is what you are trying to do to only to stop that little spinning wheel, which is hardly noticeable, a bit sooner?  Or will this have any effect on the actual painting and prioritization of resources on the page?  If you do this all only for the former, then I'm not sure it's worth at all (WONTFIX for me).

> > > +    if (flags & nsIRequest::LOAD_TRACKING_CONTENT) {
> > 
> > hmm.. and if this flag is removed during the channel's lifetime then what?
> > 
> > or, what if the flag is set after the channel has already been added to this
> > loadgroup?  I don't think we can let this code in, it's super-fragile.
> 
> I don't understand your concern here. The same concern could also apply to
> LOAD_BACKGROUND. If LOAD_BACKGROUND is set after the channel has already
> been added in load group, |mForegroundCount| could be messed.
> So, the only correct way to set LOAD_TRACKING_CONTENT is like
> LOAD_BACKGROUND: always remove the request from the load group, modify the
> load flags, and then add it back.

The problem may be that the flag may change (be set or dropped) between when you add and remove the request to/from the load group.  It's similar to when you change the LOAD_BACKGROUND flag - to do it properly, you must first remove the request from the load group (may have consequences, BTW!), change the flag, re-add the request to the load group.  I don't see that happening in your patch.  And my main concern is that when someone else somewhere resets the flags to whatever value while the request in its loadgroup, we won't do this update (remove/readd).  So, your counters and arrays will break.  I'd really like to find a different solution or architecture here if possible.

There is also a readonly property added to nsIHttpChannel in bug 1170190 (part 2 patch).  That one should be used here.  And IMO put on nsIChannel or even nsIRequest to allow loadgroup to work with it.  But not every request is a tracker... so some controversy arises here, and also it would be a lot of work to update every implementer of nsIRequest...  My guts tell me this is not the best approach as a whole...
Flags: needinfo?(honzab.moz) → needinfo?(kechang)
(In reply to Honza Bambas (:mayhemer) from comment #26)
> So, is what you are trying to do to only to stop that little spinning wheel,
> which is hardly noticeable, a bit sooner?
It is very much noticeable. And better to ask UX devs about this.
(In reply to Olli Pettay [:smaug] from comment #27)
> (In reply to Honza Bambas (:mayhemer) from comment #26)
> > So, is what you are trying to do to only to stop that little spinning wheel,
> > which is hardly noticeable, a bit sooner?
> It is very much noticeable. And better to ask UX devs about this.

As I'm watching people interacting with Fx, they usually don't even look at it.  But that's a layman opinion.
(In reply to Olli Pettay [:smaug] from comment #25)
> (In reply to Kershaw Chang [:kershaw] from comment #20)
> > (In reply to Olli Pettay [:smaug] from comment #17)
> > > I don't understand what nsILoadGroup.loadBlockerCount from part 2 means.
> > > Before these patches any non-background nsIRequest has been a "load blocker".
> > > What is the use case for nsILoadGroup.loadBlockerCount?
> 
> 
> So what is the use case for nsILoadGroup.loadBlockerCount? Load blocker
> isn't really any different to other nsIRequests.
> 

For example, given a load group which contains some requests as below.

LoadGroup - mForegroundCount = 4, trackingRequestCount = 2
 - evil.js
 - evil.css
 - good.js
 - load blocker

After the loading of good.js is finished and it has been removed from the load group.

LoadGroup - mForegroundCount = 3, trackingRequestCount = 2
 - evil.js
 - evil.css
 - load blocker

At this point, the throbber should be stooped. However, the |mForegroundCount| is still bigger than |trackingRequestCount| because the load blocker is still in the load group until evil.js and evil.css are done. If I don't know how many load blockers are in the load group (though it seems there is always 1 load blocker), I never know when to stop the throbber.

With the load blocker being counted, I know that:
LoadGroup - mForegroundCount = 3, trackingRequestCount = 2, loadBlockerCount = 1

Right now, I know that foreground requests consist of only 2 tracking requests and 1 load blocker, so it is safe to stop the throbber.


> In general it might be bad to stop throbber too early. Often it is load
> event listener which initializes the page to be ready to accept user input
> etc. But this is something to try out, I guess.
> > > > +    if (flags & nsIRequest::LOAD_TRACKING_CONTENT) {
> > > 
> > > hmm.. and if this flag is removed during the channel's lifetime then what?
> > > 
> > > or, what if the flag is set after the channel has already been added to this
> > > loadgroup?  I don't think we can let this code in, it's super-fragile.
> > 
> > I don't understand your concern here. The same concern could also apply to
> > LOAD_BACKGROUND. If LOAD_BACKGROUND is set after the channel has already
> > been added in load group, |mForegroundCount| could be messed.
> > So, the only correct way to set LOAD_TRACKING_CONTENT is like
> > LOAD_BACKGROUND: always remove the request from the load group, modify the
> > load flags, and then add it back.
> 
> The problem may be that the flag may change (be set or dropped) between when
> you add and remove the request to/from the load group.  It's similar to when
> you change the LOAD_BACKGROUND flag - to do it properly, you must first
> remove the request from the load group (may have consequences, BTW!), change
> the flag, re-add the request to the load group.  I don't see that happening
> in your patch.

I did this (remove from load group, change flags, re-add) in the function ModifyRequestHelper in my patch.
 
> And my main concern is that when someone else somewhere
> resets the flags to whatever value while the request in its loadgroup, we
> won't do this update (remove/readd).  So, your counters and arrays will
> break.  I'd really like to find a different solution or architecture here if
> possible.
> 
Understand. I'll figure out a safe way to do this.

> There is also a readonly property added to nsIHttpChannel in bug 1170190
> (part 2 patch).  That one should be used here.  And IMO put on nsIChannel or
> even nsIRequest to allow loadgroup to work with it.  But not every request
> is a tracker... so some controversy arises here, and also it would be a lot
> of work to update every implementer of nsIRequest...  My guts tell me this
> is not the best approach as a whole...

From my layman's perspective, it would be better to just spend a precious load flag bit to mark a channel loading a tracking resource.
Flags: needinfo?(kechang)
(In reply to Honza Bambas (:mayhemer) from comment #26)
> (In reply to Kershaw Chang [:kershaw] from comment #22)
> > My idea is to count how many requests in a load group are tracking requests.
> > If there are only tracking requests in the load group, then stop the
> > throbber.
> > Not sure if this idea is matched to your expectation. Or do I misunderstand
> > something?
> 
> So, is what you are trying to do to only to stop that little spinning wheel,
> which is hardly noticeable, a bit sooner?  Or will this have any effect on
> the actual painting and prioritization of resources on the page?  If you do
> this all only for the former, then I'm not sure it's worth at all (WONTFIX
> for me).
> 

My idea is to only fire an state change event with STATE_DELAY_LOAD_TRACKING_CONTENT. The event listener can then decide only to stop the little spinning wheel or do something notably.
(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Olli Pettay [:smaug] from comment #27)
> > (In reply to Honza Bambas (:mayhemer) from comment #26)
> > > So, is what you are trying to do to only to stop that little spinning wheel,
> > > which is hardly noticeable, a bit sooner?
> > It is very much noticeable. And better to ask UX devs about this.
> 
> As I'm watching people interacting with Fx, they usually don't even look at
> it.  But that's a layman opinion.

First things first, I would like to figure out the goal here.

smaug, could you suggest the correct UX developer to ask about what they think about the throbber?

Patrick, since you are the one who reopened this bug, could you explain more about the expected behavior in your mind? Is it worth to stop the throbber a bit sooner? Thanks.
Flags: needinfo?(mcmanus)
Flags: needinfo?(bugs)
(In reply to Kershaw Chang [:kershaw] from comment #29) 
> For example, given a load group which contains some requests as below.
> 
> LoadGroup - mForegroundCount = 4, trackingRequestCount = 2
>  - evil.js
>  - evil.css
>  - good.js
>  - load blocker
> 
> After the loading of good.js is finished and it has been removed from the
> load group.
> 
> LoadGroup - mForegroundCount = 3, trackingRequestCount = 2
>  - evil.js
>  - evil.css
>  - load blocker
> 
> At this point, the throbber should be stooped. However, the
> |mForegroundCount| is still bigger than |trackingRequestCount| because the
> load blocker is still in the load group until evil.js and evil.css are done.
> If I don't know how many load blockers are in the load group (though it
> seems there is always 1 load blocker), I never know when to stop the
> throbber.
> 
> With the load blocker being counted, I know that:
> LoadGroup - mForegroundCount = 3, trackingRequestCount = 2, loadBlockerCount
> = 1
> 
> Right now, I know that foreground requests consist of only 2 tracking
> requests and 1 load blocker, so it is safe to stop the throbber.
> 

How you know that? There is just one LoadBlocker per document, but load blocking count is counted internally 
in nsDocument
http://searchfox.org/mozilla-central/rev/46ded325e8f60b3d4c731b27c31d383911056a38/dom/base/nsDocument.h#1535-1536


And perhaps ask bpitoyo for ux help. If he is not the right person, hopefully he can say who is.
Flags: needinfo?(bugs)
you should double check with honza - this was really his goal.. but yes, the goal was to minimize (eliminate?) the throbber dependency on TP data.. the bug got closed because the first approach to it was bogus, I reopened the bug because that didn't invalidate the goal (just the approach).
Flags: needinfo?(mcmanus)
(In reply to Olli Pettay [:smaug] from comment #33)
> (In reply to Kershaw Chang [:kershaw] from comment #29) 
> > For example, given a load group which contains some requests as below.
> > 
> > LoadGroup - mForegroundCount = 4, trackingRequestCount = 2
> >  - evil.js
> >  - evil.css
> >  - good.js
> >  - load blocker
> > 
> > After the loading of good.js is finished and it has been removed from the
> > load group.
> > 
> > LoadGroup - mForegroundCount = 3, trackingRequestCount = 2
> >  - evil.js
> >  - evil.css
> >  - load blocker
> > 
> > At this point, the throbber should be stooped. However, the
> > |mForegroundCount| is still bigger than |trackingRequestCount| because the
> > load blocker is still in the load group until evil.js and evil.css are done.
> > If I don't know how many load blockers are in the load group (though it
> > seems there is always 1 load blocker), I never know when to stop the
> > throbber.
> > 
> > With the load blocker being counted, I know that:
> > LoadGroup - mForegroundCount = 3, trackingRequestCount = 2, loadBlockerCount
> > = 1
> > 
> > Right now, I know that foreground requests consist of only 2 tracking
> > requests and 1 load blocker, so it is safe to stop the throbber.
> > 
> 
> How you know that? There is just one LoadBlocker per document, but load
> blocking count is counted internally 
> in nsDocument
> http://searchfox.org/mozilla-central/rev/
> 46ded325e8f60b3d4c731b27c31d383911056a38/dom/base/nsDocument.h#1535-1536
> 
Current |nsLoadGroup::mForegroundCount| tells me the total count of foreground requests. In my patch, I mark those tracking requests with LOAD_TRACKING_CONTENT, so I know how many tracking requests are there. Moreover, with the dummy interface nsIOnloadBlocker, I know there is only one or zero load blocker in the load group.

At some point, when mForegroundCount == (loadBlockerCount + trackingRequestCount), it's time to stop the throbber.

I am not sure what's wrong in my idea... could you point out to me? Thanks.
Flags: needinfo?(bugs)
Hi Bram,

Please take a look at comment #27.
We might want to stop the throbber a little bit sooner before all things in the page are loaded.
Do you think it's a good idea from UX point of view?

Thanks.
Flags: needinfo?(bram)
I mean that if you have just one nsIOnLoadBlocker in the loadgroup, you don't actually know how many things are currently trying to block onload.
Always when the relevant document's mOnloadBlockCount > 0, we are still in loading phase, and shouldn't stop throbber.
Flags: needinfo?(bugs)
(In reply to Kershaw Chang [:kershaw] from comment #36)
> We might want to stop the throbber a little bit sooner before all things in
> the page are loaded.
> Do you think it's a good idea from UX point of view?

On one hand, I can see why this will result in a change in perceived performance (which is good!). On the other hand, I wonder if the speed improvement is too minor for most users to notice, and will require a lot of work to implement (which means that it’s not worth it).

My gut feeling is to say ‘no’ unless the throbber stops faster by a perceptible amount. I don’t have a precise figure here, but I’d consider ≥ 1s to be a noticeable improvement.

Thoughts?
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #38)
> (In reply to Kershaw Chang [:kershaw] from comment #36)
> > We might want to stop the throbber a little bit sooner before all things in
> > the page are loaded.
> > Do you think it's a good idea from UX point of view?
> 
> On one hand, I can see why this will result in a change in perceived
> performance (which is good!). On the other hand, I wonder if the speed
> improvement is too minor for most users to notice, and will require a lot of
> work to implement (which means that it’s not worth it).
> 
> My gut feeling is to say ‘no’ unless the throbber stops faster by a
> perceptible amount. I don’t have a precise figure here, but I’d consider ≥
> 1s to be a noticeable improvement.
> 
> Thoughts?

Of course the improvement is depended on the network speed and the amount of trackers inside the document. It's hard to tell, but I think in most of cases the improvement should be less than 1s. So, it looks like it's not worth to invest in this bug anymore.

Not sure about others thoughts.
Honza, Olli, could you please provide your comments? Thanks.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(bugs)
I'm worried that if we stop throbber before load event firing, users will try to start using the page when it is not ready yet to process user input.

(what I would like to see is iframes not causing throbber spinning if top level page has already fired load event. I think some other browsers do that already)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #40)
> I'm worried that if we stop throbber before load event firing, users will
> try to start using the page when it is not ready yet to process user input.
> 
> (what I would like to see is iframes not causing throbber spinning if top
> level page has already fired load event. I think some other browsers do that
> already)

I think we should file another bug for this.
Flags: needinfo?(honzab.moz)
Attachment #8819848 - Flags: review?(honzab.moz)
I agree with Bram and Olli.  This is a lot of complicated work for just stopping something that is not the primary factor of how people (IMHO) perceive a browser performance.  

From my experience people either don't see the throbber at all or only check it when the main content of interest (an image, a form, an acrticle) is not showing up.  And in that case we would probably spin.  The interest is rarely in adds or tracker images or scripts (invisible to the user).

Voting for WONTFIX here.
Resolve as WONTFIX according to comment #42 and comment #38.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.