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

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[Kershaw Chang [:kershaw]]

See bug 1141814 comment #21.

This bug is aimed to modify a channel's load flags if the channel is loading a tracker.

Comment 1

[Kershaw Chang [:kershaw]]

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?

Comment 2

[Kershaw Chang [:kershaw]]

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.

Comment 3

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

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.

Comment 4

[Kershaw Chang [:kershaw]]

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

Comment 5

[Honza Bambas (:mayhemer)]

Yep, let's WONTFIX this.

Comment 6

[Kershaw Chang [:kershaw]]

Clear the needinfo flag since this bug is resolved.

Comment 7

[Jason Duell]

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

Comment 8

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

FWIW, I like that idea. We could also use that flag when loading some iframes (ads) etc

Comment 9

[Patrick McManus [:mcmanus]]

we need to talk about this more.

Comment 10

[Patrick McManus [:mcmanus]]

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.

Comment 11

[Kershaw Chang [:kershaw]]

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

Comment 12

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

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.

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Part1: Find a way to get image request proxy

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.

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Part2: Calculate the tracking request count in load group

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

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Part3: Add STATE_DELAY_LOAD_TRACKING_CONTENT

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

Comment 16

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

Could you describe what behavior the patches give? Do we end up stopping throbber before load event has fired? Or what?

Comment 17

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

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?

Comment 18

[Honza Bambas (:mayhemer)]

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 19

[Honza Bambas (:mayhemer)]

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.

Comment 20

[Kershaw Chang [:kershaw]]

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

Comment 21

[Kershaw Chang [:kershaw]]

(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

Comment 22

[Kershaw Chang [:kershaw]]

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

Comment 23

[Honza Bambas (:mayhemer)]

Will answer tomorrow

Comment 24

[Honza Bambas (:mayhemer)]

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

Comment 25

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

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

Comment 26

[Honza Bambas (:mayhemer)]

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

Comment 27

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

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

Comment 28

[Honza Bambas (:mayhemer)]

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

Comment 29

[Kershaw Chang [:kershaw]]

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

Comment 30

[Kershaw Chang [:kershaw]]

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

Comment 31

[Kershaw Chang [:kershaw]]

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

Comment 32

[Kershaw Chang [:kershaw]]

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

Comment 33

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

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

Comment 34

[Patrick McManus [:mcmanus]]

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

Comment 35

[Kershaw Chang [:kershaw]]

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

Comment 36

[Kershaw Chang [:kershaw]]

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.

Comment 37

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

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.

Comment 38

[Bram Pitoyo [:bram]]

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

Comment 39

[Kershaw Chang [:kershaw]]

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

Comment 40

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

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)

Comment 41

[Kershaw Chang [:kershaw]]

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

Comment 42

[Honza Bambas (:mayhemer)]

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.

Comment 43

[Kershaw Chang [:kershaw]]

Resolve as WONTFIX according to comment #42 and comment #38.