Summary: | Do not copy the script source in the SourceProvider, just reference the existing string | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
Component: | JavaScriptCore | Assignee: | 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
Benjamin Poulain
2012-03-17 22:36:13 PDT
Created attachment 132483 [details]
Patch
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 on attachment 132483 [details] Patch Attachment 132483 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11986329 Comment on attachment 132483 [details]
Patch
r=me
Please add "virtual" to these declarations, as Alexey suggested.
Committed r111358: <http://trac.webkit.org/changeset/111358> (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 > 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.
*** Bug 79125 has been marked as a duplicate of this bug. *** There is no test, neither can I find a reason why there is no test in here or change log. > 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?
(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. > 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.
|