Bug 81466 - Do not copy the script source in the SourceProvider, just reference the existing string
Summary: Do not copy the script source in the SourceProvider, just reference the exist...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
: 79125 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-03-17 22:36 PDT by Benjamin Poulain
Modified: 2012-05-07 12:14 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.24 KB, patch)
2012-03-17 22:40 PDT, Benjamin Poulain
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.