Bug 51684 – Remove unused member of PendingScript

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Remove unused member of PendingScript

[Tony Gentilcore]

Created attachment 77569 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77569 [details]
Patch

OK.

[Eric Seidel (no email)]

How does this relate to our eventual desire to show better line numbers for javascript errors?  I assume not at all.

I guess the time we're missing line numbers is document.written JS which I don't know how you'd show line numbers for anyway.

[WebKit Commit Bot]

Comment on attachment 77569 [details]
Patch

Clearing flags on attachment: 77569

Committed r74724: <http://trac.webkit.org/changeset/74724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74724 might have broken Leopard Intel Debug (Tests)

[Antti Koivisto]

We are going to need this code to make inline scripts block on stylesheet loads as required by HTML5. Why was it so important to wipe it out right now?

[Tony Gentilcore]

(In reply to comment #7)
> We are going to need this code to make inline scripts block on stylesheet loads as required by HTML5. Why was it so important to wipe it out right now?

Feel free to add the member back if you need it. It has been dead for several months, so I cleaned it up.

Out of curiosity, are you planning to implement inline script blocking on stylesheet loads? We had initially held off on that because we didn't know of any compat problems so weren't in a rush to take the perf hit. However, if there are any sites that would benefit from it, we should obviously do it.

[Antti Koivisto]

> Feel free to add the member back if you need it. It has been dead for several months, so I cleaned it up.
> 
> Out of curiosity, are you planning to implement inline script blocking on stylesheet loads? We had initially held off on that because we didn't know of any compat problems so weren't in a rush to take the perf hit. However, if there are any sites that would benefit from it, we should obviously do it.

Yeah, I was just working on a patch (which is why I noticed when the code I was using was gone).

This causes small rendering issues all over, for example my bank login page has incorrect layout on first load due to this (https://solo1.nordea.fi/nsp/engine).

Another advantage is that simplifies script and stylesheet loading. We should be able to wipe out most of the nasty updateLayoutIgnorePendingStylesheets() code and probably find performance improvements there too.

Generally there shouldn't be a significant performance hit from this, the preloader picks up the resources that would otherwise have their load start delayed (though you can construct a case which this would slow down). Blocking execution of external scripts was a much bigger change and didn't cause performance issues in practice.

[Antti Koivisto]

Patch in bug 8852 (including reverting this patch)

[Tony Gentilcore]

(In reply to comment #9)
> > Feel free to add the member back if you need it. It has been dead for several months, so I cleaned it up.
> > 
> > Out of curiosity, are you planning to implement inline script blocking on stylesheet loads? We had initially held off on that because we didn't know of any compat problems so weren't in a rush to take the perf hit. However, if there are any sites that would benefit from it, we should obviously do it.
> 
> Yeah, I was just working on a patch (which is why I noticed when the code I was using was gone).

Sorry for the trouble, I obviously wouldn't have done this had I seen your patch.

> 
> This causes small rendering issues all over, for example my bank login page has incorrect layout on first load due to this (https://solo1.nordea.fi/nsp/engine).

I'm not sure if I agree with your point below about blocking external scripts on style not paying a performance cost in practice. While preloading may allow the overall page to complete in the same amount of time, the blocking means that during the load the user may be looking at less rendered content (a hard effect to quantify).

However, I definitely agree that if there are any compat bugs, it should be fixed. Thanks for pointing out the bank login example. I wonder if this is also causing https://bugs.webkit.org/show_bug.cgi?id=50529 (which we haven't reduced yet).

> 
> Another advantage is that simplifies script and stylesheet loading. We should be able to wipe out most of the nasty updateLayoutIgnorePendingStylesheets() code and probably find performance improvements there too.
> 
> Generally there shouldn't be a significant performance hit from this, the preloader picks up the resources that would otherwise have their load start delayed (though you can construct a case which this would slow down). Blocking execution of external scripts was a much bigger change and didn't cause performance issues in practice.