Bug 81466 – Do not copy the script source in the SourceProvider, just reference the existing string

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Benjamin Poulain]

When executing scripts, we copy giant strings in the SourceProvider. That is not good and very simple to fix.

[Benjamin Poulain]

Created attachment 132483 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 132483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132483&action=review

> Source/JavaScriptCore/parser/SourceProvider.h:89
> +        UString getRange(int start, int end) const OVERRIDE

If it's OVERRIDE, it also needs to be virtual. Although it can be implicit in C++, we prefer to spell it out.

> Source/WebCore/bindings/js/StringSourceProvider.h:45
> +        JSC::UString getRange(int start, int end) const OVERRIDE

Ditto.

[Build Bot]

Comment on attachment 132483 [details]
Patch

Attachment 132483 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11986329

[Geoffrey Garen]

Comment on attachment 132483 [details]
Patch

r=me

Please add "virtual" to these declarations, as Alexey suggested.

[Benjamin Poulain]

Committed r111358: <http://trac.webkit.org/changeset/111358>

[Jessie Berlin]

(In reply to comment #5)
> Committed r111358: <http://trac.webkit.org/changeset/111358>

This broke the Mac build:

http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/7932/steps/compile-webkit/logs/stdio

[Benjamin Poulain]

> This broke the Mac build:
> 
> http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/7932/steps/compile-webkit/logs/stdio

Damn, that is dumb. Give me a minute, I'll fix that.

[Yong Li]

*** Bug 79125 has been marked as a duplicate of this bug. ***

[Yong Li]

There is no test, neither can I find a reason why there is no test in here or change log.

[Benjamin Poulain]

> There is no test, neither can I find a reason why there is no test in here or change log.

Are you kidding me? How would you test this?

[Yong Li]

(In reply to comment #10)
> > There is no test, neither can I find a reason why there is no test in here or change log.
> 
> Are you kidding me? How would you test this?

I don't have a quick solution for testing it, either. But I would mention that in change log or bugzilla, as I've been told by many reviewers.

[Benjamin Poulain]

> I don't have a quick solution for testing it, either. But I would mention that in change log or bugzilla, as I've been told by many reviewers.

We also use common sense in reviews.