Bug 81466

Summary: Do not copy the script source in the SourceProvider, just reference the existing string
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: JavaScriptCoreAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, jberlin, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ggaren: review+, buildbot: commit-queue-

Description Benjamin Poulain 2012-03-17 22:36:13 PDT
When executing scripts, we copy giant strings in the SourceProvider. That is not good and very simple to fix.
Comment 1 Benjamin Poulain 2012-03-17 22:40:07 PDT
Created attachment 132483 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-03-18 20:26:15 PDT
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.
Comment 3 Build Bot 2012-03-19 12:29:56 PDT
Comment on attachment 132483 [details]
Patch

Attachment 132483 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11986329
Comment 4 Geoffrey Garen 2012-03-19 13:40:54 PDT
Comment on attachment 132483 [details]
Patch

r=me

Please add "virtual" to these declarations, as Alexey suggested.
Comment 5 Benjamin Poulain 2012-03-19 22:27:02 PDT
Committed r111358: <http://trac.webkit.org/changeset/111358>
Comment 7 Benjamin Poulain 2012-03-19 23:18:51 PDT
> 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.
Comment 8 Yong Li 2012-05-07 11:50:24 PDT
*** Bug 79125 has been marked as a duplicate of this bug. ***
Comment 9 Yong Li 2012-05-07 12:03:05 PDT
There is no test, neither can I find a reason why there is no test in here or change log.
Comment 10 Benjamin Poulain 2012-05-07 12:08:32 PDT
> 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?
Comment 11 Yong Li 2012-05-07 12:11:48 PDT
(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.
Comment 12 Benjamin Poulain 2012-05-07 12:14:59 PDT
> 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.